Skip to content

Commit

Permalink
Fix MultiInstanceManager leak / improper merge
Browse files Browse the repository at this point in the history
MultiInstanceManager previously failed to unregister listeners attached
to a Singleton, causing the object to leak after it and its associated
Activity were destroyed. As a result, the listener was getting triggered
and incorrectly kicking off a tab model merge, resulting in tabs from
the other Activity's model incorrectly being moved to the wrong
tab state metadata file.

Bug: 1385987
Change-Id: I5a2727a3f09cb0de950b11890f79443d28377b40
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4039782
Reviewed-by: Lijin Shen <lazzzis@google.com>
Commit-Queue: Theresa Sullivan <twellington@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1073661}
  • Loading branch information
Theresa Sullivan authored and Chromium LUCI CQ committed Nov 19, 2022
1 parent 0aae3b0 commit 8fe6d12
Showing 1 changed file with 15 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ public static void onMultiInstanceModeStarted() {
private boolean mIsRecreating;
private int mDisplayId;
private static List<Integer> sTestDisplayIds;
private boolean mDestroyed;

/**
* Create a new {@link MultiInstanceManager}.
Expand Down Expand Up @@ -143,8 +144,12 @@ protected MultiInstanceManager(Activity activity,

@Override
public void onDestroy() {
mDestroyed = true;
mMultiWindowModeStateDispatcher.removeObserver(this);
mMenuOrKeyboardActionController.unregisterMenuOrKeyboardActionHandler(this);
mActivityLifecycleDispatcher.unregister(this);
removeOtherCTAStateObserver();

DisplayManager displayManager =
(DisplayManager) mActivity.getSystemService(Context.DISPLAY_SERVICE);
if (displayManager != null && mDisplayListener != null) {
Expand Down Expand Up @@ -319,15 +324,16 @@ public void onMultiWindowModeChanged(boolean isInMultiWindowMode) {
if (otherResumedCTA == null) {
maybeMergeTabs();
} else {
// Remove the other CTA state observer if one already exists to protect
// against multiple #onMultiWindowModeChanged calls.
// See https://crbug.com/1385987.
removeOtherCTAStateObserver();
// Wait for the other ChromeTabbedActivity to pause before trying to merge
// tabs.
mOtherCTAStateObserver = new ApplicationStatus.ActivityStateListener() {
@Override
public void onActivityStateChange(Activity activity, int newState) {
if (newState == ActivityState.PAUSED) {
removeOtherCTAStateObserver();
maybeMergeTabs();
}
mOtherCTAStateObserver = (activity, newState) -> {
if (newState == ActivityState.PAUSED) {
removeOtherCTAStateObserver();
maybeMergeTabs();
}
};
ApplicationStatus.registerStateListenerForActivity(
Expand Down Expand Up @@ -441,7 +447,8 @@ private void killOtherTask() {
*/
@VisibleForTesting
public void maybeMergeTabs() {
if (!isTabModelMergingEnabled()) return;
assert !mDestroyed;
if (!isTabModelMergingEnabled() || mDestroyed) return;

killOtherTask();
RecordUserAction.record("Android.MergeState.Live");
Expand Down

0 comments on commit 8fe6d12

Please sign in to comment.