Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 354538 - [Contributions] [Compatibility] Window menus and toolbars leaked after having been closed
Summary: [Contributions] [Compatibility] Window menus and toolbars leaked after having...
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.1   Edit
Hardware: All All
: P3 major with 1 vote (vote)
Target Milestone: 4.7.1   Edit
Assignee: Andrey Loskutov CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 408896 (view as bug list)
Depends on:
Blocks: 402742 518036
  Show dependency tree
 
Reported: 2011-08-11 14:47 EDT by Remy Suen CLA
Modified: 2017-07-04 08:53 EDT (History)
10 users (show)

See Also:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Remy Suen CLA 2011-08-11 14:47:59 EDT 
Our internal map in the MenuManagerRenderer and ToolBarManagerRenderer is increasing in size every time a new workbench window is opened and closed.

Might be worth using a dispose listener for performing the disconnect but we should figure out the source of the leak first.
Comment 1 Remy Suen CLA 2011-08-12 09:33:52 EDT 
From a cursory investigation, the tool bar managers seem to be leaking from the link we create in CoolBarToTrimManager's add(int, IContributionItem) method.
Comment 2 Remy Suen CLA 2011-09-15 12:16:20 EDT 
Our contribution record map grows in size by at least 44 items every time I open and close a workbench window.
Comment 3 Remy Suen CLA 2011-09-16 13:28:05 EDT 
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>
Comment 4 Remy Suen CLA 2011-09-16 15:48:09 EDT 
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.
Comment 5 Daniel Rolka CLA 2014-03-27 10:43:56 EDT 
I've pushed to Gerrit the proposal of the patch: https://git.eclipse.org/r/#/c/23999/

Daniel
Comment 7 Dani Megert CLA 2014-04-18 03:29:48 EDT 
Just for the record: this caused major regressions: bug 432847 and bug 433020.
Comment 8 Dani Megert CLA 2014-04-18 03:31:34 EDT 
(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.
Comment 9 Daniel Rolka CLA 2014-04-18 08:07:21 EDT 
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
Comment 10 Daniel Rolka CLA 2014-04-18 10:14:22 EDT 
(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
Comment 11 Paul Webster CLA 2014-04-21 11:07:10 EDT 
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
Comment 12 Daniel Rolka CLA 2014-04-22 02:08:50 EDT 
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
Comment 14 Paul Webster CLA 2014-04-22 06:30:06 EDT 
Daniel, can you confirm the problem is gone?

PW
Comment 15 Daniel Rolka CLA 2014-04-22 06:37:28 EDT 
(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
Comment 16 Eclipse Genie CLA 2016-02-24 11:58:06 EST 
New Gerrit change created: https://git.eclipse.org/r/67267
Comment 17 Axel RICHARD CLA 2016-02-24 12:01:47 EST 
(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,
Comment 18 Axel RICHARD CLA 2016-04-18 03:59:34 EDT 
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
Comment 19 Brian de Alwis CLA 2016-04-20 22:29:14 EDT 
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.
Comment 20 Brian de Alwis CLA 2016-04-28 00:02:03 EDT 
WorkbenchWindow#hardClose() has commented out code to release contributions from IMenuService.
Comment 21 Andrey Loskutov CLA 2017-01-28 11:09:59 EST 
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.
Comment 22 Andrey Loskutov CLA 2017-01-28 11:46:56 EST 
(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.
Comment 23 Eclipse Genie CLA 2017-01-29 12:21:55 EST 
New Gerrit change created: https://git.eclipse.org/r/89818
Comment 26 Andrey Loskutov CLA 2017-02-02 14:12:38 EST 
*** Bug 408896 has been marked as a duplicate of this bug. ***
Comment 27 Andrey Loskutov CLA 2017-02-05 12:06:51 EST 
Thanks for the contribution, Axel!
Comment 28 Eclipse Genie CLA 2017-06-12 08:54:53 EDT 
New Gerrit change created: https://git.eclipse.org/r/99120
Comment 29 Eclipse Genie CLA 2017-06-12 08:55:38 EDT 
New Gerrit change created: https://git.eclipse.org/r/99121
Comment 32 Dani Megert CLA 2017-06-12 08:57:37 EDT 
Reverted due to bad regression reported by bug 518036.
Comment 33 Eclipse Genie CLA 2017-06-12 09:08:39 EDT 
New Gerrit change created: https://git.eclipse.org/r/99126
Comment 34 Eclipse Genie CLA 2017-06-12 09:08:42 EDT 
New Gerrit change created: https://git.eclipse.org/r/99125
Comment 35 Andrey Loskutov CLA 2017-06-12 09:29:21 EDT 
(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.
Comment 36 Andrey Loskutov CLA 2017-06-12 09:44:53 EDT 
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".
Comment 37 Eclipse Genie CLA 2017-06-23 09:37:12 EDT 
New Gerrit change created: https://git.eclipse.org/r/99965
Comment 40 Andrey Loskutov CLA 2017-06-23 10:36:06 EDT 
@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.
Comment 41 Andrey Loskutov CLA 2017-07-03 10:35:09 EDT 
(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.
Comment 42 Dirk Fauth CLA 2017-07-04 07:13:11 EDT 
(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! :)
Comment 43 Eclipse Genie CLA 2017-07-04 08:10:26 EDT 
New Gerrit change created: https://git.eclipse.org/r/100620
Comment 44 Eclipse Genie CLA 2017-07-04 08:10:30 EDT 
New Gerrit change created: https://git.eclipse.org/r/100619
Comment 45 Eclipse Genie CLA 2017-07-04 08:10:35 EDT 
New Gerrit change created: https://git.eclipse.org/r/100621
Comment 49 Andrey Loskutov CLA 2017-07-04 08:53:43 EDT 
OK, everything is now merged into 4.7.1. Let hope we will never hear about this issue again :-)