Bugzilla – Full Text Bug Listing
|
Description
Remy Suen
From a cursory investigation, the tool bar managers seem to be leaking from the link we create in CoolBarToTrimManager's add(int, IContributionItem) method. Our contribution record map grows in size by at least 44 items every time I open and close a workbench window. Action sets seem to be one manifestation of the leak described by comment 2. <actionSet id="org.eclipse.egit.ui.navigation" label="%NavigationActionSet.label" visible="true"> <action class="org.eclipse.egit.ui.internal.commit.OpenCommitAction" icon="icons/obj16/open-commit.gif" id="org.eclipse.egit.ui.commit.OpenCommitAction" label="%OpenCommitAction.label" menubarPath="navigate/open.ext3" style="push" toolbarPath="org.eclipse.search.searchActionSet/Search" tooltip="%OpenCommitAction.tooltip"/> </actionSet> MenuManagerRenderer's cleanUp(MMenu) method doesn't seem to be called for the 'Navigate' menu (amongst others). Though I suppose fixing the original problem of the manager map will probably cascade the clean-up fix to the contribution record map also. I've pushed to Gerrit the proposal of the patch: https://git.eclipse.org/r/#/c/23999/ Daniel Released as http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=0b8935d41933c2c0237d88e7fc8ab4374a2fb818 Thanks Daniel. PW Just for the record: this caused major regressions: bug 432847 and bug 433020. (In reply to Dani Megert from comment #7) > Just for the record: this caused major regressions: bug 432847 and bug > 433020. Or at least bug 433020. The previous patch introduces the regression, see the previous comment. The 'cleanUp' methods of the previous patch don't work correctly for the shared elements. I've prepared the new patch that seems to be fixing the issues: https://git.eclipse.org/r/#/c/25265/ Daniel (In reply to Daniel Rolka from comment #9) > The previous patch introduces the regression, see the previous comment. > The 'cleanUp' methods of the previous patch don't work correctly for the > shared elements. > > I've prepared the new patch that seems to be fixing the issues: > https://git.eclipse.org/r/#/c/25265/ > > Daniel If the new patch is too risky I think we should revert the original commit and return to the bug at the beginning of the Mars. However it would be good to have it fixed now Daniel After looking over the changes, I think it's better to revert the original change and look at this again in 4.5. I'm not convinced that weak maps are the way to go here. The Renderer was the primary owner of the *ContributionRecord (is responsible for its lifecycle). I'd rather see us look at the create/dispose lifecycle. Weak sets/maps are for lifecycles where we don't well define the beginning/end. This is the only commit I could find for this bug. http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=0b8935d41933c2c0237d88e7fc8ab4374a2fb818 PW As Paul said, we are going to look at it at the beginning of the 4.5. We will have more time to catch any issue connected to the change Daniel Reverted with http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=b246817e772936999394dadaff7130d1206ec423 PW Daniel, can you confirm the problem is gone? PW (In reply to Paul Webster from comment #14) > Daniel, can you confirm the problem is gone? > > PW The issue with the missing 'Configure' context menu item - the Bug 433020 - yes. However we still have the serious regression with the disappearing eGit context menu, that has been reported with the Bug 432847 Daniel New Gerrit change created: https://git.eclipse.org/r/67267 (In reply to Eclipse Genie from comment #16) > New Gerrit change created: https://git.eclipse.org/r/67267 Hi all, I just pushed a new gerrit change that fix the leak. If someone of the team could review this contribution to check If did it correctly, it would be great ! Thanks in advance. Best Regards, Hi, The gerrit change associated with this bug has two +1 code reviews. May be someone of the team could check a last time de code and merge the review if everything is fine ? Thank you very much. Axel Thanks for the contribution Axel. It makes sense, and it's a definite improvement. There are still leaks from menus registered with IMenuService#populateContributionManager(). This is typically called with the ViewPart's actionBar's MenuManager which results in the same MenuManager being linked to two different models. This is a problem as the MenuManagerRenderer assumes there's a one-to-one relationship. In my testing, this frequently occurs with ExtendedMarkerViews#init(), called as part of creating a ProblemsView. Opening a view's context menu (e.g., the Project Explorer) also leaks a large number of items. There is a similar massive leak with ToolBarManagerRenderer also requires similar code. Given that the leaks aren't fixed, and we've survived with this issue since 4.1, I'm going to keep this for 4.7. WorkbenchWindow#hardClose() has commented out code to release contributions from IMenuService. Brian, do you have time to look into this again? We've got two patches (see bug 511014 and bug 408896), they seem to be related. (In reply to Andrey Loskutov from comment #21) > Brian, do you have time to look into this again? We've got two patches (see > bug 511014 and bug 408896), they seem to be related. I set the target to 4.7 M6. The mess should be finally cleaned up. Brian, if you can't commit to 4.7 M6 please reassign to me. New Gerrit change created: https://git.eclipse.org/r/89818 Gerrit change https://git.eclipse.org/r/89818 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=388c1c61da34692d0d96036213cc4075f46d106e Gerrit change https://git.eclipse.org/r/67267 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=816f3435bbadbbb83fc129292a6de2a291b3c877 *** Bug 408896 has been marked as a duplicate of this bug. *** Thanks for the contribution, Axel! New Gerrit change created: https://git.eclipse.org/r/99120 New Gerrit change created: https://git.eclipse.org/r/99121 Gerrit change https://git.eclipse.org/r/99121 was merged to [R4_7_maintenance]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=7d14bc1da0c59ec6829e8a445873f6d21755e478 Gerrit change https://git.eclipse.org/r/99120 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=160b68476826a9df2c2178ff044d765763e3dce2 Reverted due to bad regression reported by bug 518036. New Gerrit change created: https://git.eclipse.org/r/99126 New Gerrit change created: https://git.eclipse.org/r/99125 (In reply to Eclipse Genie from comment #33) > New Gerrit change created: https://git.eclipse.org/r/99126 Just the original commit restored back after revert due the regression described in bug 518036. (In reply to Eclipse Genie from comment #34) > New Gerrit change created: https://git.eclipse.org/r/99125 The patch which fixes the issue with dynamic menu contributions described in bug 518036. The problem was that the fix for 389063 called clearModelToContribution() which was modified now to cleanup *also* children for given element too. This was clearly not what the removeDynamicMenuContributions() wanted to achieve. Note for debugging this code on Linux: GTK locks up if during the popup opening another window requests the focus, so the only way I found to debug was to import code in *two* different workspaces, start one Eclipse with the code from vncviewer, *run* (not debug) the application there with the VM argument -agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=8000 and *debug* this started application from the second Eclipse via "attach to remote Java application". New Gerrit change created: https://git.eclipse.org/r/99965 Gerrit change https://git.eclipse.org/r/99965 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=54fae03615dcae3c1f6304f4c89be8abb6c0dcf3 Gerrit change https://git.eclipse.org/r/99126 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=37a8204e57a2b5e9580c9fea6fd5957bce21f7db @Dirk: the regression you've reported in bug 518036 should be fixed now. Could you please take a next I-build and verify? If this would be OK, I would like to backport the fix to 4.7.1. (In reply to Andrey Loskutov from comment #40) > @Dirk: the regression you've reported in bug 518036 should be fixed now. > Could you please take a next I-build and verify? > If this would be OK, I would like to backport the fix to 4.7.1. Ping. (In reply to Andrey Loskutov from comment #41) > (In reply to Andrey Loskutov from comment #40) > > @Dirk: the regression you've reported in bug 518036 should be fixed now. > > Could you please take a next I-build and verify? > > If this would be OK, I would like to backport the fix to 4.7.1. > > Ping. I downloaded eclipse-SDK-I20170703-2000 opened the workspace containing the RCPTest plugin that showed the regression and started the application via .product file. The menu items are not duplicated anymore, so it looks like the regression is solved. Thanks for the fix and the reminder Andrey! :) New Gerrit change created: https://git.eclipse.org/r/100620 New Gerrit change created: https://git.eclipse.org/r/100619 New Gerrit change created: https://git.eclipse.org/r/100621 Gerrit change https://git.eclipse.org/r/100619 was merged to [R4_7_maintenance]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=c147af62fd1794f4fb18c6c1e4dfaa056c949431 Gerrit change https://git.eclipse.org/r/100620 was merged to [R4_7_maintenance]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=8a8bb0f6dcf49f48b734de77224dca2f1c4ed7cf Gerrit change https://git.eclipse.org/r/100621 was merged to [R4_7_maintenance]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=96dc559f38ef55ccbf69f0b4a8ec9ad9c1349da4 OK, everything is now merged into 4.7.1. Let hope we will never hear about this issue again :-) |