From 0b7f4cf1e286cb519f565764a5dce52aa7915c60 Mon Sep 17 00:00:00 2001 From: Sunny Goyal Date: Thu, 17 Oct 2024 09:44:25 -0700 Subject: [PATCH] Fixing deadlock in dagger singletons Bug: 373557167 Flag: EXEMPT dagger Test: atest DaggerSingletonDeadlockTest Change-Id: I2304237bfd818c99b82bbfceea8a81ddb136b5a1 --- .../launcher3/model/WellbeingModel.java | 3 +- .../util/AsyncClockEventDelegate.java | 15 ++-- .../util/DaggerSingletonTracker.java | 19 ++++- .../android/launcher3/util/ExecutorUtil.java | 37 --------- .../launcher3/util/ScreenOnTracker.java | 2 +- .../android/launcher3/util/SettingsCache.java | 2 +- .../widget/custom/CustomWidgetManager.java | 36 ++++----- tests/Android.bp | 8 +- .../util/DaggerSingletonDeadlockTest.kt | 77 +++++++++++++++++++ 9 files changed, 124 insertions(+), 75 deletions(-) delete mode 100644 src/com/android/launcher3/util/ExecutorUtil.java create mode 100644 tests/multivalentTests/src/com/android/launcher3/util/DaggerSingletonDeadlockTest.kt diff --git a/quickstep/src/com/android/launcher3/model/WellbeingModel.java b/quickstep/src/com/android/launcher3/model/WellbeingModel.java index 94a18142e5..0f3aaa6fa7 100644 --- a/quickstep/src/com/android/launcher3/model/WellbeingModel.java +++ b/quickstep/src/com/android/launcher3/model/WellbeingModel.java @@ -51,7 +51,6 @@ import com.android.launcher3.popup.RemoteActionShortcut; import com.android.launcher3.popup.SystemShortcut; import com.android.launcher3.util.DaggerSingletonObject; import com.android.launcher3.util.DaggerSingletonTracker; -import com.android.launcher3.util.ExecutorUtil; import com.android.launcher3.util.Executors; import com.android.launcher3.util.Preconditions; import com.android.launcher3.util.SafeCloseable; @@ -122,7 +121,7 @@ public final class WellbeingModel implements SafeCloseable { } }; mWorkerHandler.post(this::initializeInBackground); - ExecutorUtil.executeSyncOnMainOrFail(() -> tracker.addCloseable(this)); + tracker.addCloseable(this); } @WorkerThread diff --git a/quickstep/src/com/android/quickstep/util/AsyncClockEventDelegate.java b/quickstep/src/com/android/quickstep/util/AsyncClockEventDelegate.java index 4a84b1b630..54f6443fa6 100644 --- a/quickstep/src/com/android/quickstep/util/AsyncClockEventDelegate.java +++ b/quickstep/src/com/android/quickstep/util/AsyncClockEventDelegate.java @@ -34,11 +34,8 @@ import androidx.annotation.WorkerThread; import com.android.launcher3.dagger.ApplicationContext; import com.android.launcher3.dagger.LauncherAppSingleton; -import com.android.launcher3.dagger.LauncherBaseAppComponent; import com.android.launcher3.util.DaggerSingletonObject; import com.android.launcher3.util.DaggerSingletonTracker; -import com.android.launcher3.util.ExecutorUtil; -import com.android.launcher3.util.MainThreadInitializedObject; import com.android.launcher3.util.SafeCloseable; import com.android.launcher3.util.SettingsCache; import com.android.launcher3.util.SettingsCache.OnChangeListener; @@ -61,6 +58,7 @@ public class AsyncClockEventDelegate extends ClockEventDelegate new DaggerSingletonObject<>(QuickstepBaseAppComponent::getAsyncClockEventDelegate); private final Context mContext; + private final SettingsCache mSettingsCache; private final SimpleBroadcastReceiver mReceiver = new SimpleBroadcastReceiver(UI_HELPER_EXECUTOR, this::onClockEventReceived); @@ -72,11 +70,14 @@ public class AsyncClockEventDelegate extends ClockEventDelegate private boolean mDestroyed = false; @Inject - AsyncClockEventDelegate(@ApplicationContext Context context, DaggerSingletonTracker tracker) { + AsyncClockEventDelegate(@ApplicationContext Context context, + DaggerSingletonTracker tracker, + SettingsCache settingsCache) { super(context); mContext = context; + mSettingsCache = settingsCache; mReceiver.register(mContext, ACTION_TIME_CHANGED, ACTION_TIMEZONE_CHANGED); - ExecutorUtil.executeSyncOnMainOrFail(() -> tracker.addCloseable(this)); + tracker.addCloseable(this); } @Override @@ -100,7 +101,7 @@ public class AsyncClockEventDelegate extends ClockEventDelegate } synchronized (mFormatObservers) { if (!mFormatRegistered && !mDestroyed) { - SettingsCache.INSTANCE.get(mContext).register(mFormatUri, this); + mSettingsCache.register(mFormatUri, this); mFormatRegistered = true; } mFormatObservers.add(observer); @@ -136,7 +137,7 @@ public class AsyncClockEventDelegate extends ClockEventDelegate @Override public void close() { mDestroyed = true; - SettingsCache.INSTANCE.get(mContext).unregister(mFormatUri, this); + mSettingsCache.unregister(mFormatUri, this); mReceiver.unregisterReceiverSafely(mContext); } } diff --git a/src/com/android/launcher3/util/DaggerSingletonTracker.java b/src/com/android/launcher3/util/DaggerSingletonTracker.java index 2946da1d0c..b7a88dbf44 100644 --- a/src/com/android/launcher3/util/DaggerSingletonTracker.java +++ b/src/com/android/launcher3/util/DaggerSingletonTracker.java @@ -16,6 +16,8 @@ package com.android.launcher3.util; +import static com.android.launcher3.util.Executors.MAIN_EXECUTOR; + import com.android.launcher3.dagger.LauncherAppSingleton; import java.util.ArrayList; @@ -31,7 +33,9 @@ import javax.inject.Inject; @LauncherAppSingleton public class DaggerSingletonTracker implements SafeCloseable { - private final ArrayList mLauncherAppSingletons = new ArrayList<>(); + private final ArrayList mCloseables = new ArrayList<>(); + + private boolean mClosed = false; @Inject DaggerSingletonTracker() { @@ -44,14 +48,21 @@ public class DaggerSingletonTracker implements SafeCloseable { * {@link MainThreadInitializedObject.SandboxContext#onDestroy()} */ public void addCloseable(SafeCloseable closeable) { - mLauncherAppSingletons.add(closeable); + MAIN_EXECUTOR.execute(() -> { + if (mClosed) { + closeable.close(); + } else { + mCloseables.add(closeable); + } + }); } @Override public void close() { + mClosed = true; // Destroy in reverse order - for (int i = mLauncherAppSingletons.size() - 1; i >= 0; i--) { - mLauncherAppSingletons.get(i).close(); + for (int i = mCloseables.size() - 1; i >= 0; i--) { + mCloseables.get(i).close(); } } } diff --git a/src/com/android/launcher3/util/ExecutorUtil.java b/src/com/android/launcher3/util/ExecutorUtil.java deleted file mode 100644 index efc0eec194..0000000000 --- a/src/com/android/launcher3/util/ExecutorUtil.java +++ /dev/null @@ -1,37 +0,0 @@ -/* - * Copyright (C) 2024 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.android.launcher3.util; - -import static com.android.launcher3.util.Executors.MAIN_EXECUTOR; - -import android.os.Looper; - -import java.util.concurrent.ExecutionException; - -public final class ExecutorUtil { - - /** - * Executes runnable on {@link Looper#getMainLooper()}, otherwise fails with an exception. - */ - public static void executeSyncOnMainOrFail(Runnable runnable) { - try { - MAIN_EXECUTOR.submit(runnable).get(); - } catch (InterruptedException | ExecutionException e) { - throw new RuntimeException(e); - } - } -} diff --git a/src/com/android/launcher3/util/ScreenOnTracker.java b/src/com/android/launcher3/util/ScreenOnTracker.java index 3582ad8277..50be98b45c 100644 --- a/src/com/android/launcher3/util/ScreenOnTracker.java +++ b/src/com/android/launcher3/util/ScreenOnTracker.java @@ -68,7 +68,7 @@ public class ScreenOnTracker implements SafeCloseable { private void init(DaggerSingletonTracker tracker) { mIsScreenOn = true; mReceiver.register(mContext, ACTION_SCREEN_ON, ACTION_SCREEN_OFF, ACTION_USER_PRESENT); - ExecutorUtil.executeSyncOnMainOrFail(() -> tracker.addCloseable(this)); + tracker.addCloseable(this); } @Override diff --git a/src/com/android/launcher3/util/SettingsCache.java b/src/com/android/launcher3/util/SettingsCache.java index a1ed49990b..29d5032cea 100644 --- a/src/com/android/launcher3/util/SettingsCache.java +++ b/src/com/android/launcher3/util/SettingsCache.java @@ -94,7 +94,7 @@ public class SettingsCache extends ContentObserver implements SafeCloseable { SettingsCache(@ApplicationContext Context context, DaggerSingletonTracker tracker) { super(new Handler(Looper.getMainLooper())); mResolver = context.getContentResolver(); - ExecutorUtil.executeSyncOnMainOrFail(() -> tracker.addCloseable(this)); + tracker.addCloseable(this); } @Override diff --git a/src/com/android/launcher3/widget/custom/CustomWidgetManager.java b/src/com/android/launcher3/widget/custom/CustomWidgetManager.java index 4aeac7639a..9dddc18ecc 100644 --- a/src/com/android/launcher3/widget/custom/CustomWidgetManager.java +++ b/src/com/android/launcher3/widget/custom/CustomWidgetManager.java @@ -18,6 +18,7 @@ package com.android.launcher3.widget.custom; import static com.android.launcher3.Flags.enableSmartspaceAsAWidget; import static com.android.launcher3.model.data.LauncherAppWidgetInfo.CUSTOM_WIDGET_ID; +import static com.android.launcher3.util.Executors.MAIN_EXECUTOR; import static com.android.launcher3.widget.LauncherAppWidgetProviderInfo.CLS_CUSTOM_WIDGET_PREFIX; import android.appwidget.AppWidgetManager; @@ -38,7 +39,6 @@ import com.android.launcher3.dagger.LauncherAppSingleton; import com.android.launcher3.dagger.LauncherBaseAppComponent; import com.android.launcher3.util.DaggerSingletonObject; 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.widget.LauncherAppWidgetHostView; @@ -90,27 +90,23 @@ public class CustomWidgetManager implements PluginListener { mCustomWidgets = new ArrayList<>(); pluginManager.addPluginListener(this, CustomWidgetPlugin.class, true); - - ExecutorUtil.executeSyncOnMainOrFail(() -> { - if (enableSmartspaceAsAWidget()) { - for (String s: context.getResources() - .getStringArray(R.array.custom_widget_providers)) { - try { - Class cls = Class.forName(s); - CustomWidgetPlugin plugin = (CustomWidgetPlugin) - cls.getDeclaredConstructor(Context.class).newInstance(context); - onPluginConnected(plugin, context); - } catch (ClassNotFoundException | InstantiationException - | IllegalAccessException - | ClassCastException | NoSuchMethodException - | InvocationTargetException e) { - Log.e(TAG, "Exception found when trying to add custom widgets: " + e); - } + if (enableSmartspaceAsAWidget()) { + for (String s: context.getResources() + .getStringArray(R.array.custom_widget_providers)) { + try { + Class cls = Class.forName(s); + CustomWidgetPlugin plugin = (CustomWidgetPlugin) + cls.getDeclaredConstructor(Context.class).newInstance(context); + MAIN_EXECUTOR.execute(() -> onPluginConnected(plugin, context)); + } catch (ClassNotFoundException | InstantiationException + | IllegalAccessException + | ClassCastException | NoSuchMethodException + | InvocationTargetException e) { + Log.e(TAG, "Exception found when trying to add custom widgets: " + e); } } - - tracker.addCloseable(() -> pluginManager.removePluginListener(this)); - }); + } + tracker.addCloseable(() -> pluginManager.removePluginListener(this)); } @Override diff --git a/tests/Android.bp b/tests/Android.bp index 9f62d02b64..96672778fa 100644 --- a/tests/Android.bp +++ b/tests/Android.bp @@ -98,6 +98,8 @@ android_library { "com_android_launcher3_flags_lib", "com_android_wm_shell_flags_lib", "android.appwidget.flags-aconfig-java", + "platform-parametric-runner-lib", + "kotlin-reflect", ], manifest: "AndroidManifest-common.xml", platform_apis: true, @@ -111,6 +113,9 @@ android_library { asset_dirs: ["assets"], // TODO(b/319712088): re-enable use_resource_processor use_resource_processor: false, + static_libs: [ + "kotlin-reflect", + ], } android_test { @@ -193,10 +198,7 @@ android_robolectric_test { name: "Launcher3RoboTests", srcs: [ ":launcher3-robo-src", - - // Test util classes ":launcher-testing-helpers-robo", - ":launcher-testing-shared", ], exclude_srcs: [ //"src/com/android/launcher3/util/CellContentDimensionsTest.kt", // Failing - b/316553889 diff --git a/tests/multivalentTests/src/com/android/launcher3/util/DaggerSingletonDeadlockTest.kt b/tests/multivalentTests/src/com/android/launcher3/util/DaggerSingletonDeadlockTest.kt new file mode 100644 index 0000000000..642c6283be --- /dev/null +++ b/tests/multivalentTests/src/com/android/launcher3/util/DaggerSingletonDeadlockTest.kt @@ -0,0 +1,77 @@ +/* + * Copyright (C) 2024 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.launcher3.util + +import androidx.test.filters.SmallTest +import com.android.launcher3.dagger.LauncherAppComponent +import com.android.launcher3.util.LauncherModelHelper.SandboxModelContext +import java.util.concurrent.TimeUnit.SECONDS +import kotlin.reflect.KFunction +import kotlin.reflect.full.memberFunctions +import org.junit.After +import org.junit.Assert +import org.junit.Test +import org.junit.runner.RunWith +import platform.test.runner.parameterized.ParameterizedAndroidJunit4 +import platform.test.runner.parameterized.Parameters + +@SmallTest +@RunWith(ParameterizedAndroidJunit4::class) +class DaggerSingletonDeadlockTest(val method: KFunction<*>, val methodName: String) { + + private val context = SandboxModelContext() + + @After + fun tearDown() { + context.onDestroy() + } + + /** Test to verify that the object can be created successfully on the main thread. */ + @Test + fun objectCreationOnMainThread() { + Executors.MAIN_EXECUTOR.submit { + method.call(context.appComponent).also(Assert::assertNotNull) + } + .get(10, SECONDS) + } + + /** + * Test to verify that the object can be created successfully on the background thread, when the + * main thread is blocked. + */ + @Test + fun objectCreationOnBackgroundThread() { + TestUtil.runOnExecutorSync(Executors.MAIN_EXECUTOR) { + Executors.THREAD_POOL_EXECUTOR.submit { + method.call(context.appComponent).also(Assert::assertNotNull) + } + .get(10, SECONDS) + } + } + + companion object { + @Parameters(name = "{1}") + @JvmStatic + fun getTestMethods() = + LauncherAppComponent::class + .memberFunctions + .filter { it.parameters.size == 1 } + .map { + arrayOf(it, if (it.name.startsWith("get")) it.name.substring(3) else it.name) + } + } +}