Community
Participate
Working Groups
When you show the history of some file in CVS, you will see a PageSite instance get instantiated. Place a breakpoint in PageSite's dispose() method and also GenericHistoryView's dispose() method. Close the 'History' view. Only the GHV's dispose() method will get called, the PageSite's one doesn't. :( See also bug 331181 which is another problem related to PageSites and the GHV.
Created attachment 201422 [details] Screenshot depicting the state in question. We dispose our 4.x PopupMenuExtender when the PageSite gets disposed but since the site never gets disposed in a GHV, the PME doesn't get destroyed and we end up causing a large leak comprising of EGit-related objects.
Looking at the code again, it's not clear how Team can resolve this problem (without too much churn). This is because the dispose() method of PageSite is a protected method. PageBookView can call this method because the PBV is in the same package as PageSite (org.eclipse.ui.part). Perhaps PageSite itself shouldn't have been a public API, as this would "force" PBV look-alikes (such as the GenericHistoryView) to extend PBV instead of implementing it themselves (like what the GHV does, it actually extends ViewPart and not PBV). I suppose Team could make GHV subclass PBV instead but I imagine it will have to undergo a bit of testing even if the code changes aren't _huge_.
DJ, I know you've had memory problems with EGit. Were you using 3.x or 4.x? Or have you noticed the problem on both? I haven't analyzed the issue on 3.x yet so I don't know if the same problem occurs there or not.
Created attachment 201449 [details] Screenshot depicting the state in question. I don't think the first screenshot I provided in comment 1 was that good, I think I might have screwed it up. This new one is better and is from 3.x and not 4.x. If PageSite's dispose() method was called then its tool bar manager would've cleared its cache but this has not happened.
For CVS, it seems that the context menu is registered with the view's site so this does not become a problem. Thread [main] (Suspended (breakpoint at line 122 in PopupMenuExtender)) PopupMenuExtender.<init>(String, MenuManager, ISelectionProvider, IWorkbenchPart, boolean) line: 122 PartSite.registerContextMenu(String, MenuManager, ISelectionProvider, boolean, IWorkbenchPart, Collection) line: 113 ViewSite(PartSite).registerContextMenu(String, MenuManager, ISelectionProvider) line: 354 ViewSite(PartSite).registerContextMenu(MenuManager, ISelectionProvider) line: 363 CVSHistoryPage.contributeActions() line: 767 CVSHistoryPage.createControl(Composite) line: 255 GenericHistoryView.createPage(IHistoryPageSource, Object) line: 857 GenericHistoryView.showHistoryPageFor(Object, boolean, boolean, IHistoryPageSource) line: 727 TeamUI.showInputInView(IWorkbenchPage, Object, IHistoryView, IHistoryPageSource) line: 129 TeamUI.showHistoryFor(IWorkbenchPage, Object, IHistoryPageSource) line: 110 ShowHistoryAction$1.run(IProgressMonitor) line: 66 RepositoryManager.run(IRunnableWithProgress, IProgressMonitor) line: 717 CVSAction$2.run(IProgressMonitor) line: 360 CVSAction$3.run() line: 369 BusyIndicator.showWhile(Display, Runnable) line: 70 ShowHistoryAction(CVSAction).run(IRunnableWithProgress, boolean, int) line: 366 ShowHistoryAction.execute(IAction) line: 63 ShowHistoryAction(CVSAction).run(IAction) line: 117 ShowHistoryAction(TeamAction).runWithEvent(IAction, Event) line: 549 ObjectPluginAction(PluginAction).runWithEvent(Event) line: 241
In general I've been using 4.x builds.
(In reply to comment #2) > Looking at the code again, it's not clear how Team can resolve this problem > (without too much churn). This is because the dispose() method of PageSite is a > protected method. Well, I guess they could just extend PageSite and "expose" that dispose() method so it could be called.
DJ had some memory problems and from inspecting his heap dump, he has about 70 megs leaking because of this bug. Either we workaround the problem in EGit using the same code as the CVS plug-in or the code needs to be fixed in Team.
Adding Dani to the CC list (as a PMC rep) as I'm not sure who is working on Team now.
Szymon, can you take a look?
Remy, in 3.x (in I20110927-0800 to be more precise) when I close the History view GHV#dispose() is not called. It only happens when I'm closing the workbench. Did you refer to org.eclipse.team.ui.history.HistoryPage being disposed when switching editors or changing selection. A lame question for a UI guy: are these PageSite instances expected to be disposed at the same time as HistoryPages? From a quick look at PageBookView it seems that PageSites are disposed when disposing the view. As for the solution you suggested: GHV looks like a perfect candidate to be a subclass of PBV, I wonder why it was decided to extend ViewPart directly. Anyway, I would be fine with trying that now for any other view but the History ;) Nevertheless I think I will give it a try as see how much effort it requires.
(In reply to comment #11) > Remy, in 3.x (in I20110927-0800 to be more precise) when I close the History > view GHV#dispose() is not called. It only happens when I'm closing the > workbench. I tested this with I20110927-0800 on Windows XP and GHV's dispose() method _is_ getting invoked. Thread [main] (Suspended (breakpoint at line 906 in GenericHistoryView)) GenericHistoryView.dispose() line: 906 ViewReference(WorkbenchPartReference).doDisposePart() line: 737 ViewReference.doDisposePart() line: 104 ViewReference(WorkbenchPartReference).dispose() line: 684 WorkbenchPage.disposePart(WorkbenchPartReference) line: 1797 WorkbenchPage.partRemoved(WorkbenchPartReference) line: 1789 ViewFactory.releaseView(IViewReference) line: 257 Perspective.hideView(IViewReference) line: 606 WorkbenchPage.hideView(IViewReference) line: 2469 ViewPane.doHide() line: 224 ViewStack(PartStack).close(IPresentablePart) line: 537 ViewStack(PartStack).close(IPresentablePart[]) line: 520 PartStack$1.close(IPresentablePart[]) line: 120 TabbedStackPresentation$1.handleEvent(TabFolderEvent) line: 83 DefaultTabFolder(AbstractTabFolder).fireEvent(TabFolderEvent) line: 269 DefaultTabFolder(AbstractTabFolder).fireEvent(int, AbstractTabItem) line: 278 DefaultTabFolder.access$1(DefaultTabFolder, int, AbstractTabItem) line: 1 DefaultTabFolder$1.closeButtonPressed(CTabItem) line: 71 PaneFolder.notifyCloseListeners(CTabItem) line: 631 PaneFolder$3.close(CTabFolderEvent) line: 206 CTabFolder.onMouse(Event) line: 1598 Please ensure you do _not_ have any other 'History' views open in other perspectives when you are testing this. If you also have the view in the other perspective, then the view is still "alive". Note that if IWorkbenchPart's dispose() method was only called when the workbench shutdown, we would be having major leaking issues. :) > A lame question for a UI > guy: are these PageSite instances expected to be disposed at the same time as > HistoryPages? You're right, the page and the page site should be destroyed. Though the page site should be disposed _after_ the page is destroyed. You can take a look at PageBookView's removePage(PageRec) method. > From a quick look at PageBookView it seems that PageSites are > disposed when disposing the view. This is one of the scenarios, yes. We also destroy pages if the "target" part in question is destroyed. See PageBookView's partClosed(IWorkbenchPart) method. For example, the 'Outline' view has a page in the current Java editor. However, if that Java editor is closed, the page in the 'Outline' view also gets disposed even though the 'Outline' view itself has not been closed.
(In reply to comment #12) > Please ensure you do _not_ have any other 'History' views open in other > perspectives when you are testing this. Got it now, thx.
(In reply to comment #8) > Either we workaround the problem in EGit using the same code as the CVS plug-in > or the code needs to be fixed in Team. Remy, have you looked at how a potential workaround in EGit could look like? The fix in Team is not going to be trivial, so I thought we could fix it in EGit first (to avoid the leak and reduce severity). This would bought me some time to provide a proper fix in Team later on (after 3.8 M3).
(In reply to comment #14) > Remy, have you looked at how a potential workaround in EGit could look like? No. On the surface it seems like it'd just be a matter of changing the context menu registration to use the view's site instead of the page's site but I haven't actually even launched an inner with that change before to be honest.
I can confirm that changing the code to use the view's site instead of the page's site does cause GitHistoryPage, FindToolbar, and SWTCommit objects to have their finalize() methods called.
Remy, any chance to release the workaround to EGit, so we can lower the severity here?
(In reply to comment #17) > Remy, any chance to release the workaround to EGit, so we can lower the > severity here? It's hard to say because I don't know what adverse effects creating the context menu off of the part's site might have. Perhaps it's safe since the CVS plug-in does it this way but I can't be sure. Another workaround might be to just null out the references manually so that the commit objects get garbage collected.
(In reply to comment #17) > Remy, any chance to release the workaround to EGit, so we can lower the > severity here? I've emailed egit-dev asking for input from the community. As to the severity, well, it doesn't really matter to me at the end of the day. One man's bug is another man's enhancement request as I like to say. ;) This bug usually doesn't affect me on a day-to-day basis and I usually just restart Eclipse if it gets out of hand. I have a very high tolerance and I know you all are working hard to find the _right_ fix for this problem. :)
Created attachment 205535 [details] The right fix v01 Here is a first attempt to make GHV a sublclass of PageBookView. I've tested it briefly and it seems to be working fine, it even fixes the bug :) Any chance to move it to early 3.8 M4 so I get extra 2 weeks to check if the new GHV is as good as the old one? Remy, Dani, is that ok with you guys? If I had prepared the fix a week earlier, I wouldn't ask you about it, but it's only a week before M3 and I'm not 100% about the patch.
Created attachment 205537 [details] mylyn/context/zip
(In reply to comment #20) > Created attachment 205535 [details] > The right fix v01 I didn't go over the whole patch but I am a little sketchy about your private implementation of IWorkbenchPart which you call super.partActivated(*) with. I also see that you have a TODO comment there. :P > Here is a first attempt to make GHV a sublclass of PageBookView. I've tested it > briefly and it seems to be working fine, it even fixes the bug :) I can confirm that PageSites are now disposed and that the change to make GHV extend PBV also fixes bug 331181! :) > Any chance to > move it to early 3.8 M4 so I get extra 2 weeks to check if the new GHV is as > good as the old one? I am okay with delaying this to M4. While it is a major problem, the low number of people that are CC'd on this bug leads me to believe that users of EGit are either a) not working with repositories with a long list of commits or are b) filtering their list of commits in the preferences. Since the 'History' view is an integral part of all of our daily development, I don't think we should release this until we are confident about it.
(In reply to comment #22) > (In reply to comment #20) > > Created attachment 205535 [details] [diff] [details] > > The right fix v01 > > I didn't go over the whole patch but I am a little sketchy about your private > implementation of IWorkbenchPart which you call super.partActivated(*) with. I > also see that you have a TODO comment there. :P This definitely sounds like M4 to me ;-)
(In reply to comment #22) > I didn't go over the whole patch but I am a little sketchy about your private > implementation of IWorkbenchPart which you call super.partActivated(*) with. That was not my idea, I noticed this kind of fake parts being used by other PageBookView subclasses: SynchronizeViewWorkbenchPart in SynchronizeView, ConsoleWorkbenchPart in ConsoleView and last but not least DummyPart in SearchView :) I do agree calling super.partActivated(*) smells fishy, I did plan to look at it again. > I also see that you have a TODO comment there. :P Yup, I still need to figure out how to preserve behavior of the old GHV where a new page is *not* created for a file (input) with the same HistoryPageSource. In the "fix v01" a new page is created each time.
Created attachment 206402 [details] Fix v02 (In reply to comment #24) > calling super.partActivated(*) smells fishy, I did plan to look at it again. This works just fine, #partActivated is overridden in GHV so it doesn't call super.partActivated(IWorkbenchPart), it's done in #showHistoryPageFor(...). It may be confusing at first sight, but it looks like a pattern for GHV subclasses which have public methods for displaying a page eg. Console#display(IConsole), SynchronizeView#display(ISynchronizeParticipant) and GHV#showHistoryPageFor(...). > need to figure out how to preserve behavior of the old GHV where a > new page is *not* created for a file (input) with the same HistoryPageSource. This has been acheived by implementing #equals in HistoryPageSourceWorkbenchPart and overriding #getPageRec in GHV. I will double check if making the GHV a PBV actually fixes the bug, then I will do some smoke testing and call GHV#showHistoryPageFor(...) with different configurations (pinned, linked with editor etc) and providers (CVS, SVN, git). If I don't find anything suspicious I will release the fix this week (on Monday at the latest).
(In reply to comment #25) > Created attachment 206402 [details] > Fix v02 Forgot to remove the TODO comment from the patch. It's no longer valid.
Fixed with 8bd7229015f8e34a5c3e23ca9b86504023141b51. Available in builds >=N20111103-2000.
*** Bug 331181 has been marked as a duplicate of this bug. ***
