Bug 551192

Comment 1 Paul Pazderski 2019-09-18 07:49:57 EDT

Interesting problem. A short debugging showed that QuickAccessDialog.close is called twice. Once before shell is disposed and once after.

Pressing ESC triggers the first close, which dispose the shell, which remove focus from shell, which triggers the next close due to lost focus. All before the shell is marked as disposed.

Could probably fixed by either null the shell before dispose in Window.close or imo better add an additional isDisposed check inside the asyncExec of PopupDialog.asyncClose.

Comment 2 Mickael Istria 2019-09-18 08:33:03 EDT

(In reply to Paul Pazderski from comment #1)
> Could probably fixed by either null the shell before dispose in Window.close
> or imo better add an additional isDisposed check inside the asyncExec of
> PopupDialog.asyncClose.

I don't think fixing the super class is better: calling `close` twice is the bug, and the reason why close is called twice belongs to the QuickAccessDialog focus handler.
To me, the fix would be better to remove the focus listeners from the dialog before closing it.

Comment 3 Paul Pazderski 2019-09-18 08:49:16 EDT

Whatever you think is better. I did not looked deeper into this and thought a close from focus loss is part of the PopupDialog and not QuickAccess specific.

Comment 4 Mickael Istria 2019-09-18 09:29:35 EDT

Unfortunarly, I cannot reproduce it on Fedora 30 with top of master (currently downloading latest I-Build to check), so it seems to be a Platform-specific issue and I don't think I'm able to properly remediate it as I can't even reproduce it.
So I leave it to anyone else ;) Paul's comments are giving the right track to follow (about focus), it's just that this issue is specific to QuickAccessDialog or related classes (QuickAccessContents, SearchField) so the fix should live there and not upstream.

Comment 5 Sarika Sinha 2019-09-20 01:48:28 EDT

I can reproduce on Mac with Build id: I20190918-1800

Comment 6 Manoj N Palat 2019-09-27 00:17:43 EDT

Reproducible on Windows as well..
org.eclipse.swt.SWTException: Widget is disposed
	at org.eclipse.swt.SWT.error(SWT.java:4711)
	at org.eclipse.swt.SWT.error(SWT.java:4626)
...

Comment 7 Sarika Sinha 2019-09-27 00:18:45 EDT

I am seeing it consistently on Mac. Can someone work on this by trying to reproduce on Mac/Windows ?

Comment 8 Vikas Chandra 2019-09-27 01:44:07 EDT

Seeing this on 

Version: 2019-12 (4.14)
Build id: I20190926-0625

I am using windows 7

Comment 9 Mickael Istria 2019-09-30 06:16:41 EDT

Can someone review Ed's patch: https://git.eclipse.org/r/150366 ?
@Ed: which OS are you using?

While the patch seems correct to me, I'm more curious about why .close() gets called twice on some OS (still didn't see it on linux)

Comment 10 Ed Merks 2019-09-30 06:26:40 EDT

I'm also using Windows 7.

But it seems to me it must be getting called twice on all platforms. 

Once here in org.eclipse.jface.dialogs.PopupDialog.asyncClose().

	private void asyncClose() {
		// workaround for https://bugs.eclipse.org/bugs/show_bug.cgi?id=152010
		Shell shell = getShell();
		if (shell != null && !shell.isDisposed()) {
			shell.getDisplay().asyncExec(() -> close());
		}
	}

Which is I would generally expect would come last.

And first in org.eclipse.ui.internal.quickaccess.QuickAccessContents.handleSelection():

	private void handleSelection() {
		QuickAccessElement selectedElement = null;
		String text = filterText.getText().toLowerCase();
		if (table.getSelectionCount() == 1) {
			QuickAccessEntry entry = (QuickAccessEntry) table.getSelection()[0].getData();
			selectedElement = entry == null ? null : entry.element;
		}
		if (selectedElement != null) {
			doClose();
			handleElementSelected(text, selectedElement);
		}
	}

This is called during mouseUp so I generally would expect that to come first on all platforms.

Ah, but now I see org.eclipse.jface.dialogs.PopupDialog.configureShell(Shell) has a guard and this is exactly where the second close happens on Windows (and Mac I would assume?) but not with GTK.

	protected void configureShell(Shell shell) {
		GridLayoutFactory.fillDefaults().margins(0, 0).spacing(5, 5).applyTo(
				shell);
		shell.addListener(SWT.Deactivate, event -> {
			/*
			 * Close if we are deactivating and have no child shells. If we
			 * have child shells, we are deactivating due to their opening.
			 *
			 * Feature in GTK: this causes the Quick Outline/Type Hierarchy
			 * Shell to close on re-size/movement on Gtk3. For this reason,
			 * the asyncClose() call is disabled in GTK. See Eclipse Bugs
			 * 466500 and 113577 for more information.
			 */
			if (listenToDeactivate && event.widget == getShell()
					&& getShell().getShells().length == 0) {
				if (!Util.isGtk()) {
					asyncClose();
				}

Comment 11 Eclipse Genie 2019-09-30 07:15:08 EDT

Gerrit change https://git.eclipse.org/r/150366 was merged to [master].
Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=8ab5177660adac48fbe2d09ecc329bef42dbe9fd