From e0420ab2bc8966b7b436c5ec714c4ea2c937edcd Mon Sep 17 00:00:00 2001 From: Sihua Ma Date: Wed, 1 Mar 2023 20:36:10 -0800 Subject: [PATCH] Fix an issue with concurrent modification on widget holders list This could happen when the QuickstepWidgetHolder is not initialized on the main thread, or destroy() is not called on the main thread, while a random widget is removed at the same time which iterates the widget holder list and calls the callbacks. Also fixing an issue with memory leak that is caused by the strong reference in WeakHashMap Test: Presubmit Fix: 271362384 Change-Id: I621ffb29c167cef9842758c5fdefd6bb66dd487e --- .../uioverrides/QuickstepWidgetHolder.java | 76 ++++++++++--------- 1 file changed, 42 insertions(+), 34 deletions(-) diff --git a/quickstep/src/com/android/launcher3/uioverrides/QuickstepWidgetHolder.java b/quickstep/src/com/android/launcher3/uioverrides/QuickstepWidgetHolder.java index 278a45ae26..ff3a2929b6 100644 --- a/quickstep/src/com/android/launcher3/uioverrides/QuickstepWidgetHolder.java +++ b/quickstep/src/com/android/launcher3/uioverrides/QuickstepWidgetHolder.java @@ -22,6 +22,7 @@ import android.appwidget.AppWidgetHost; import android.appwidget.AppWidgetHostView; import android.appwidget.AppWidgetProviderInfo; import android.content.Context; +import android.util.Log; import android.util.SparseArray; import android.widget.RemoteViews; @@ -33,14 +34,14 @@ import androidx.annotation.WorkerThread; import com.android.launcher3.config.FeatureFlags; import com.android.launcher3.model.WidgetsModel; import com.android.launcher3.util.IntSet; -import com.android.launcher3.util.Thunk; import com.android.launcher3.widget.LauncherAppWidgetHostView; import com.android.launcher3.widget.LauncherAppWidgetProviderInfo; import com.android.launcher3.widget.LauncherWidgetHolder; import java.util.ArrayList; +import java.util.Collections; import java.util.List; -import java.util.Map; +import java.util.Set; import java.util.WeakHashMap; import java.util.function.BiConsumer; import java.util.function.IntConsumer; @@ -50,6 +51,8 @@ import java.util.function.IntConsumer; */ public final class QuickstepWidgetHolder extends LauncherWidgetHolder { + private static final String TAG = "QuickstepWidgetHolder"; + private static final UpdateKey KEY_PROVIDER_UPDATE = AppWidgetHostView::onUpdateProviderInfo; private static final UpdateKey KEY_VIEWS_UPDATE = @@ -63,6 +66,8 @@ public final class QuickstepWidgetHolder extends LauncherWidgetHolder { private static AppWidgetHost sWidgetHost = null; + private final SparseArray mViews = new SparseArray<>(); + private final @Nullable RemoteViews.InteractionHandler mInteractionHandler; private final @NonNull IntConsumer mAppWidgetRemovedCallback; @@ -71,15 +76,14 @@ public final class QuickstepWidgetHolder extends LauncherWidgetHolder { // Map to all pending updated keyed with appWidgetId; private final SparseArray mPendingUpdateMap = new SparseArray<>(); - @Thunk - QuickstepWidgetHolder(@NonNull Context context, + private QuickstepWidgetHolder(@NonNull Context context, @Nullable IntConsumer appWidgetRemovedCallback, @Nullable RemoteViews.InteractionHandler interactionHandler) { super(context, appWidgetRemovedCallback); mAppWidgetRemovedCallback = appWidgetRemovedCallback != null ? appWidgetRemovedCallback : i -> {}; mInteractionHandler = interactionHandler; - sHolders.add(this); + MAIN_EXECUTOR.execute(() -> sHolders.add(this)); } @Override @@ -92,7 +96,7 @@ public final class QuickstepWidgetHolder extends LauncherWidgetHolder { sHolders.forEach(h -> h.mAppWidgetRemovedCallback.accept(i))), () -> MAIN_EXECUTOR.execute(() -> sHolders.forEach(h -> h.mProviderChangedListeners.forEach( - ProviderChangedListener::notifyWidgetProvidersChanged))), + ProviderChangedListener::notifyWidgetProvidersChanged))), UI_HELPER_EXECUTOR.getLooper()); if (!WidgetsModel.GO_DISABLE_WIDGETS) { sWidgetHost.startListening(); @@ -107,11 +111,7 @@ public final class QuickstepWidgetHolder extends LauncherWidgetHolder { int count = mPendingUpdateMap.size(); for (int i = 0; i < count; i++) { int widgetId = mPendingUpdateMap.keyAt(i); - QuickstepWidgetHolderListener listener = sListeners.get(widgetId); - if (listener == null) { - continue; - } - AppWidgetHostView view = listener.mView.get(this); + AppWidgetHostView view = mViews.get(widgetId); if (view == null) { continue; } @@ -131,7 +131,16 @@ public final class QuickstepWidgetHolder extends LauncherWidgetHolder { mPendingUpdateMap.clear(); } - private void addPendingAction(int widgetId, UpdateKey key, T data) { + private void onWidgetUpdate(int widgetId, UpdateKey key, T data) { + if (isListening()) { + AppWidgetHostView view = mViews.get(widgetId); + if (view == null) { + return; + } + key.accept(view, data); + return; + } + PendingUpdate pendingUpdate = mPendingUpdateMap.get(widgetId); if (pendingUpdate == null) { pendingUpdate = new PendingUpdate(); @@ -167,7 +176,11 @@ public final class QuickstepWidgetHolder extends LauncherWidgetHolder { */ @Override public void destroy() { - sHolders.remove(this); + try { + MAIN_EXECUTOR.submit(() -> sHolders.remove(this)).get(); + } catch (Exception e) { + Log.e(TAG, "Failed to remove self from holder list", e); + } } @Override @@ -228,15 +241,16 @@ public final class QuickstepWidgetHolder extends LauncherWidgetHolder { } widgetView.setInteractionHandler(mInteractionHandler); widgetView.setAppWidget(appWidgetId, appWidget); + mViews.put(appWidgetId, widgetView); QuickstepWidgetHolderListener listener = sListeners.get(appWidgetId); if (listener == null) { - listener = new QuickstepWidgetHolderListener(appWidgetId, this, widgetView); + listener = new QuickstepWidgetHolderListener(appWidgetId); sWidgetHost.setListener(appWidgetId, listener); sListeners.put(appWidgetId, listener); - } else { - listener.resetView(this, widgetView); } + RemoteViews remoteViews = listener.addHolder(this); + widgetView.updateAppWidget(remoteViews); return widgetView; } @@ -247,31 +261,30 @@ public final class QuickstepWidgetHolder extends LauncherWidgetHolder { @Override public void clearViews() { for (int i = sListeners.size() - 1; i >= 0; i--) { - sListeners.valueAt(i).mView.remove(this); + sListeners.valueAt(i).mListeningHolders.remove(this); } } private static class QuickstepWidgetHolderListener implements AppWidgetHost.AppWidgetHostListener { - @NonNull - private final Map mView = new WeakHashMap<>(); + // Static listeners should use a set that is backed by WeakHashMap to avoid memory leak + private final Set mListeningHolders = Collections.newSetFromMap( + new WeakHashMap<>()); private final int mWidgetId; - @Nullable private RemoteViews mRemoteViews = null; + private @Nullable RemoteViews mRemoteViews; - QuickstepWidgetHolderListener(int widgetId, @NonNull QuickstepWidgetHolder holder, - @NonNull LauncherAppWidgetHostView view) { + QuickstepWidgetHolderListener(int widgetId) { mWidgetId = widgetId; - mView.put(holder, view); } @UiThread - public void resetView(@NonNull QuickstepWidgetHolder holder, - @NonNull AppWidgetHostView view) { - mView.put(holder, view); - view.updateAppWidget(mRemoteViews); + @Nullable + public RemoteViews addHolder(@NonNull QuickstepWidgetHolder holder) { + mListeningHolders.add(holder); + return mRemoteViews; } @Override @@ -295,13 +308,8 @@ public final class QuickstepWidgetHolder extends LauncherWidgetHolder { } private void executeOnMainExecutor(UpdateKey key, T data) { - MAIN_EXECUTOR.execute(() -> mView.forEach((holder, view) -> { - if (holder.isListening()) { - key.accept(view, data); - } else { - holder.addPendingAction(mWidgetId, key, data); - } - })); + MAIN_EXECUTOR.execute(() -> mListeningHolders.forEach(holder -> + holder.onWidgetUpdate(mWidgetId, key, data))); } }