Synchronize PluginInstance mutation methods
Flag: None
Bug: 316747123
Test: atest PluginInstanceTest#testLoadUnloadSimultaneous_HoldsUnload
Change-Id: I9ce8942f9f59b06425ce23f1cd3b7a3db705987b
diff --git a/packages/SystemUI/shared/src/com/android/systemui/shared/plugins/PluginInstance.java b/packages/SystemUI/shared/src/com/android/systemui/shared/plugins/PluginInstance.java
index 92f66902..387f2e1 100644
--- a/packages/SystemUI/shared/src/com/android/systemui/shared/plugins/PluginInstance.java
+++ b/packages/SystemUI/shared/src/com/android/systemui/shared/plugins/PluginInstance.java
@@ -101,7 +101,7 @@
}
/** Alerts listener and plugin that the plugin has been created. */
- public void onCreate() {
+ public synchronized void onCreate() {
boolean loadPlugin = mListener.onPluginAttached(this);
if (!loadPlugin) {
if (mPlugin != null) {
@@ -128,7 +128,7 @@
}
/** Alerts listener and plugin that the plugin is being shutdown. */
- public void onDestroy() {
+ public synchronized void onDestroy() {
logDebug("onDestroy");
unloadPlugin();
mListener.onPluginDetached(this);
@@ -143,12 +143,13 @@
/**
* Loads and creates the plugin if it does not exist.
*/
- public void loadPlugin() {
+ public synchronized void loadPlugin() {
if (mPlugin != null) {
logDebug("Load request when already loaded");
return;
}
+ // Both of these calls take about 1 - 1.5 seconds in test runs
mPlugin = mPluginFactory.createPlugin();
mPluginContext = mPluginFactory.createPluginContext();
if (mPlugin == null || mPluginContext == null) {
@@ -171,7 +172,7 @@
*
* This will free the associated memory if there are not other references.
*/
- public void unloadPlugin() {
+ public synchronized void unloadPlugin() {
if (mPlugin == null) {
logDebug("Unload request when already unloaded");
return;
diff --git a/packages/SystemUI/tests/src/com/android/systemui/shared/plugins/PluginInstanceTest.java b/packages/SystemUI/tests/src/com/android/systemui/shared/plugins/PluginInstanceTest.java
index 6eabf44..5e57c83 100644
--- a/packages/SystemUI/tests/src/com/android/systemui/shared/plugins/PluginInstanceTest.java
+++ b/packages/SystemUI/tests/src/com/android/systemui/shared/plugins/PluginInstanceTest.java
@@ -17,13 +17,17 @@
package com.android.systemui.shared.plugins;
import static junit.framework.Assert.assertEquals;
+import static junit.framework.Assert.assertFalse;
import static junit.framework.Assert.assertNotNull;
import static junit.framework.Assert.assertNull;
+import static junit.framework.Assert.assertTrue;
+import static junit.framework.Assert.fail;
import android.content.ComponentName;
import android.content.Context;
import android.content.pm.ApplicationInfo;
import android.test.suitebuilder.annotation.SmallTest;
+import android.util.Log;
import androidx.test.runner.AndroidJUnit4;
@@ -40,7 +44,11 @@
import java.lang.ref.WeakReference;
import java.util.Collections;
+import java.util.concurrent.Semaphore;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.Supplier;
@SmallTest
@RunWith(AndroidJUnit4.class)
@@ -104,6 +112,7 @@
mPluginInstance = mPluginInstanceFactory.create(
mContext, mAppInfo, TEST_PLUGIN_COMPONENT_NAME,
TestPlugin.class, mPluginListener);
+ mPluginInstance.setIsDebug(true);
mPluginContext = new WeakReference<>(mPluginInstance.getPluginContext());
}
@@ -158,7 +167,7 @@
@Test
public void testOnAttach_SkipLoad() {
- mPluginListener.mAttachReturn = false;
+ mPluginListener.mOnAttach = () -> false;
mPluginInstance.onCreate();
assertEquals(1, mPluginListener.mAttachedCount);
assertEquals(0, mPluginListener.mLoadCount);
@@ -166,6 +175,65 @@
assertInstances(0, 0);
}
+ @Test
+ public void testLoadUnloadSimultaneous_HoldsUnload() throws Exception {
+ final Semaphore loadLock = new Semaphore(1);
+ final Semaphore unloadLock = new Semaphore(1);
+
+ mPluginListener.mOnAttach = () -> false;
+ mPluginListener.mOnLoad = () -> {
+ assertNotNull(mPluginInstance.getPlugin());
+
+ // Allow the bg thread the opportunity to delete the plugin
+ loadLock.release();
+ Thread.yield();
+ boolean isLocked = getLock(unloadLock, 1000);
+
+ // Ensure the bg thread failed to do delete the plugin
+ assertNotNull(mPluginInstance.getPlugin());
+ // We expect that bgThread deadlocked holding the semaphore
+ assertFalse(isLocked);
+ };
+
+ AtomicBoolean isBgThreadFailed = new AtomicBoolean(false);
+ Thread bgThread = new Thread(() -> {
+ assertTrue(getLock(unloadLock, 10));
+ assertTrue(getLock(loadLock, 3000)); // Wait for the foreground thread
+ assertNotNull(mPluginInstance.getPlugin());
+ // Attempt to delete the plugin, this should block until the load completes
+ mPluginInstance.unloadPlugin();
+ assertNull(mPluginInstance.getPlugin());
+ unloadLock.release();
+ loadLock.release();
+ });
+
+ // This protects the test suite from crashing due to the uncaught exception.
+ bgThread.setUncaughtExceptionHandler((Thread t, Throwable ex) -> {
+ Log.e("testLoadUnloadSimultaneous_HoldsUnload", "Exception from BG Thread", ex);
+ isBgThreadFailed.set(true);
+ });
+
+ loadLock.acquire();
+ mPluginInstance.onCreate();
+
+ assertNull(mPluginInstance.getPlugin());
+ bgThread.start();
+ mPluginInstance.loadPlugin();
+
+ bgThread.join(5000);
+ assertFalse(isBgThreadFailed.get());
+ assertNull(mPluginInstance.getPlugin());
+ }
+
+ private boolean getLock(Semaphore lock, long millis) {
+ try {
+ return lock.tryAcquire(millis, TimeUnit.MILLISECONDS);
+ } catch (InterruptedException ex) {
+ fail();
+ return false;
+ }
+ }
+
// This target class doesn't matter, it just needs to have a Requires to hit the flow where
// the mock version info is called.
@ProvidesInterface(action = TestPlugin.ACTION, version = TestPlugin.VERSION)
@@ -226,7 +294,10 @@
}
public class FakeListener implements PluginListener<TestPlugin> {
- public boolean mAttachReturn = true;
+ public Supplier<Boolean> mOnAttach = null;
+ public Runnable mOnDetach = null;
+ public Runnable mOnLoad = null;
+ public Runnable mOnUnload = null;
public int mAttachedCount = 0;
public int mDetachedCount = 0;
public int mLoadCount = 0;
@@ -236,13 +307,16 @@
public boolean onPluginAttached(PluginLifecycleManager<TestPlugin> manager) {
mAttachedCount++;
assertEquals(PluginInstanceTest.this.mPluginInstance, manager);
- return mAttachReturn;
+ return mOnAttach != null ? mOnAttach.get() : true;
}
@Override
public void onPluginDetached(PluginLifecycleManager<TestPlugin> manager) {
mDetachedCount++;
assertEquals(PluginInstanceTest.this.mPluginInstance, manager);
+ if (mOnDetach != null) {
+ mOnDetach.run();
+ }
}
@Override
@@ -261,6 +335,9 @@
assertEquals(expectedContext, pluginContext);
}
assertEquals(PluginInstanceTest.this.mPluginInstance, manager);
+ if (mOnLoad != null) {
+ mOnLoad.run();
+ }
}
@Override
@@ -274,6 +351,9 @@
assertEquals(expectedPlugin, plugin);
}
assertEquals(PluginInstanceTest.this.mPluginInstance, manager);
+ if (mOnUnload != null) {
+ mOnUnload.run();
+ }
}
}
}