From d59298a246292253b97987dd6b0cb4a34993628a Mon Sep 17 00:00:00 2001 From: Lars Vogel Date: Wed, 4 Feb 2026 10:14:43 +0100 Subject: [PATCH 1/2] Fix race condition in CleanupAddon.subscribeRenderingChanged The test ensureCleanUpAddonCleansUp was failing intermittently because CleanupAddon deferred container cleanup via Display.asyncExec(), creating a race condition where event loop pumping during part activation could interfere with async cleanup tasks. Root cause analysis: - When hidePart() is called on parts with REMOVE_ON_HIDE_TAG, it triggers synchronous EMF model change events via UIEventPublisher - Event delivery is fully synchronous: EventBroker.send() -> EventAdmin.sendEvent() -> Display.syncExec() -> handler execution - The subscribeRenderingChanged handler detected visCount==0 but deferred cleanup via asyncExec (line 352) - Meanwhile, activate() calls during hidePart() pumped the event loop (via focusGui/SWT focus handling), potentially consuming queued asyncExecs before they could observe the final state - This created timing-dependent behavior where cleanup could be skipped Fix: - Remove asyncExec wrapper from subscribeRenderingChanged's visCount==0 path - Since event delivery is synchronous, visCount already reflects the actual state at event time - no need to defer and re-check - Container is now hidden immediately when last renderable child becomes non-renderable, before any event loop pumping can interfere - Update test to use simple spinEventLoop + assertFalse instead of 30-second waitForCondition, as cleanup is now synchronous This fixes the flaky test that failed ~1-3% of the time even with the 30-second timeout. The subscribeTopicChildren handler still uses asyncExec for children removal/sash unwrapping, but toBeRendered cleanup is now deterministic. Fixes https://github.com/eclipse-platform/eclipse.platform.ui/issues/3581 Co-Authored-By: Claude Sonnet 4.5 --- .../addons/cleanupaddon/CleanupAddon.java | 17 +++++++---------- .../workbench/PartRenderingEngineTests.java | 10 +++------- 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/bundles/org.eclipse.e4.ui.workbench.addons.swt/src/org/eclipse/e4/ui/workbench/addons/cleanupaddon/CleanupAddon.java b/bundles/org.eclipse.e4.ui.workbench.addons.swt/src/org/eclipse/e4/ui/workbench/addons/cleanupaddon/CleanupAddon.java index c2fab02e0657..beb4e0f9dfee 100644 --- a/bundles/org.eclipse.e4.ui.workbench.addons.swt/src/org/eclipse/e4/ui/workbench/addons/cleanupaddon/CleanupAddon.java +++ b/bundles/org.eclipse.e4.ui.workbench.addons.swt/src/org/eclipse/e4/ui/workbench/addons/cleanupaddon/CleanupAddon.java @@ -346,16 +346,13 @@ private void subscribeRenderingChanged( int visCount = modelService.countRenderableChildren(container); // Remove stacks with no visible children from the display (but not the - // model) - final MElementContainer theContainer = container; - if (visCount == 0) { - Display.getCurrent().asyncExec(() -> { - int visCount1 = modelService.countRenderableChildren(theContainer); - if (!isLastEditorStack(theContainer) && visCount1 == 0) { - theContainer.setToBeRendered(false); - transferPrimaryDataStackIfRemoved(theContainer); - } - }); + // model). Since event delivery is synchronous (EventBroker.send via + // EventAdmin.sendEvent and Display.syncExec), the visCount reflects + // the actual state at the time of this event and can be acted upon + // immediately without deferring via asyncExec. + if (visCount == 0 && !isLastEditorStack(container)) { + container.setToBeRendered(false); + transferPrimaryDataStackIfRemoved(container); } else if (container.getParent() != null) { // omit detached windows // if there are rendered elements but none are 'visible' we should // make the container invisible as well diff --git a/tests/org.eclipse.e4.ui.tests/src/org/eclipse/e4/ui/tests/workbench/PartRenderingEngineTests.java b/tests/org.eclipse.e4.ui.tests/src/org/eclipse/e4/ui/tests/workbench/PartRenderingEngineTests.java index b62b0d41d6ac..d646a2150eef 100644 --- a/tests/org.eclipse.e4.ui.tests/src/org/eclipse/e4/ui/tests/workbench/PartRenderingEngineTests.java +++ b/tests/org.eclipse.e4.ui.tests/src/org/eclipse/e4/ui/tests/workbench/PartRenderingEngineTests.java @@ -64,7 +64,6 @@ import org.eclipse.swt.widgets.Shell; import org.eclipse.swt.widgets.ToolBar; import org.eclipse.swt.widgets.Widget; -import org.eclipse.ui.tests.harness.util.DisplayHelper; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Disabled; @@ -2447,12 +2446,9 @@ public void ensureCleanUpAddonCleansUp() { assertTrue(partStackForPartBPartC.isToBeRendered(), " PartStack with children should be rendered"); partService.hidePart(partB); partService.hidePart(partC); - // DisplayHelper.waitForCondition() handles event processing via Display.sleep() - // and retries. Calling spinEventLoop() here creates a race condition where - // events may be processed before CleanupAddon's asyncExec() is queued (line 352). - assertTrue( - DisplayHelper.waitForCondition(Display.getDefault(), 30_000, - () -> !partStackForPartBPartC.isToBeRendered()), "CleanupAddon should ensure that partStack is not rendered anymore, as all childs have been removed"); + contextRule.spinEventLoop(); + assertFalse(partStackForPartBPartC.isToBeRendered(), + "CleanupAddon should ensure that partStack is not rendered anymore, as all childs have been removed"); // PartStack with IPresentationEngine.NO_AUTO_COLLAPSE should not be removed // even if children are removed partService.hidePart(editor, true); From 7deb295c5f7c6409a08c66efd1688563f641e9cf Mon Sep 17 00:00:00 2001 From: Eclipse Platform Bot Date: Sun, 1 Mar 2026 11:41:42 +0000 Subject: [PATCH 2/2] Version bump(s) for 4.40 stream --- .../org.eclipse.e4.ui.workbench.addons.swt/META-INF/MANIFEST.MF | 2 +- tests/org.eclipse.e4.ui.tests/META-INF/MANIFEST.MF | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/bundles/org.eclipse.e4.ui.workbench.addons.swt/META-INF/MANIFEST.MF b/bundles/org.eclipse.e4.ui.workbench.addons.swt/META-INF/MANIFEST.MF index b671e2de079a..b01ce425043e 100644 --- a/bundles/org.eclipse.e4.ui.workbench.addons.swt/META-INF/MANIFEST.MF +++ b/bundles/org.eclipse.e4.ui.workbench.addons.swt/META-INF/MANIFEST.MF @@ -1,7 +1,7 @@ Manifest-Version: 1.0 Bundle-ManifestVersion: 2 Bundle-SymbolicName: org.eclipse.e4.ui.workbench.addons.swt;singleton:=true -Bundle-Version: 1.6.0.qualifier +Bundle-Version: 1.6.100.qualifier Bundle-Name: %pluginName Bundle-Vendor: %providerName Bundle-Localization: plugin diff --git a/tests/org.eclipse.e4.ui.tests/META-INF/MANIFEST.MF b/tests/org.eclipse.e4.ui.tests/META-INF/MANIFEST.MF index ca9302d7765b..025940a11ee1 100644 --- a/tests/org.eclipse.e4.ui.tests/META-INF/MANIFEST.MF +++ b/tests/org.eclipse.e4.ui.tests/META-INF/MANIFEST.MF @@ -2,7 +2,7 @@ Manifest-Version: 1.0 Bundle-ManifestVersion: 2 Bundle-Name: %pluginName Bundle-SymbolicName: org.eclipse.e4.ui.tests;singleton:=true -Bundle-Version: 0.16.0.qualifier +Bundle-Version: 0.16.100.qualifier Bundle-Vendor: %providerName Bundle-Localization: plugin Require-Bundle: org.eclipse.emf.ecore.xmi;bundle-version="2.4.0",