Community
Participate
Working Groups
Created attachment 132996 [details] possible patch Build ID: 3.4.2.M20090127-1700 Steps To Reproduce: Create a new plugin project using the RCP Mail Template - Add an actionSet extension, I used the "Hello, World" action set template Run example application - Edit OpenViewAction.run change to if(window != null) { try { window.getWorkbench().showPerspective("cbg.perf.example.rcp2.perspective", window); Thread.sleep(500); IWorkbenchPage page = window.getActivePage(); if(page == null) return; page.closePerspective(page.getPerspective(), false, true); } catch (Exception e) { e.printStackTrace(); } } I added a conditional breakpoint at the bottom of the ReferenceHashSet.add(Object,int) method The condition is: if(referenceType == 0 && values.length > 8) System.out.println("RHS + " + System.identityHashCode(this) + " " + elementSize); return false; I'm using the conditional breakpoint to demonstrate that each time I open/close the perspective a strong reference is added to the ReferenceHashSet but not removed. Run the application and press the "plus" action. I can attach my example project if that's preferable. This patch fixes the problem for me, does this look reasonable? More information:
When I deal with a window, I find that removeActionSet is not called even if I close the perspective that added some new actionSets (i.e. going from Java to Debug adds 2 action sets, but closing Debug doesn't remove them). They hang around (probably in the invisible set) until the window is closed (easy to see in the 2 workbench window case). Where should the symmetry of adding a ref and removing a ref be? PW
Wouldn't the most natural symetery be to remove them when the perspective is closed? Whoever was responsbile for adding them should also remove them.
(In reply to comment #2) > Wouldn't the most natural symetery be to remove them when the perspective is > closed? Whoever was responsbile for adding them should also remove them. The IActionSetDescriptors are designed to be removed when the window is closed, not the perspective. But I see that PluginActionSetBuilder is adding a org.eclipse.ui.internal.PluginActionSetBuilder.Binding as a strong ref. window.getExtensionTracker().registerObject( set.getConfigElement().getDeclaringExtension(), binding, IExtensionTracker.REF_STRONG); I can only find org.eclipse.ui.internal.WorkbenchWindow.actionSetHandler.new IExtensionChangeHandler() {...}.removeExtension(IExtension, Object[]) that deals with the Binding. That implies it would only be removed if the IExtension itself is. Investigation continues. PW
We'll have another look at this later. PW
Looks like, this leak happens only when all the perspectives closed (which Eclipse SDK does not allow the user to do it) then a perspective opened again. Our application allows the user to close all the perspectives. When all the perspectives closed, Eclipse closes the workbench page, also. My guess is probably Eclipse needs to call UIExtensionTracker.unregisterObject() when the workbench page closed. Just in case, here is the call stack when registerObject is called (where the leak happens when the user closes all the perspectives and reopen a perspective) : ReferenceHashSet.add(Object, int) line: 177 UIExtensionTracker(ExtensionTracker).registerObject(IExtension, Object, int) line: 100 PluginActionSetBuilder.readActionExtensions(PluginActionSet, IWorkbenchWindow) line: 280 PluginActionSetBuilder.processActionSets(ArrayList, WorkbenchWindow) line: 221 ActionPresentation.setActionSets(IActionSetDescriptor[]) line: 187 WorkbenchWindow.updateActionSets() line: 3142 WorkbenchPage$ActionSwitcher.updateActionSets(ArrayList) line: 534 WorkbenchPage$ActionSwitcher.updateActivePart(IWorkbenchPart) line: 367 WorkbenchPage.setActivePart(IWorkbenchPart) line: 3489 WorkbenchPage.activate(IWorkbenchPart) line: 610 WorkbenchPage.makeActive(IWorkbenchPartReference) line: 1228 WorkbenchPage.updateActivePart() line: 1208 WorkbenchPage.updateVisibility(Perspective, Perspective) line: 3634 WorkbenchPage.onActivate() line: 2594 WorkbenchWindow$25.run() line: 2873 BusyIndicator.showWhile(Display, Runnable) line: 70 WorkbenchWindow.setActivePage(IWorkbenchPage) line: 2854 WorkbenchWindow.busyOpenPage(String, IAdaptable) line: 759 WorkbenchWindow$7.run() line: 1711 BusyIndicator.showWhile(Display, Runnable) line: 70 WorkbenchWindow.openPage(String, IAdaptable) line: 1708 Workbench.showPerspective(String, IWorkbenchWindow) line: 2556
Created attachment 139469 [details] Another potential patch We are currently evaluating this patch. On the surface it appears to be less risky than the previous patch.
Created attachment 139568 [details] Updated Patch I have updated the patch based on feedback. I've also implemented some support for dispose handling on action sets so that we can remove the bindings at the proper time. The use of SWT's DisposeListener may seem bogus but it's there.
Created attachment 139577 [details] PluginActionSet v03 I get a lot of errors using the Updated Patch. java.lang.IllegalArgumentException: null source at java.util.EventObject.<init>(EventObject.java:38) at org.eclipse.swt.internal.SWTEventObject.<init>(SWTEventObject.java:38) at org.eclipse.swt.events.TypedEvent.<init>(TypedEvent.java:71) at org.eclipse.swt.events.DisposeEvent.<init>(DisposeEvent.java:35) at org.eclipse.ui.internal.PluginActionSet.dispose(PluginActionSet.java:86) Matt, could you have a look at the attached patch. It should be the same flavour as your patch, just refactored to use IDisposable instead of attaching listeners. PW
(In reply to comment #8) > Created an attachment (id=139577) [details] > PluginActionSet v03 ^^^ PW
(In reply to comment #9) Thanks Paul, The latest patch appears to resolve the leaks for us. We're going to execute some more tests and will update with results. Is this something you think is worthy of a 3.4.x
(In reply to comment #10) > Is this something you think is worthy of a 3.4.x While there's always risk associated with backporting actionSet fixes, this one should be no more "risky" in 3.4.x than in 3.5. The patch should apply to 3.4.x with only minor modification (possibly different line numbers). But I'll also mention this leak has never been raised as a major problem by the community in 3.4.x. PW
Boris, could I get a +1 for 3.5.1 PW
(In reply to comment #10) > The latest patch appears to resolve the leaks for us. We're going to execute > some more tests and will update with results. Have you done this testing? Were the results good?
Paul, +1 from me under the assumption that Matt has tested this fix. Will this be released to 3.6 HEAD as well?
(In reply to comment #14) > Paul, +1 from me under the assumption that Matt has tested this fix. Will this > be released to 3.6 HEAD as well? Yes, Matt and Chris tested this fix thoroughly. I'll be releasing all of me 3.5.1 fixes to HEAD as well. PW
(In reply to comment #8) > Created an attachment (id=139577) [details] > PluginActionSet v03 Released to R3_5_maintenance PW
In M20090826-1100 by testing and inspection. PW
