Bugzilla – Full Text Bug Listing
| Summary: | [Viewers] Protect against label providers that spin the event loop | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Boris Bokowski <bokowski> | ||||||||
| Component: | UI | Assignee: | Boris Bokowski <bokowski> | ||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | brockj, Ed.Merks, jared_burns, Kenn.Hussey, markus.kell.r, Michael.Valenta, n.a.edgar, tom.schindl, wuntoy, zaphod | ||||||||
| Version: | 3.2 | ||||||||||
| Target Milestone: | 3.3 M7 | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Boris Bokowski
*** Bug 121483 has been marked as a duplicate of this bug. *** *** Bug 139328 has been marked as a duplicate of this bug. *** *** Bug 145807 has been marked as a duplicate of this bug. *** See the above bugs for stack traces. Oops, wrong assignee. I've got similar exceptions from the Synchronize view in I20061114-1636. org.eclipse.swt.SWTException: Failed to execute runnable (java.lang.IllegalArgumentException: Index out of bounds) at org.eclipse.swt.SWT.error(SWT.java:3404) at org.eclipse.swt.SWT.error(SWT.java:3327) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:126) at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:3440) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3080) at org.eclipse.ui.internal.Workbench.runEventLoop(Workbench.java:1936) at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:1900) at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:422) at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149) at org.eclipse.ui.internal.ide.IDEApplication.run(IDEApplication.java:95) at org.eclipse.core.internal.runtime.PlatformActivator$1.run(PlatformActivator.java:78) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:104) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:74) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:348) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:165) 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:585) at org.eclipse.core.launcher.Main.invokeFramework(Main.java:341) at org.eclipse.core.launcher.Main.basicRun(Main.java:285) at org.eclipse.core.launcher.Main.run(Main.java:987) at org.eclipse.core.launcher.Main.main(Main.java:962) Caused by: java.lang.IllegalArgumentException: Index out of bounds at org.eclipse.swt.SWT.error(SWT.java:3388) at org.eclipse.swt.SWT.error(SWT.java:3327) at org.eclipse.swt.SWT.error(SWT.java:3298) at org.eclipse.swt.widgets.TreeItem.findPrevious(TreeItem.java:210) at org.eclipse.swt.widgets.TreeItem.<init>(TreeItem.java:119) at org.eclipse.jface.viewers.TreeViewer.createNewRowPart(TreeViewer.java:947) at org.eclipse.jface.viewers.TreeViewer.newItem(TreeViewer.java:467) at org.eclipse.jface.viewers.AbstractTreeViewer.createTreeItem(AbstractTreeViewer.java:795) at org.eclipse.jface.viewers.AbstractTreeViewer.updateChildren(AbstractTreeViewer.java:2546) at org.eclipse.jface.viewers.AbstractTreeViewer.internalRefreshStruct(AbstractTreeViewer.java:1718) at org.eclipse.jface.viewers.TreeViewer.internalRefreshStruct(TreeViewer.java:860) at org.eclipse.jface.viewers.AbstractTreeViewer.internalRefresh(AbstractTreeViewer.java:1693) at org.eclipse.jface.viewers.AbstractTreeViewer.internalRefresh(AbstractTreeViewer.java:1649) at org.eclipse.jface.viewers.AbstractTreeViewer.internalRefresh(AbstractTreeViewer.java:1635) at org.eclipse.jface.viewers.StructuredViewer$7.run(StructuredViewer.java:1391) at org.eclipse.jface.viewers.StructuredViewer.preservingSelection(StructuredViewer.java:1326) at org.eclipse.jface.viewers.StructuredViewer.refresh(StructuredViewer.java:1389) at org.eclipse.jface.viewers.StructuredViewer.refresh(StructuredViewer.java:1348) at org.eclipse.team.internal.ui.synchronize.AbstractSynchronizeModelProvider.refreshModelRoot(AbstractSynchronizeModelProvider.java:352) at org.eclipse.team.internal.ui.synchronize.AbstractSynchronizeModelProvider.access$0(AbstractSynchronizeModelProvider.java:345) at org.eclipse.team.internal.ui.synchronize.AbstractSynchronizeModelProvider$2.run(AbstractSynchronizeModelProvider.java:338) at org.eclipse.swt.custom.BusyIndicator.showWhile(BusyIndicator.java:67) at org.eclipse.team.internal.ui.Utils$4.run(Utils.java:760) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:123) ... 20 more And also: Caused by: java.lang.IllegalArgumentException: Index out of bounds at org.eclipse.swt.SWT.error(SWT.java:3388) at org.eclipse.swt.SWT.error(SWT.java:3327) at org.eclipse.swt.SWT.error(SWT.java:3298) at org.eclipse.swt.widgets.TreeItem.findPrevious(TreeItem.java:210) at org.eclipse.swt.widgets.TreeItem.<init>(TreeItem.java:119) at org.eclipse.jface.viewers.TreeViewer.createNewRowPart(TreeViewer.java:947) at org.eclipse.jface.viewers.TreeViewer.newItem(TreeViewer.java:467) at org.eclipse.jface.viewers.AbstractTreeViewer.createTreeItem(AbstractTreeViewer.java:795) at org.eclipse.jface.viewers.AbstractTreeViewer.createAddedElements(AbstractTreeViewer.java:370) at org.eclipse.jface.viewers.AbstractTreeViewer.internalAdd(AbstractTreeViewer.java:261) at org.eclipse.jface.viewers.TreeViewer.internalAdd(TreeViewer.java:812) at org.eclipse.jface.viewers.AbstractTreeViewer.add(AbstractTreeViewer.java:137) at org.eclipse.team.internal.ui.synchronize.SynchronizeModelUpdateHandler.internalRunViewUpdate(SynchronizeModelUpdateHandler.java:586) at org.eclipse.team.internal.ui.synchronize.SynchronizeModelUpdateHandler.access$1(SynchronizeModelUpdateHandler.java:561) at org.eclipse.team.internal.ui.synchronize.SynchronizeModelUpdateHandler$5.run(SynchronizeModelUpdateHandler.java:540) at org.eclipse.swt.custom.BusyIndicator.showWhile(BusyIndicator.java:67) at org.eclipse.team.internal.ui.synchronize.SynchronizeModelUpdateHandler$4.run(SynchronizeModelUpdateHandler.java:538) at org.eclipse.ui.internal.UILockListener.doPendingWork(UILockListener.java:155) at org.eclipse.ui.internal.UISynchronizer$1.run(UISynchronizer.java:36) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:123) ... 20 more Michael, could you have a look at Markus's stack trace? Do you have any idea as to why this is happening? It is just a theory of mine that it could have to do with label providers that spin the event loop, and I am looking for anything that can support or falsify my theory. All I can really say is that the Team code involved in this stack trace has not undergone any change since 3.1 and this is the first time I have seen a trace like this. Therefore, my guess is it is caused by recent changes elsewhere. Markus, did this happen at startup or shortly thereafter? Could it be that a label provider (or some of the code it called) tried to acquire a lock that was held by a background job, which caused the "Operation is waiting for a background job" progress dialog to open? It happened hours after my last startup. However, I have "Always run in background" set to true, and I often cancel blocking compiler jobs (but not other jobs). Just FYI: I have not seen the exception from comment 6 any more (and I'm not aware of changes in my usage patterns that could have caused this). Nick had another one of these stack traces, and together we were able to come up with an explanation for at list his case: Context: It happened in a view that updated itself every five minutes. 1. The first update calls refresh() on a table viewer. 2. In internalRefreshAll(), some of the items are disassociated because their data no longer matches the element at that index 3. After disassociating some or all of the existing items, we do a second pass through the items, bringing back their data and updating their text etc. (It has to be done in two passes or else the element map might end up corrupted.) 4. For one of these items, the label provider threw a NPE and we opened an error dialog. This dialog spins the event loop, which is bad because it allows others to post asyncExecs when we don't expect it. (Also, Nick was not at his computer when the error dialog came up, so it stayed up for some time.) 5. One of those asyncExecs happened to be (after 5 minutes) the next refresh, calling into TableViewer reentrantly. This is bad because: 6. At this time, some items did not have associated data, which caused assertion failures. What we can do is: - don't open a modal error dialog for every exception happening in label providers - detect cases where viewer code is called reentrantly and ignore the second update request, logging the error because we consider it a programming error to spin the event loop from within a label provider. Target milestone was set to 3.2M5 but should be 3.3M5. Moving to M6 *** Bug 173535 has been marked as a duplicate of this bug. *** Created attachment 62175 [details]
patch
Patch released >20070327, to see what this does to the nightly build.
We'll have to see how this plays out. With this patch, reentrant calls to public methods are detected, logged, and then ignored. As explained in comment 12, this can happen if label providers/content providers call back into the viewer, or if they spin the event loop which then causes asyncExecs to run. When I last talked with Nick about this, our consensus was that it is safer to ignore these calls than to try and process them because otherwise the viewer's data structures can be corrupted. We should also punt the error dialog in SafeRunnable, or avoid using it in the viewers, if that hasn't been done already. Created attachment 62287 [details]
next patch (incremental)
(In reply to comment #18) > We should also punt the error dialog in SafeRunnable, or avoid using it in the > viewers, if that hasn't been done already. Yes. See bug 168301. I have reverted both patches - it turned out to be more complicated than I thought. I have another idea for how to approach this, which I hope to implement tomorrow. Created attachment 65145 [details]
patch
New attempt, less ambitious...
Patch released >20070426-0010. (I.e. it is not in the midnight I build.) Verified - we are seeing useful stack traces being reported now (see e.g. bug 184991) This change has caused a serious problem for EMF. https://bugs.eclipse.org/bugs/show_bug.cgi?id=191010 It's not unusual for label computations to reference data that might cause proxies to resolve and such a resolve can lead to resources being loaded, which then updates the view to show these new resource as children. This has never caused a problem, but now it's broken. If we could detect that a viewer is busy, we could minimize the impact of changes we make to EMF, but since we can't know a viewer is busy, we can't know an attempt to update it will fail and that we must try later. So while this fix may have fixed a problem for someone, it's not a problem we ever saw manifested, but now we do have a problem that requires us to make significant changes to the update behavior that I'm not totally comfortable with. So this change has caused problems we didn't have before and if we react, we may well cause new problems for those downstream from us. Would you consider making isBusy available for clients? I talked to Ed and he said he is investigating possible fixes on their side. I would suggest continuing the discussion on bug 191010. If it turns out to be too difficult or risky to fix this in EMF, that bug can be given to us. We need to act quickly though, we are in the last week for 3.3! |