Community
Participate
Working Groups
GenericHistoryView used to dispose toolbars from a pageContainer on a showPageRec(). This was changed in the following commit: http://git.eclipse.org/c/platform/eclipse.platform.team.git/commit/bundles/org.eclipse.team.ui/src/org/eclipse/team/internal/ui/history/GenericHistoryView.java?id=8bd7229015f8e34a5c3e23ca9b86504023141b51 This breaks the specified API described in IHistoryPage and can cause performance degradation and lead to UI errors like this: https://bugs.eclipse.org/bugs/show_bug.cgi?id=385272#c155 Here is the Javadoc from IHistoryPage: The old page in the History View (along with its {@link SubActionBars} is disposed - * {@link HistoryPage#dispose()} gets called; interested clients can supply a <code>dispose()</code> method in their subclass. Finally, the new page is shown * and its SubActionBars are activated.
Bingo, this affects MercurialEclipse plugin as well.
(In reply to comment #0) > GenericHistoryView used to dispose toolbars from a pageContainer on a > showPageRec(). This was changed in the following commit: > > http://git.eclipse.org/c/platform/eclipse.platform.team.git/commit/bundles/ > org.eclipse.team.ui/src/org/eclipse/team/internal/ui/history/ > GenericHistoryView.java?id=8bd7229015f8e34a5c3e23ca9b86504023141b51 > > This breaks the specified API described in IHistoryPage and can cause > performance degradation and lead to UI errors like this: > > https://bugs.eclipse.org/bugs/show_bug.cgi?id=385272#c155 > > Here is the Javadoc from IHistoryPage: > > The old page in the History View (along with its {@link SubActionBars} is > disposed - > * {@link HistoryPage#dispose()} gets called; interested clients can supply > a <code>dispose()</code> method in their subclass. Finally, the new page is > shown > * and its SubActionBars are activated. The Javadoc of showPageRec() does not specify anything about creation or disposal - it simply says that the page/control must be shown. If the internal implementation previously created a new page each time and disposed the old one, then that's an internal decision. That decision was changed in 3.8/4.2 where now reuses the page via APIs in IHistoryPage - this sounds reasonable. I also verified that IHistoryPage.dispose() gets called when the view is closed. Further investigations reveal issues/leaks in Platform UI: 1) In 3.8 and 4.2 org.eclipse.ui.part.PageBookView.dispose() misses to dispose the action bar of the default page. ==> Fix is easy: add defaultPageRec.subActionBars.dispose()); 2) in 4.2 it additionally misses to dispose the action bar created here: ActionBars(SubActionBars).<init>(IActionBars, IServiceLocator) line: 126 ActionBars.<init>(IActionBars, IServiceLocator, MPart) line: 43 ViewSite.<init>(MPart, IWorkbenchPart, IWorkbenchPartReference, IConfigurationElement) line: 31 ViewReference.initialize(IWorkbenchPart) line: 142 CompatibilityView(CompatibilityPart).create() line: 288 ... NOTE: This bug applies to all views! Test Case: 1. add a breakpoint to 'SubActionBars' constructor and dispose() methods 2. open any view (e.g. 'Javadoc' view) 3. close the view ==> only the constructor is hit. dispose() is not called. I'm moving this back to Platform UI. Once those bugs are fixed and one can still reproduce bug 385272 comment 155, a new bug should be opened against Team with steps to reproduce the issue.
(In reply to comment #2) > > The Javadoc of showPageRec() does not specify anything about creation or > disposal - it simply says that the page/control must be shown. If the > internal implementation previously created a new page each time and disposed > the old one, then that's an internal decision. You're right, our showPageRec() doesn't dispose. But it's in the API contract for IHistoryPage. It states that: "Once the IHistoryPage is created, IHistoryPage.setInput(Object) is called; this is handled by HistoryPage which clients should subclass for their own IHistoryPage implementations. HistoryPage will in turn call HistoryPage.inputSet() - which clients can use for setting up their IHistoryPage. The old page in the History View (along with its SubActionBars is disposed - HistoryPage.dispose() gets called; interested clients can supply a dispose() method in their subclass. Finally, the new page is shown and its SubActionBars are activated." This call to IHistoryPage.dispose()/subactionbar.dispose() (which their old implementation did during a showPageRec(*)) is missing in their new implementation. But I'll still look at this subactionbar problem. PW
(In reply to comment #2) > 1) In 3.8 and 4.2 org.eclipse.ui.part.PageBookView.dispose() misses to > dispose > the action bar of the default page. > ==> Fix is easy: add defaultPageRec.subActionBars.dispose()); released fix as http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=37239038d643ff02e465fc88a0bcf66fc5d0f6dd PW
(In reply to comment #3) > (In reply to comment #2) > This call to IHistoryPage.dispose()/subactionbar.dispose() (which their old > implementation did during a showPageRec(*)) is missing in their new > implementation. As said in comment 2, dispose() is called, at last when the view closes. It's not called on setting the input because the corresponding page is reused instead of being created - at least that's what I see when testing with CVS or local history. I suggest that if someone still sees a leak with a build that has the fix for this bug here, a new bug should be opened with steps to reproduce that concrete problem.
Released fix, closing views now dispose subactionbars. http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=f5649a39732b647cad70c58dc3e3b0d40e22f117 This still doesn't address the IHistoryPage.dispose() not being called often enough for team contributors. Bogdan, can we still reproduce the replicating toolbar with my changes in R4_2_maintenance? PW
(In reply to comment #4) > (In reply to comment #2) > > 1) In 3.8 and 4.2 org.eclipse.ui.part.PageBookView.dispose() misses to > > dispose > > the action bar of the default page. > > ==> Fix is easy: add defaultPageRec.subActionBars.dispose()); > > released fix as > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=37239038d643ff02e465fc88a0bcf66fc5d0f6dd Dani, should I backport this part of the fix to 3.8.2? PW
(In reply to comment #7) > (In reply to comment #4) > > (In reply to comment #2) > > > 1) In 3.8 and 4.2 org.eclipse.ui.part.PageBookView.dispose() misses to > > > dispose > > > the action bar of the default page. > > > ==> Fix is easy: add defaultPageRec.subActionBars.dispose()); > > > > released fix as > > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > > ?id=37239038d643ff02e465fc88a0bcf66fc5d0f6dd > > Dani, should I backport this part of the fix to 3.8.2? > > PW Yes I think that would be good - the fix looks good to me.
(In reply to comment #6) > Released fix, closing views now dispose subactionbars. > > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/ > ?id=f5649a39732b647cad70c58dc3e3b0d40e22f117 > > This still doesn't address the IHistoryPage.dispose() not being called often > enough for team contributors. > > Bogdan, can we still reproduce the replicating toolbar with my changes in > R4_2_maintenance? > > PW Unfortunately, the replicating toolbar is still there even with the changes. I put breakpoints on all of the new dispose code you added but none of them get hit by the P4 history view.
*** Bug 392175 has been marked as a duplicate of this bug. ***
(In reply to comment #9) > Unfortunately, the replicating toolbar is still there even with the changes. > I put breakpoints on all of the new dispose code you added but none of them > get hit by the P4 history view. I cannot reproduce this with M20121017-1200 and the steps given in bug 392175 comment 11. Bogdan, can you please try again too?
Can't reproduce with latest M-build.
There were several fixes released to fix this bug. Wouldn't it be better to mark this bug FIXED instead of WORKSFORME?
(In reply to comment #13) > There were several fixes released to fix this bug. Wouldn't it be better to > mark this bug FIXED instead of WORKSFORME? Right.
