Bugzilla – Full Text Bug Listing
New Gerrit change created: https://git.eclipse.org/r/119744 Please note, display.getShell() returns non-disposed shells. As per javadoc:
/**
* Returns a (possibly empty) array containing all shells which have
* not been disposed and have the receiver as their display.
I tested:
Display display = new Display ();
Shell shell = new Shell(display);
shell.setData(new Integer(1));
Shell shell2 = new Shell(display);
shell2.setData(new Integer(2));
Shell shell3 = new Shell(display);
shell3.setData(new Integer(3));
Shell shell4 = new Shell(display);
shell4.setData(new Integer(4));
shell3.dispose();
for (Shell iShell : display.getShells()) {
System.out.println(iShell.getData());
}
}
1
2
4
If this method is returning a shell that is disposed, then that is a bug in display.getShell();.
Do you have a way to reproduce this issue?
It just occurred to me, that maybe what's happening is that ShellA is somehow causing shellB to dispose. If you have any way to reproduce this, it would be useful to investigate. Thank you for bugfix. Gerrit change https://git.eclipse.org/r/119744 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=b72ec4ee4a25b3b62b1cea617c5fcc5fd2549359 I'm pretty sure this bug is not really closed, because the root cause is not found and not fixed, and also if we want to fix only symptoms, we should fix at least Display.getShells() and not BusyIndicator.
We've just got the same problem from a customer running 4.7.2.
We have a workspace log, and we see that from one point in time the Display.getShells() and Shell.getShells() start to deliver a *disposed* shell in the list. This should never happen.
!ENTRY org.eclipse.jface 4 2 2018-07-17 16:57:36.499
!MESSAGE Problems occurred when invoking code from plug-in: "org.eclipse.jface".
!STACK 0
org.eclipse.swt.SWTException: Widget is disposed
at org.eclipse.swt.SWT.error(SWT.java:4533)
at org.eclipse.swt.SWT.error(SWT.java:4448)
at org.eclipse.swt.SWT.error(SWT.java:4419)
at org.eclipse.swt.widgets.Widget.error(Widget.java:487)
at org.eclipse.swt.widgets.Widget.checkWidget(Widget.java:424)
at org.eclipse.swt.widgets.Widget.getData(Widget.java:541)
at org.eclipse.swt.custom.BusyIndicator.showWhile(BusyIndicator.java:74)
at org.eclipse.ui.internal.progress.ProgressManager.busyCursorWhile(ProgressManager.java:851)
at org.eclipse.ui.internal.progress.ProgressManager.busyCursorWhile(ProgressManager.java:827)
[...]
!ENTRY org.eclipse.ui 4 4 2018-07-17 16:57:36.597
!MESSAGE An internal error has occurred.
!STACK 0
org.eclipse.swt.SWTException: Widget is disposed
at org.eclipse.swt.SWT.error(SWT.java:4533)
at org.eclipse.swt.SWT.error(SWT.java:4448)
at org.eclipse.swt.SWT.error(SWT.java:4419)
at org.eclipse.swt.widgets.Widget.error(Widget.java:487)
at org.eclipse.swt.widgets.Widget.checkWidget(Widget.java:424)
at org.eclipse.swt.widgets.Shell.getShells(Shell.java:1236)
at org.eclipse.ui.internal.progress.ProgressManagerUtil.getModalChildExcluding(ProgressManagerUtil.java:468)
at org.eclipse.ui.internal.progress.ProgressManagerUtil.getModalShellExcluding(ProgressManagerUtil.java:437)
at org.eclipse.ui.internal.progress.ProgressManagerUtil.safeToOpen(ProgressManagerUtil.java:414)
at org.eclipse.ui.internal.progress.ProgressManager$2.runInUIThread(ProgressManager.java:865)
at org.eclipse.ui.progress.UIJob.lambda$0(UIJob.java:95)
at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:37)
at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:182)
at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:4581)
[...]
It looks like we have a branch where org.eclipse.swt.widgets.Display.widgetTable get corrupted. Why this?
This is because Display.getShells() iterates over entire widgetTable and adds *all* found shells to the list. One from added items is a disposed Shell, but the widgetTable supposed to contain only not disposed objects! So how can widgetTable contain a disposed Shell?
So far as I can understand, this problem can only root from the widget disposal, from this code in Display:
Widget removeWidget (long /*int*/ handle) {
if (handle == 0) return null;
lastWidget = null;
Widget widget = null;
int index = (int)/*64*/ OS.g_object_get_qdata (handle, SWT_OBJECT_INDEX) - 1;
if (0 <= index && index < widgetTable.length) {
widget = widgetTable [index];
widgetTable [index] = null;
indexTable [index] = freeSlot;
freeSlot = index;
OS.g_object_set_qdata (handle, SWT_OBJECT_INDEX, 0);
}
return widget;
}
There are few branches where a Shell.dispose() can left his own reference in the table above:
1) if (handle == 0) return null; Somehow shell handle got corrupted. Not sure how.
2) index is negative. This can happen if (int)/*64*/ OS.g_object_get_qdata (handle, SWT_OBJECT_INDEX) will return a negative value:
2.a) Someone saves the negative index for the shell (I don't see right now how).
2.b) the index overflows. index is an integer, and if one constantly increments integers they tend to overflow and the values are starting to be negative :-)
3) index is positive, but does not represent our shell object in the table. Means: somehow index retrieved via OS.g_object_get_qdata() is corrupted (GTK bug?). In this case, we set ANOTHER widget to null via widgetTable [index] = null; and our Shell is still there.
I think we should add some extra checks/logging here.
See also: https://dev.eclipse.org/recommenders/committers/aeri/v2/#!/problems/560a8caae4b0e7ba3c4bda40 https://dev.eclipse.org/recommenders/committers/aeri/v2/#!/problems/55190128e4b026254edff011 https://dev.eclipse.org/recommenders/committers/aeri/v2/#!/problems/58b0bcffe4b0379d94fb4540 New Gerrit change created: https://git.eclipse.org/r/126567 (In reply to Eclipse Genie from comment #8) > New Gerrit change created: https://git.eclipse.org/r/126567 With this patch I've added strict checks for widgetTable consistency during creation/disposal. Patch set 5 in particular shows that DateTime widget leaked itself in the widgetTable, because it registered itself two times with the same handle. See fails in https://git.eclipse.org/r/#/c/126567/5 / https://ci.eclipse.org/platform/job/eclipse.platform.swt-Gerrit/7109/. This DateTime leaks shouldn't affect the shell disposal issue we have here, I will open another bug for that. But they show the possible path for leaking shells. I'm still trying to understand where the disposed shells are leaking, but I believe adding a strict checks as shown in the patch above should help us to identify the root cause. I plan also add such checks (off by default) ASAP. (In reply to Andrey Loskutov from comment #9) > I plan also add such checks (off by default) ASAP. The proposed strict checks should not affect the default SWT behavior if disabled (by default), but I wonder if some of them (which do not affect performance) could be even enabled by default? This could help us to track down memory leaks and widgetTable corruption. Adding SWT team for comments on that. Any opinions on checks added in https://git.eclipse.org/r/#/c/126567/4 ? 1) Should we enable some of them by default? 2) Should we enable some of them during tests? New Gerrit change created: https://git.eclipse.org/r/126614 (In reply to Eclipse Genie from comment #11) > New Gerrit change created: https://git.eclipse.org/r/126614 This is the patch which tries to minimize the damage created by a broken widgetTable. It is NOT the final fix, root cause is still not understood. Some observations here from learning the code: Observation: the widgetTable contains multiple entries of the same widget, based on the widget hierarchy, up to 4: Shell.register() calls Composite.register() and addWidget() shellHandle after that Composite.register() calls Scrollable.register() and addWidget() socketHandle after that Scrollable.register() calls Control.register() and addWidget() scrolledHandle after that Control.register() calls Widget.register() and addWidget() fixedHandle and imHandle after that Widget.register() finally addWidget() handle to Display If anyone from them uses same handle, Display.addWidget() will override the previously used handle index with new one via g_object_set_qdata. On Display.removeWidget() we will then use last index to free the table slot, and the additional slot will be not freed. Regarding the widgetTable design: indexTable is filled with indices pointing to the next free index, by default they are all filled with index + 1. last index is always -1 - if this is found, the tables are re-created with higher size & content is copied. If an entry in the widgetTable is occupied, indexTable[<widgetTable index>] will have -2. widgets in the widgetTable remember they index in the index/widget table via g_object_set_qdata "index +1" they retrieve they index in the index & widget table via object_get_qdata - 1. "freeSlot" int is always pointing to the next free slot (surprise) and the slots for same widget may be occupied in a "random" order over the table. freeSlot is updated every time addWidget() occupies the next slot to the value found in the indexTable at the current freeSlot location. One very dangerous place I've found is Display.setData(String, Object). It allows any client to register/deregister ANY widget with ANY handle !?!?
See
if (key.equals (ADD_WIDGET_KEY)) {
Object [] data = (Object []) value;
long /*int*/ handle = ((LONG) data [0]).value;
Widget widget = (Widget) data [1];
if (widget != null) {
addWidget (handle, widget);
} else {
removeWidget (handle);
}
}
I don't see any callers using this ADD_WIDGET_KEY however...
I think its important to note *where* the exception occurs in org.eclipse.swt.custom.BusyIndicator.showWhile(Display, Runnable).
There are two iterations over the shells, which do the same:
...
Shell[] shells = display.getShells();
for (int i = 0; i < shells.length; i++) {
Integer id = (Integer)shells[i].getData(BUSYID_NAME);
...
}
...
shells = display.getShells();
for (int i = 0; i < shells.length; i++) {
Integer id = (Integer)shells[i].getData(BUSYID_NAME); // <--- SWT assertion
...
}
In our product (based on 4.7.2) runs into an exception in the second loop. In-between, a progress dialog is shown. Since this code runs in the UI thread, maybe exactly this shell caused the problem?
Gerrit change https://git.eclipse.org/r/126567 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=8b62bd6c5c1038d43fa2d6f2d8fa5422749bbf2e Gerrit change https://git.eclipse.org/r/126614 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=80249abf1b2bdadea845ea360026d4551a9521d1 (In reply to Simeon Andreev from comment #14) > I think its important to note *where* the exception occurs in > org.eclipse.swt.custom.BusyIndicator.showWhile(Display, Runnable). Unfortunately we don't know what was in the runnable and how it broke the shells. Also we don't know which from the shells was broken (they are not sorted in any way, as the table positions can be almost random). I've added now strict checks in the Display/Shell classes, which can be enabled via -Dorg.eclipse.swt.internal.gtk.enableStrictChecks options passed to the Eclipse on startup. Those checks are now enabled for SWT tests, and I plan to enable this for our custom product. They should hopefully point to the broken place in the code. @Simeon: WDYT if we enable those checks for the Platform UI Gerrit tests? They run on GTK3. @SWT devs: most of the checks should not affect performance at runtime, but will throw exceptions. I personally think we should enable them by default (similar to the checkWidget() calls they guarantee SWT consistency). Has anyone objections if I will do this for M2? I've noticed other isDisposed() checks in the same area and in particular noticed bug 229022 comment 4. {quote} Display.getShells() answers all non-disposed Shells in this scenario, but 1+ of them becomes disposed by a listener while the list is being held in ProgressManager.setUserInterfaceActive(boolean). {quote} > @Simeon: WDYT if we enable those checks for the Platform UI Gerrit tests? They run on GTK3.
I see no reason not to. Its tests; the more checks the better.
(In reply to Simeon Andreev from comment #18) > I've noticed other isDisposed() checks in the same area and in particular > noticed bug 229022 comment 4. > > {quote} > Display.getShells() answers all non-disposed Shells in this scenario, but 1+ > of them becomes disposed by a listener while the list is being held in > ProgressManager.setUserInterfaceActive(boolean). > {quote} OK, here we have Shell.setCursor(null) and Shell.setData(). Last one doesn't run any GTK code, but setCursor is interesting. We have interesting mix of Control -> Shell -> Control code: Control: public void setCursor (Cursor cursor) { checkWidget(); if (cursor != null && cursor.isDisposed ()) error (SWT.ERROR_INVALID_ARGUMENT); this.cursor = cursor; setCursor (cursor != null ? cursor.handle : 0); } the last call goes into Shell: void setCursor (long /*int*/ cursor) { if (enableWindow != 0) { GDK.gdk_window_set_cursor (enableWindow, cursor); GDK.gdk_flush (); } super.setCursor (cursor); } And the last call goes back to Control: void setCursor (long /*int*/ cursor) { long /*int*/ window = eventWindow (); if (window != 0) { GDK.gdk_window_set_cursor (window, cursor); GDK.gdk_flush (); } } Not sure what the methods are doing, but if they trigger GTK events which go immediately back to SWT and enter the code which will process display loop, we can have really some code executed in between two lines below in BusyIndicator: shells[i].setCursor(null); // here we have a possible unexpected UI processing which disposes the shell shells[i].setData(BUSYID_NAME, null); O-ha. The fix would be always to check if the shell is disposed right after setCursor(). How crazy is this? > Not sure what the methods are doing, but if they trigger GTK events which go immediately back to SWT and enter the code which will process display loop, we can have really some code executed in between two lines below in BusyIndicator:
But the code in BusyIndicator must run from UI thread, otherwise it throws an exception? If the main thread is busy, who can process events?
(In reply to Simeon Andreev from comment #21) > > Not sure what the methods are doing, but if they trigger GTK events which go immediately back to SWT and enter the code which will process display loop, we can have really some code executed in between two lines below in BusyIndicator: > > But the code in BusyIndicator must run from UI thread, otherwise it throws > an exception? If the main thread is busy, who can process events? SWT->GTK->SWT->eventHandler->...->shell is disposed (because user did it before in async request). What I still don't get: OK, let assume this can happen here: but why do we see gazillions of errors AFTER this BusyIndicator.showWhile() code, in all other places where Display.getShells() is used? I can believe, that we manage to run some GTK/SWT code on setCursor(), but I can't believe all other places ALSO manage to run some bad code closing shells right after Display.getShells()? Much simpler explanation is that Display.getShells() returns already disposed shell, and here we are back to the inconsistent Display.widgetTable theory. I ran our products ARTs overnight with the new checks in Shell.deregister and Display.addWidget / Display.removeWidget. I see a few only errors caused by DateTime, nothing about shells. I don't think the DateTime problem can cause a disposed shell; at most the widget table should contain left-over DateTime widgets or shell widgets would be wrongly removed from the table. But the disposed flag of a shell would not be touched by this. I think we need more logging? (In reply to Simeon Andreev from comment #23) > I think we need more logging? If you have ideas where => Gerrit. New Gerrit change created: https://git.eclipse.org/r/126690 New Gerrit change created: https://git.eclipse.org/r/126691 Gerrit change https://git.eclipse.org/r/126690 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=91cef84d181a3ed1b4a2c91526b558b2c87e5a77 Gerrit change https://git.eclipse.org/r/126691 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=69f64942e41085ef7106d4e3b9e33d5e1087473e New Gerrit change created: https://git.eclipse.org/r/126729 Gerrit change https://git.eclipse.org/r/126729 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=575cefefcb59507538ad44f6a3bec859d874c40b We have now lot of SWT checks, test checks and higher level code fixes which should prevent us from using disposed shells OR warn us if we manage to "leak" one somehow. We should see now bug reports with hopefully more interesting stack traces pointing to the root cause at SWT level => we should have a new bug for fixing them, so closing this now. |
The following problem was reported via the automated error reporting: Message: org.eclipse.swt.SWTException: Widget is disposed org.eclipse.swt.SWTException: Widget is disposed at org.eclipse.swt.widgets.Widget.checkWidget(Widget.java:414) at org.eclipse.swt.widgets.Widget.getData(Widget.java:531) at org.eclipse.swt.custom.BusyIndicator.showWhile(BusyIndicator.java:62) at org.eclipse.ui.internal.Workbench.close(Workbench.java:1397) at org.eclipse.ui.internal.Workbench.close(Workbench.java:1370) at org.eclipse.ui.internal.handlers.QuitHandler.execute(QuitHandler.java:37) at org.eclipse.ui.internal.handlers.HandlerProxy.execute(HandlerProxy.java:295) at org.eclipse.ui.internal.handlers.E4HandlerProxy.execute(E4HandlerProxy.java:90) at sun.reflect.GeneratedMethodAccessor.invoke(null:-1) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:606) at org.eclipse.e4.core.internal.di.MethodRequestor.execute(MethodRequestor.java:55) Bundles: | org.eclipse.e4.core.di | 1.5.0.v20150421-2214 | 1.6.100.v20170421-1418 | | org.eclipse.swt | 3.104.0.v20150528-0211 | 3.106.2.v20171129-0543 | | org.eclipse.ui | 3.107.0.v20150507-1945 | 3.109.0.v20170411-1742 | Operating Systems: | Linux | 3.10.0.10 | 4.13.0 | | Windows | 6.1.0 | 6.1.0 | The above information is a snapshot of the collected data. Visit https://dev.eclipse.org/recommenders/committers/aeri/v2/#!/problems/55d2213fe4b0f0b83a6e0c4a for the latest data. Thank you for your assistance. Your friendly error-reports-inbox.