Community
Participate
Working Groups
HEAD Widget is disposed exception when refreshing TreeViewer after modifying a filter. Steps: - check out HEAD of org.eclipse.jdt.astview from repository /cvsroot/eclipse , module jdt-ui-home/plugins/org.eclipse.jdt.astview - debug a new runtime workbench - in the runtime workbench, paste this to the Package Explorer: public class C {} - open view "ASTView" - expand node "TYPES (1)" > "TypeDeclaration [0, 18]" > "> type binding: C" - in the view menu, toggle the second-to-last action (Hide Non-Relevant Att.) => each time when I check the action (i.e. enable the filter), I get the SWTException below. The ASTView uses a normal TreeViewer and doesn't fiddle around with TreeItems and their "data" properties, so this must be a problem of the TreeViewer implementation. !ENTRY org.eclipse.ui 4 0 2010-10-05 15:13:45.990 !MESSAGE Unhandled event loop exception !STACK 0 org.eclipse.swt.SWTException: Widget is disposed at org.eclipse.swt.SWT.error(SWT.java:4083) at org.eclipse.swt.SWT.error(SWT.java:3998) at org.eclipse.swt.SWT.error(SWT.java:3969) at org.eclipse.swt.widgets.Widget.error(Widget.java:477) at org.eclipse.swt.widgets.Widget.checkWidget(Widget.java:349) at org.eclipse.swt.widgets.Widget.getData(Widget.java:534) at org.eclipse.jface.viewers.AbstractTreeViewer.updateChildren(AbstractTreeViewer.java:2623) at org.eclipse.jface.viewers.AbstractTreeViewer.internalRefreshStruct(AbstractTreeViewer.java:1867) at org.eclipse.jface.viewers.TreeViewer.internalRefreshStruct(TreeViewer.java:721) at org.eclipse.jface.viewers.AbstractTreeViewer.internalRefreshStruct(AbstractTreeViewer.java:1874) at org.eclipse.jface.viewers.TreeViewer.internalRefreshStruct(TreeViewer.java:721) at org.eclipse.jface.viewers.AbstractTreeViewer.internalRefreshStruct(AbstractTreeViewer.java:1874) at org.eclipse.jface.viewers.TreeViewer.internalRefreshStruct(TreeViewer.java:721) at org.eclipse.jface.viewers.AbstractTreeViewer.internalRefreshStruct(AbstractTreeViewer.java:1874) at org.eclipse.jface.viewers.TreeViewer.internalRefreshStruct(TreeViewer.java:721) at org.eclipse.jface.viewers.AbstractTreeViewer.internalRefresh(AbstractTreeViewer.java:1842) at org.eclipse.jface.viewers.AbstractTreeViewer.internalRefresh(AbstractTreeViewer.java:1799) at org.eclipse.jface.viewers.AbstractTreeViewer.internalRefresh(AbstractTreeViewer.java:1785) at org.eclipse.jface.viewers.StructuredViewer$7.run(StructuredViewer.java:1487) at org.eclipse.jface.viewers.StructuredViewer.preservingSelection(StructuredViewer.java:1422) at org.eclipse.jface.viewers.TreeViewer.preservingSelection(TreeViewer.java:403) at org.eclipse.jface.viewers.StructuredViewer.preservingSelection(StructuredViewer.java:1383) at org.eclipse.jface.viewers.StructuredViewer.refresh(StructuredViewer.java:1485) at org.eclipse.jface.viewers.ColumnViewer.refresh(ColumnViewer.java:537) at org.eclipse.jface.viewers.StructuredViewer.refresh(StructuredViewer.java:1444) at org.eclipse.jdt.astview.views.ASTView.performFilterNonRelevant(ASTView.java:1329) at org.eclipse.jdt.astview.views.ASTView$16.run(ASTView.java:967) at org.eclipse.jface.action.Action.runWithEvent(Action.java:498) at org.eclipse.jface.action.ActionContributionItem.handleWidgetSelection(ActionContributionItem.java:586) at org.eclipse.jface.action.ActionContributionItem.access$2(ActionContributionItem.java:503) at org.eclipse.jface.action.ActionContributionItem$5.handleEvent(ActionContributionItem.java:413) at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:89) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1062) at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:4083) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3674) at org.eclipse.ui.internal.Workbench.runEventLoop(Workbench.java:2629) at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:2593) at org.eclipse.ui.internal.Workbench.access$4(Workbench.java:2427) at org.eclipse.ui.internal.Workbench$7.run(Workbench.java:670) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:332) at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:663) at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149) at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:115) at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:369) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:179) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:621) at org.eclipse.equinox.launcher.Main.basicRun(Main.java:576) at org.eclipse.equinox.launcher.Main.run(Main.java:1409) at org.eclipse.equinox.launcher.Main.main(Main.java:1385)
A subtle bug... good catch.
This came up again recently in bug 294650 and there are some more bugzilla reports regarding that code area.
*** Bug 298750 has been marked as a duplicate of this bug. ***
Created attachment 192088 [details] Fix
While reproducing the problem, I realized that org.eclipse.jdt.astview.views.Binding#equals(Object) has a bug: It doesn't compare fLabel and thus answers true for some elements that are not actually equal. After I fixed that bug in HEAD, I can no longer reproduce with the steps from comment 0 (and without applying a patch to AbstractTreeViewer).
Created attachment 192104 [details] Fix 2 The underlying problem is this: AbstractTreeViewer#updateChildren(*) can throw an exception if the parent node contains multiple equal elements that are not identical. This is illegal -- ATV only supports equal elements with different parent chains. Instead of trying to deal with the bad content provider, we better throw an exception that explains the problem when it occurs.
Dani, can you please review the second patch? NOTE: To reproduce in the ASTView with steps from comment 0, you now have to revert org.eclipse.jdt.astview.views.Binding to 1.36.
(In reply to comment #7) > Dani, can you please review the second patch? Looked at it together with Markus: the equality thing is a big issue and a big bug in the client code but we can't throw an exception at this point as it would cause existing running clients to fail. Instead we need to add a log entry if DEBUG is set (probably in the content provider code). Having said that, the patch from comment 4 is still needed since the code there is broken and only works by luck even if equals thing is fixed.
We currently don't know of a concrete case where fixing equals(Object) is not sufficient to hide the "Widget is disposed" exception, but it makes sense to fix the loop and avoid throwing exceptions whenever we can. We've accepted bad content providers for too long now, and we shouldn't start throwing even more exceptions now. I've filed bug 341259 for a general facility to find broken content providers (with a debug flag).
Comment on attachment 192088 [details] Fix Dani's fix (attachment 192088 [details]) is the one to be reviewed for this bug.
(In reply to comment #8) > the patch from comment 4 is still needed since the code there is broken > and only works by luck even if equals thing is fixed. Could you specify what is broken? I find that code to be already unnecessary complicated and hard to understand. The proposed patch makes it even more complex.
By the way, original code was added for bug 161805.
(In reply to comment #4) > Created attachment 192088 [details] > Fix The proposed fix is broken. Consider: items = {a, b, c, d, e} children = {a, d, e} Unless I am missing something, the result is items = {a, e} and "min" set to 0. Note that "d" is lost and having "min" set to 0 breaks the rest of the code in the updateChildren() method.
>The proposed fix is broken. Yes, I agree. After a second review, I see that 'min' is now decreased too often and that the main bug in the code is the following condition: items.length - i <= numItemsToDispose This starts to randomly delete elements after some point (more on this later). > Could you specify what is broken? Yes, sorry. I should have spelled that out a bit more. Markus and I looked at it and didn't dump our findings. First, you can see that the code fails (widget disposed error) if you use the steps given in comment 0 *** but check out 'R1_1_7' *** (as in HEAD Markus already fixed the equals code). Second, let's look at the code: The code is intended to delete 'numItemsToDispose' elements from the 'items' array. However, the loop can delete items if items.length - i <= numItemsToDispose and in that case the code wrongly decreases 'numItemsToDispose' which can then result in the following problems: 0. Elements which are not in 'elementChildren' might get deleted. 1. Items will not get disposed if 'numItemsToDispose' is 0 too early. 2. Shifting to the left leaves the right elements in the 'items' array and if it shifted (i.e. disposed) items so that 'numItemsToDispose' is not yet 0 but the next element already got disposed/shifted, then getData() throws an exception (this bug). 3. Since 'min' is not decreased, deleted items might be accessed in the following code that loops over 'items' while 'i' < 'min' and blow up. > I find that code to be already unnecessary > complicated and hard to understand. Yes, I completely agree on that, however, I wanted the fix to stay local and minimal. Understanding all angles of the very complicated code, rewrite it and having it reviewed is out of scope - at least for me. And also, the less we touch there the better. I'll try to follow-up with a new patch shortly.
Created attachment 192171 [details] Improved fix A closer look reveals that the condition items.length - i <= numItemsToDispose is just wrong and needs to be deleted: it does not make any sense to remove arbitrary elements just because the index i is >= items.length - numItemsToDispose. Does anyone see a good reason why this should stay there?
(In reply to comment #15) > Created attachment 192171 [details] [diff] > Improved fix I think that's the way to go for 3.7.
Created attachment 192180 [details] Test Case This is a test case that shows the widget disposed error if there are equal elements in the viewer and that is green with the improved fix/patch.
(In reply to comment #15) > Created attachment 192171 [details] > Improved fix > Looks good to me. > A closer look reveals that the condition > items.length - i <= numItemsToDispose > is just wrong and needs to be deleted: it does not make any sense to remove > arbitrary elements just because the index i is >= items.length - > numItemsToDispose. I think this works all right as long as there are no equal children for a given parent. My guess is that the logic behind this statement is: if we are asked to remove 3 elements, and there are only 3 elements left, then they can be simply removed without the need to check "children.containsKey(data)".
Thanks for the feedback Oleg! I've committed the fix plus test to HEAD.
Verified in I20110425-1800.
