Find/replace overlay: unify async executions, avoid NPEs #2244

Several operation in the find/replace overlay need to be executed
asynchronously as they may be called in OS callback which can lead to
deadlocks in GTK. Currently, this processing is distributed and repeated
across the class and did not check preconditions (in particular the
dialog being opened and its shell being present) appropriate in every
case. In some cases, when the overlay is already or temporarily closed
but an asynchronous task is still to be executed, that task may fail
with an NPE.

This change moves the functionality for the asynchronous processing to a
central place, ensuring that the precondition requiring the shell to
exist to always be evaluated. Factoring out this functionality also
makes explicit that the called operation is executed only if the overlay
respectively its shell are currently open. It also fixes the mentioned
NPEs.

Fixes #2244

Author: HeikoKlare

bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/findandreplace/overlay/FindReplaceOverlay.java

@@ -16,7 +16,7 @@
 import static org.eclipse.ui.internal.findandreplace.overlay.FindReplaceShortcutUtil.registerActionShortcutsAtControl;
 
 import java.util.List;
-import java.util.function.Supplier;
+import java.util.function.Consumer;
 
 import org.osgi.framework.FrameworkUtil;
 
@@ -156,7 +156,7 @@ public FindReplaceOverlay(Shell parent, IWorkbenchPart part, IFindReplaceTarget
 		setShellStyle(SWT.MODELESS);
 		setBlockOnOpen(false);
 		targetPart = part;
-		targetPartVisibilityHandler = new TargetPartVisibilityHandler(targetPart, this::getShell, this::close,
+		targetPartVisibilityHandler = new TargetPartVisibilityHandler(targetPart, this::asyncExecIfOpen, this::close,
 				this::updatePlacementAndVisibility);
 	}
 
@@ -195,21 +195,26 @@ private void performSelectAll() {
 	private ControlListener shellMovementListener = new ControlListener() {
 		@Override
 		public void controlMoved(ControlEvent e) {
-			asyncUpdatePlacementAndVisibility();
+			asyncExecIfOpen(FindReplaceOverlay.this::updatePlacementAndVisibility);
 		}
 
 		@Override
 		public void controlResized(ControlEvent e) {
-			asyncUpdatePlacementAndVisibility();
+			asyncExecIfOpen(FindReplaceOverlay.this::updatePlacementAndVisibility);
 		}
 	};
 
-	private PaintListener widgetMovementListener = __ -> asyncUpdatePlacementAndVisibility();
+	private PaintListener widgetMovementListener = __ -> asyncExecIfOpen(
+			FindReplaceOverlay.this::updatePlacementAndVisibility);
 
-	private void asyncUpdatePlacementAndVisibility() {
+	private void asyncExecIfOpen(Runnable operation) {
 		Shell shell = getShell();
 		if (shell != null) {
-			shell.getDisplay().asyncExec(this::updatePlacementAndVisibility);
+			shell.getDisplay().asyncExec(() -> {
+				if (getShell() != null) {
+					operation.run();
+				}
+			});
 		}
 	}
 
@@ -228,17 +233,18 @@ public void shellDeactivated(ShellEvent e) {
 	private static class TargetPartVisibilityHandler implements IPartListener2, IPageChangedListener {
 		private final IWorkbenchPart targetPart;
 		private final IWorkbenchPart topLevelPart;
-		private final Supplier<Shell> shellProvider;
+		private final Consumer<Runnable> asyncExecIfOpen;
 		private final Runnable closeCallback;
 		private final Runnable placementUpdateCallback;
 
 		private boolean isTopLevelVisible = true;
 		private boolean isNestedLevelVisible = true;
 
-		TargetPartVisibilityHandler(IWorkbenchPart targetPart, Supplier<Shell> shellProvider, Runnable closeCallback,
+		TargetPartVisibilityHandler(IWorkbenchPart targetPart, Consumer<Runnable> asyncExecIfOpen,
+				Runnable closeCallback,
 				Runnable placementUpdateCallback) {
 			this.targetPart = targetPart;
-			this.shellProvider = shellProvider;
+			this.asyncExecIfOpen = asyncExecIfOpen;
 			this.closeCallback = closeCallback;
 			this.placementUpdateCallback = placementUpdateCallback;
 			if (targetPart != null && targetPart.getSite() instanceof MultiPageEditorSite multiEditorSite) {
@@ -252,23 +258,23 @@ private static class TargetPartVisibilityHandler implements IPartListener2, IPag
 		public void partBroughtToTop(IWorkbenchPartReference partRef) {
 			if (partRef.getPart(false) == topLevelPart && !isTopLevelVisible) {
 				this.isTopLevelVisible = true;
-				shellProvider.get().getDisplay().asyncExec(this::adaptToPartActivationChange);
+				asyncExecIfOpen.accept(this::adaptToPartActivationChange);
 			}
 		}
 
 		@Override
 		public void partVisible(IWorkbenchPartReference partRef) {
 			if (partRef.getPart(false) == topLevelPart && !isTopLevelVisible) {
 				this.isTopLevelVisible = true;
-				shellProvider.get().getDisplay().asyncExec(this::adaptToPartActivationChange);
+				asyncExecIfOpen.accept(this::adaptToPartActivationChange);
 			}
 		}
 
 		@Override
 		public void partHidden(IWorkbenchPartReference partRef) {
 			if (partRef.getPart(false) == topLevelPart && isTopLevelVisible) {
 				this.isTopLevelVisible = false;
-				shellProvider.get().getDisplay().asyncExec(this::adaptToPartActivationChange);
+				asyncExecIfOpen.accept(this::adaptToPartActivationChange);
 			}
 		}
 
@@ -285,26 +291,26 @@ public void pageChanged(PageChangedEvent event) {
 				boolean isPageVisible = event.getSelectedPage() == targetPart;
 				if (isNestedLevelVisible != isPageVisible) {
 					this.isNestedLevelVisible = isPageVisible;
-					shellProvider.get().getDisplay().asyncExec(this::adaptToPartActivationChange);
+					asyncExecIfOpen.accept(this::adaptToPartActivationChange);
 				}
 			}
 		}
 
 		private void adaptToPartActivationChange() {
-			if (shellProvider.get() == null || targetPart.getSite().getPart() == null) {
+			if (targetPart.getSite().getPart() == null) {
 				return;
 			}
 			placementUpdateCallback.run();
 
 			if (!isTargetVisible()) {
 				targetPart.getSite().getShell().setActive();
 				targetPart.setFocus();
-				shellProvider.get().getDisplay().asyncExec(this::focusTargetWidget);
+				asyncExecIfOpen.accept(this::focusTargetWidget);
 			}
 		}
 
 		private void focusTargetWidget() {
-			if (shellProvider.get() == null || targetPart.getSite().getPart() == null) {
+			if (targetPart.getSite().getPart() == null) {
 				return;
 			}
 			if (targetPart instanceof StatusTextEditor textEditor) {
@@ -845,7 +851,7 @@ private void updatePlacementAndVisibility() {
 			return;
 		}
 		if (isInvalidTargetShell()) {
-			getShell().getDisplay().asyncExec(() -> {
+			asyncExecIfOpen(() -> {
 				if (isInvalidTargetShell()) {
 					close();
 					setParentShell(targetPart.getSite().getShell());
@@ -883,7 +889,7 @@ private boolean isInvalidTargetShell() {
 		if (isInvalidTargetPart()) {
 			return false;
 		}
-		return getShell() == null || !targetPart.getSite().getShell().equals(getShell().getParent());
+		return !targetPart.getSite().getShell().equals(getShell().getParent());
 	}
 
 	private Rectangle calculateAbsoluteControlBounds(Control control) {