From 9cd3154952121389f85bc7e421ca031aef264665 Mon Sep 17 00:00:00 2001 From: Sunny Goyal Date: Thu, 17 Oct 2024 11:47:07 -0700 Subject: [PATCH] Moving PluginManager to dagger Multiple singletons depend on Plugin which can cause deadlock if PluginManager is initialized on main thread Test: presubmit Bug: 361850561 Bug: 373557167 Flag: EXEMPT dagger Change-Id: I79f17ac6b78a2ce60df2d27a6e794b9e4eba1b51 --- quickstep/res/values/config.xml | 1 - .../plugins/PluginManagerWrapperImpl.java | 16 ++++++++----- .../quickstep/dagger/QuickStepModule.java | 8 ++++++- res/values/config.xml | 1 - .../dagger/LauncherBaseAppComponent.java | 2 ++ .../launcher3/util/PluginManagerWrapper.java | 23 ++++++++++++------- .../widget/custom/CustomWidgetManager.java | 20 ++++++---------- .../widget/custom/CustomWidgetManagerTest.kt | 17 +++++++------- 8 files changed, 49 insertions(+), 39 deletions(-) diff --git a/quickstep/res/values/config.xml b/quickstep/res/values/config.xml index 41b2384a35..db5ff199ab 100644 --- a/quickstep/res/values/config.xml +++ b/quickstep/res/values/config.xml @@ -33,7 +33,6 @@ com.android.launcher3.taskbar.TaskbarModelCallbacksFactory com.android.launcher3.taskbar.TaskbarViewCallbacksFactory com.android.quickstep.LauncherRestoreEventLoggerImpl - com.android.launcher3.uioverrides.plugins.PluginManagerWrapperImpl com.android.launcher3.taskbar.TaskbarEduTooltipController com.android.quickstep.contextualeducation.SystemContextualEduStatsManager diff --git a/quickstep/src/com/android/launcher3/uioverrides/plugins/PluginManagerWrapperImpl.java b/quickstep/src/com/android/launcher3/uioverrides/plugins/PluginManagerWrapperImpl.java index 74572c4136..3aa196365b 100644 --- a/quickstep/src/com/android/launcher3/uioverrides/plugins/PluginManagerWrapperImpl.java +++ b/quickstep/src/com/android/launcher3/uioverrides/plugins/PluginManagerWrapperImpl.java @@ -27,6 +27,8 @@ import android.content.Intent; import android.content.pm.ResolveInfo; import com.android.launcher3.BuildConfig; +import com.android.launcher3.dagger.ApplicationContext; +import com.android.launcher3.dagger.LauncherAppSingleton; import com.android.launcher3.util.PluginManagerWrapper; import com.android.systemui.plugins.Plugin; import com.android.systemui.plugins.PluginListener; @@ -34,7 +36,6 @@ import com.android.systemui.shared.plugins.PluginActionManager; import com.android.systemui.shared.plugins.PluginInstance; import com.android.systemui.shared.plugins.PluginManagerImpl; import com.android.systemui.shared.plugins.PluginPrefs; -import com.android.systemui.shared.system.UncaughtExceptionPreHandlerManager; import java.io.PrintWriter; import java.util.ArrayList; @@ -42,16 +43,17 @@ import java.util.Collections; import java.util.List; import java.util.Set; -public class PluginManagerWrapperImpl extends PluginManagerWrapper { +import javax.inject.Inject; - private static final UncaughtExceptionPreHandlerManager UNCAUGHT_EXCEPTION_PRE_HANDLER_MANAGER = - new UncaughtExceptionPreHandlerManager(); +@LauncherAppSingleton +public class PluginManagerWrapperImpl extends PluginManagerWrapper { private final Context mContext; private final PluginManagerImpl mPluginManager; private final PluginEnablerImpl mPluginEnabler; - public PluginManagerWrapperImpl(Context c) { + @Inject + public PluginManagerWrapperImpl(@ApplicationContext Context c) { mContext = c; mPluginEnabler = new PluginEnablerImpl(c); List privilegedPlugins = Collections.emptyList(); @@ -64,9 +66,11 @@ public class PluginManagerWrapperImpl extends PluginManagerWrapper { c.getSystemService(NotificationManager.class), mPluginEnabler, privilegedPlugins, instanceFactory); + // Use null preHandlerManager, as the handler is never unregistered which can cause leaks + // when using multiple dagger graphs. mPluginManager = new PluginManagerImpl(c, instanceManagerFactory, BuildConfig.IS_DEBUG_DEVICE, - UNCAUGHT_EXCEPTION_PRE_HANDLER_MANAGER, mPluginEnabler, + null /* preHandlerManager */, mPluginEnabler, new PluginPrefs(c), privilegedPlugins); } diff --git a/quickstep/src/com/android/quickstep/dagger/QuickStepModule.java b/quickstep/src/com/android/quickstep/dagger/QuickStepModule.java index 08345b85f6..ab77a7f62c 100644 --- a/quickstep/src/com/android/quickstep/dagger/QuickStepModule.java +++ b/quickstep/src/com/android/quickstep/dagger/QuickStepModule.java @@ -15,8 +15,14 @@ */ package com.android.quickstep.dagger; +import com.android.launcher3.uioverrides.plugins.PluginManagerWrapperImpl; +import com.android.launcher3.util.PluginManagerWrapper; + +import dagger.Binds; import dagger.Module; @Module -public class QuickStepModule { +public abstract class QuickStepModule { + + @Binds abstract PluginManagerWrapper bindPluginManagerWrapper(PluginManagerWrapperImpl impl); } diff --git a/res/values/config.xml b/res/values/config.xml index 701e64a4b4..504218b78e 100644 --- a/res/values/config.xml +++ b/res/values/config.xml @@ -85,7 +85,6 @@ - diff --git a/src/com/android/launcher3/dagger/LauncherBaseAppComponent.java b/src/com/android/launcher3/dagger/LauncherBaseAppComponent.java index c3508b77db..4da7c2716a 100644 --- a/src/com/android/launcher3/dagger/LauncherBaseAppComponent.java +++ b/src/com/android/launcher3/dagger/LauncherBaseAppComponent.java @@ -20,6 +20,7 @@ import android.content.Context; import com.android.launcher3.pm.InstallSessionHelper; import com.android.launcher3.util.DaggerSingletonTracker; +import com.android.launcher3.util.PluginManagerWrapper; import com.android.launcher3.util.ScreenOnTracker; import com.android.launcher3.util.SettingsCache; import com.android.launcher3.widget.custom.CustomWidgetManager; @@ -40,6 +41,7 @@ public interface LauncherBaseAppComponent { ScreenOnTracker getScreenOnTracker(); SettingsCache getSettingsCache(); CustomWidgetManager getCustomWidgetManager(); + PluginManagerWrapper getPluginManagerWrapper(); /** Builder for LauncherBaseAppComponent. */ interface Builder { diff --git a/src/com/android/launcher3/util/PluginManagerWrapper.java b/src/com/android/launcher3/util/PluginManagerWrapper.java index b27aa120b9..5b28570acb 100644 --- a/src/com/android/launcher3/util/PluginManagerWrapper.java +++ b/src/com/android/launcher3/util/PluginManagerWrapper.java @@ -15,32 +15,39 @@ */ package com.android.launcher3.util; -import static com.android.launcher3.util.MainThreadInitializedObject.forOverride; +import androidx.annotation.AnyThread; -import com.android.launcher3.R; +import com.android.launcher3.dagger.LauncherAppSingleton; +import com.android.launcher3.dagger.LauncherBaseAppComponent; import com.android.systemui.plugins.Plugin; import com.android.systemui.plugins.PluginListener; import java.io.PrintWriter; -public class PluginManagerWrapper implements ResourceBasedOverride, SafeCloseable { +import javax.inject.Inject; - public static final MainThreadInitializedObject INSTANCE = - forOverride(PluginManagerWrapper.class, R.string.plugin_manager_wrapper_class); +@LauncherAppSingleton +public class PluginManagerWrapper{ + public static final DaggerSingletonObject INSTANCE = + new DaggerSingletonObject<>(LauncherBaseAppComponent::getPluginManagerWrapper); + + @Inject + public PluginManagerWrapper() { } + + @AnyThread public void addPluginListener( PluginListener listener, Class pluginClass) { addPluginListener(listener, pluginClass, false); } + @AnyThread public void addPluginListener( PluginListener listener, Class pluginClass, boolean allowMultiple) { } + @AnyThread public void removePluginListener(PluginListener listener) { } - @Override - public void close() { } - public void dump(PrintWriter pw) { } } diff --git a/src/com/android/launcher3/widget/custom/CustomWidgetManager.java b/src/com/android/launcher3/widget/custom/CustomWidgetManager.java index 0778172127..4aeac7639a 100644 --- a/src/com/android/launcher3/widget/custom/CustomWidgetManager.java +++ b/src/com/android/launcher3/widget/custom/CustomWidgetManager.java @@ -41,7 +41,6 @@ import com.android.launcher3.util.DaggerSingletonTracker; import com.android.launcher3.util.ExecutorUtil; import com.android.launcher3.util.PackageUserKey; import com.android.launcher3.util.PluginManagerWrapper; -import com.android.launcher3.util.SafeCloseable; import com.android.launcher3.widget.LauncherAppWidgetHostView; import com.android.launcher3.widget.LauncherAppWidgetProviderInfo; import com.android.systemui.plugins.CustomWidgetPlugin; @@ -61,7 +60,7 @@ import javax.inject.Inject; * CustomWidgetManager handles custom widgets implemented as a plugin. */ @LauncherAppSingleton -public class CustomWidgetManager implements PluginListener, SafeCloseable { +public class CustomWidgetManager implements PluginListener { public static final DaggerSingletonObject INSTANCE = new DaggerSingletonObject<>(LauncherBaseAppComponent::getCustomWidgetManager); @@ -75,12 +74,14 @@ public class CustomWidgetManager implements PluginListener, private final @NonNull AppWidgetManager mAppWidgetManager; @Inject - CustomWidgetManager(@ApplicationContext Context context, DaggerSingletonTracker tracker) { - this(context, AppWidgetManager.getInstance(context), tracker); + CustomWidgetManager(@ApplicationContext Context context, PluginManagerWrapper pluginManager, + DaggerSingletonTracker tracker) { + this(context, pluginManager, AppWidgetManager.getInstance(context), tracker); } @VisibleForTesting CustomWidgetManager(@ApplicationContext Context context, + PluginManagerWrapper pluginManager, @NonNull AppWidgetManager widgetManager, DaggerSingletonTracker tracker) { mContext = context; @@ -88,11 +89,9 @@ public class CustomWidgetManager implements PluginListener, mPlugins = new HashMap<>(); mCustomWidgets = new ArrayList<>(); + pluginManager.addPluginListener(this, CustomWidgetPlugin.class, true); ExecutorUtil.executeSyncOnMainOrFail(() -> { - PluginManagerWrapper.INSTANCE.get(context) - .addPluginListener(this, CustomWidgetPlugin.class, true); - if (enableSmartspaceAsAWidget()) { for (String s: context.getResources() .getStringArray(R.array.custom_widget_providers)) { @@ -110,15 +109,10 @@ public class CustomWidgetManager implements PluginListener, } } - tracker.addCloseable(this); + tracker.addCloseable(() -> pluginManager.removePluginListener(this)); }); } - @Override - public void close() { - PluginManagerWrapper.INSTANCE.get(mContext).removePluginListener(this); - } - @Override public void onPluginConnected(CustomWidgetPlugin plugin, Context context) { List providers = mAppWidgetManager diff --git a/tests/multivalentTests/src/com/android/launcher3/widget/custom/CustomWidgetManagerTest.kt b/tests/multivalentTests/src/com/android/launcher3/widget/custom/CustomWidgetManagerTest.kt index 82f56b8c2e..1c25db92be 100644 --- a/tests/multivalentTests/src/com/android/launcher3/widget/custom/CustomWidgetManagerTest.kt +++ b/tests/multivalentTests/src/com/android/launcher3/widget/custom/CustomWidgetManagerTest.kt @@ -26,17 +26,19 @@ import androidx.test.platform.app.InstrumentationRegistry.getInstrumentation import com.android.launcher3.util.DaggerSingletonTracker import com.android.launcher3.util.LauncherModelHelper.SandboxModelContext import com.android.launcher3.util.PluginManagerWrapper +import com.android.launcher3.util.SafeCloseable import com.android.launcher3.util.WidgetUtils import com.android.launcher3.widget.LauncherAppWidgetHostView import com.android.launcher3.widget.LauncherAppWidgetProviderInfo import com.android.systemui.plugins.CustomWidgetPlugin -import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith +import org.mockito.ArgumentCaptor +import org.mockito.Captor import org.mockito.Mock import org.mockito.Mockito.mock import org.mockito.MockitoAnnotations @@ -60,16 +62,12 @@ class CustomWidgetManagerTest { @Mock private lateinit var mockAppWidgetManager: AppWidgetManager @Mock private lateinit var tracker: DaggerSingletonTracker + @Captor private lateinit var closableCaptor: ArgumentCaptor + @Before fun setUp() { MockitoAnnotations.initMocks(this) - context.putObject(PluginManagerWrapper.INSTANCE, pluginManager) - underTest = CustomWidgetManager(context, mockAppWidgetManager, tracker) - } - - @After - fun tearDown() { - underTest.close() + underTest = CustomWidgetManager(context, pluginManager, mockAppWidgetManager, tracker) } @Test @@ -80,7 +78,8 @@ class CustomWidgetManagerTest { @Test fun close_widget_manager_should_remove_plugin_listener() { - underTest.close() + verify(tracker).addCloseable(closableCaptor.capture()) + closableCaptor.allValues.forEach(SafeCloseable::close) verify(pluginManager).removePluginListener(same(underTest)) }