Dispose activity management during shutdown completely and early #1084

The WorkbenchActivitySupport is currently disabled quite late during
workbench shutdown. In particular, it is performed after the service
locator disposal, which produces several events for the disablement of
expression-controlled activities that are processed by the activity
management. Since the activity management is shut down afterwards
anyway, the processing of these events is unnecessary.

This change ensures that activities enablement changes are not
unnecessarily processed during workbench shutdown. It consists of two
parts:
1. It performs the disposal of activity management and its persistence
handler early in the workbench shutdown process (in particular before
the service locator disposal).
2. It adds proper dispose functionality for the IActivityManager, such
that disposing the workbench activity support disposes the activity
manager, which then removes all its listeners to not react to further
events.

Contributes to
#1084.

bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/activities/IActivityManager.java

@@ -114,4 +114,13 @@ public interface IActivityManager {
 	 *                                performed.
 	 */
 	void removeActivityManagerListener(IActivityManagerListener activityManagerListener);
+
+	/**
+	 * Disposes this activity manager. Removes all listeners. The behavior of all
+	 * other methods after disposing is undefined.
+	 *
+	 * @since 3.131
+	 */
+	void dispose();
+
 }

bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/Workbench.java

@@ -3019,30 +3019,35 @@ private void shutdown() {
 		workbenchListeners.clear();
 
 		cancelEarlyStartup();
-		if (workbenchService != null)
+		if (workbenchService != null) {
 			workbenchService.unregister();
+		}
 		workbenchService = null;
 
-		if (e4WorkbenchService != null)
+		if (e4WorkbenchService != null) {
 			e4WorkbenchService.unregister();
+		}
 		e4WorkbenchService = null;
 
 		// for dynamic UI
 		registry.removeRegistryChangeListener(extensionEventHandler);
 		registry.removeRegistryChangeListener(startupRegistryListener);
 
+		// shut down activity helper before disposing workbench activity support;
+		// dispose activity support before disposing service locator to avoid
+		// unnecessary activity disablement processing
+		activityHelper.shutdown();
+		workbenchActivitySupport.dispose();
+		WorkbenchHelpSystem.disposeIfNecessary();
+
 		// Bring down all of the services.
 		serviceLocator.dispose();
 		application.getCommands().removeAll(commandsToRemove);
 		application.getCategories().removeAll(categoriesToRemove);
 		getDisplay().removeFilter(SWT.MouseDown, backForwardListener);
 		backForwardListener = null;
 
-		workbenchActivitySupport.dispose();
-		WorkbenchHelpSystem.disposeIfNecessary();
-
 		// shutdown the rest of the workbench
-		activityHelper.shutdown();
 		uninitializeImages();
 		if (WorkbenchPlugin.getDefault() != null) {
 			WorkbenchPlugin.getDefault().reset();

bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/activities/AbstractActivityManager.java

@@ -22,6 +22,8 @@
 public abstract class AbstractActivityManager implements IActivityManager {
 	private ListenerList<IActivityManagerListener> activityManagerListeners;
 
+	private boolean disposed;
+
 	protected AbstractActivityManager() {
 	}
 
@@ -60,4 +62,15 @@ public void removeActivityManagerListener(IActivityManagerListener activityManag
 			activityManagerListeners.remove(activityManagerListener);
 		}
 	}
+
+	@Override
+	public void dispose() {
+		activityManagerListeners.clear();
+		disposed = true;
+	}
+
+	protected boolean isDisposed() {
+		return disposed;
+	}
+
 }

bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/activities/MutableActivityManager.java

@@ -133,6 +133,23 @@ public MutableActivityManager(ITriggerPointAdvisor triggerPointAdvisor, IActivit
 		readRegistry(true);
 	}
 
+	@Override
+	public void dispose() {
+		super.dispose();
+		clearExpressions();
+		activityRegistry.removeActivityRegistryListener(activityRegistryListener);
+		activitiesById.clear();
+		activityRequirementBindingsByActivityId.clear();
+		activityPatternBindingsByActivityId.clear();
+		categoriesById.clear();
+		categoryActivityBindingsByCategoryId.clear();
+		categoryDefinitionsById.clear();
+		definedActivityIds.clear();
+		definedCategoryIds.clear();
+		enabledActivityIds.clear();
+		identifiersById.clear();
+	}
+
 	@Override
 	synchronized public IActivity getActivity(String activityId) {
 		if (activityId == null) {
@@ -596,7 +613,7 @@ private Map<String, ActivityEvent> updateActivities(Collection<String> activityI
 	}
 
 	private IPropertyChangeListener enabledWhenListener = event -> {
-		if (addingEvaluationListener) {
+		if (addingEvaluationListener || isDisposed()) {
 			return;
 		}
 

bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/activities/ProxyActivityManager.java

@@ -66,4 +66,5 @@ public Set<String> getEnabledActivityIds() {
 	public IIdentifier getIdentifier(String identifierId) {
 		return activityManager.getIdentifier(identifierId);
 	}
+
 }

bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/activities/ws/WorkbenchActivitySupport.java

@@ -315,11 +315,13 @@ private ImageBindingRegistry getCategoryImageBindingRegistry() {
 	}
 
 	/**
-	 * Dispose of the image registries.
+	 * Dispose of the image registries and activity manager.
 	 *
 	 * @since 3.1
 	 */
 	public void dispose() {
+		proxyActivityManager.dispose();
+		mutableActivityManager.dispose();
 		if (activityImageBindingRegistry != null) {
 			activityImageBindingRegistry.dispose();
 			PlatformUI.getWorkbench().getExtensionTracker().unregisterHandler(activityImageBindingRegistry);

tests/org.eclipse.ui.tests.rcp/Eclipse RCP Tests/org/eclipse/ui/tests/rcp/RcpTestSuite.java

@@ -25,7 +25,7 @@
 @RunWith(Suite.class)
 @Suite.SuiteClasses({ PlatformUITest.class, WorkbenchAdvisorTest.class, WorkbenchConfigurerTest.class,
 		WorkbenchWindowConfigurerTest.class, ActionBarConfigurerTest.class, IWorkbenchPageTest.class,
-		WorkbenchSaveRestoreStateTest.class, WorkbenchListenerTest.class })
+		WorkbenchSaveRestoreStateTest.class, WorkbenchListenerTest.class, WorkbenchTest.class })
 public class RcpTestSuite {
 
 

tests/org.eclipse.ui.tests.rcp/Eclipse RCP Tests/org/eclipse/ui/tests/rcp/WorkbenchTest.java

@@ -0,0 +1,56 @@
+/*******************************************************************************
+ * Copyright (c) 2023 Vector Informatik GmbH and others.
+ *
+ * This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License 2.0
+ * which accompanies this distribution, and is available at
+ * https://www.eclipse.org/legal/epl-2.0/
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ *******************************************************************************/
+package org.eclipse.ui.tests.rcp;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+
+import org.eclipse.swt.widgets.Display;
+import org.eclipse.ui.IWorkbench;
+import org.eclipse.ui.PlatformUI;
+import org.eclipse.ui.activities.IActivityManager;
+import org.eclipse.ui.activities.IActivityManagerListener;
+import org.eclipse.ui.application.WorkbenchAdvisor;
+import org.eclipse.ui.tests.harness.util.RCPTestWorkbenchAdvisor;
+import org.junit.Test;
+
+public class WorkbenchTest {
+	/**
+	 * Tests activity manager behavior during workbench shutdown.
+	 *
+	 * See https://github.com/eclipse-platform/eclipse.platform.ui/issues/1084
+	 */
+	@Test
+	public void testWorkbenchShutdownProducesNoActivityManagerEvents() {
+		IActivityManagerListener activityListener = mock(IActivityManagerListener.class);
+		WorkbenchAdvisor closeAfterStartupAdvisor = new RCPTestWorkbenchAdvisor() {
+			@Override
+			public void postStartup() {
+				IWorkbench workbench = getWorkbenchConfigurer().getWorkbench();
+				IActivityManager activityManager = workbench.getActivitySupport().getActivityManager();
+				activityManager.addActivityManagerListener(activityListener);
+				workbench.close();
+			};
+		};
+
+		runWorkbench(closeAfterStartupAdvisor);
+
+		verify(activityListener, never()).activityManagerChanged(any());
+	}
+
+	private void runWorkbench(WorkbenchAdvisor workbenchAdvisor) {
+		Display display = PlatformUI.createDisplay();
+		PlatformUI.createAndRunWorkbench(display, workbenchAdvisor);
+	}
+
+}

tests/org.eclipse.ui.tests.rcp/META-INF/MANIFEST.MF

@@ -7,6 +7,7 @@ Bundle-Vendor: %providerName
 Require-Bundle: org.eclipse.core.runtime,
  org.eclipse.ui,
  org.junit,
+ org.mockito.mockito-core,
  org.eclipse.test.performance,
  org.eclipse.ui.tests.harness
 Bundle-ActivationPolicy: lazy