Skip to content

Commit

Permalink
[Messages] Ensure message queue mediator is ready before any animation
Browse files Browse the repository at this point in the history 
The broken behavior in the bug happens when the second message is
enqueued while the first message is still waiting for onStartShowing
to be finished. Currently, the second message assumes the first
message has already been on the screen and accidentally cancels its
previous animation.

During the experiment, most crashes and broken behaviors are caused
during the period in which messages are waiting for mediator to be ready
for showing a message. It appears there are many edge cases during
that period.

One reason is that currently, before executing onStartShowing, we've
mutated some internal variables and made layout changes which are
hard to be reverted if something unexpected occurs during that period.

To simplify the logic and get rid of most edge cases, this CL makes sure
message queue mediator is ready before we commit any mutation and
execute any animation, such as adding views to container.

Bug: 1408627
Change-Id: I73c1cc8dc8f2b5bc9cc8bff2ed6d55c4e38a15c4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4182581
Commit-Queue: Lijin Shen <lazzzis@google.com>
Reviewed-by: Theresa Sullivan <twellington@chromium.org>
Reviewed-by: Aishwarya Rajesh <aishwaryarj@google.com>
Reviewed-by: Matthew Jones <mdjones@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1096296}
  • Loading branch information
Lijin Shen authored and Chromium LUCI CQ committed Jan 24, 2023
1 parent e7376fa commit 9a0ee02
Show file tree
Hide file tree
Showing 7 changed files with 267 additions and 42 deletions.
31 changes: 27 additions & 4 deletions chrome/android/java/src/org/chromium/chrome/browser/messages/ChromeMessageQueueMediator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,33 +164,45 @@ public void destroy() {
mBrowserControlsManager.getBrowserVisibilityDelegate().releasePersistentShowingToken(
mBrowserControlsToken);
}
mBrowserControlsToken = TokenHolder.INVALID_TOKEN;
mBrowserControlsManager = null;
mUrlFocusToken = TokenHolder.INVALID_TOKEN;
mQueueHandler.removeCallbacksAndMessages(null);
mQueueHandler = null;
}

@Override
public void onStartShowing(Runnable runnable) {
public void onRequestShowing(Runnable runnable) {
if (mBrowserControlsManager == null) return;
assert mBrowserControlsToken == TokenHolder.INVALID_TOKEN : "Already requested.";
mBrowserControlsToken =
mBrowserControlsManager.getBrowserVisibilityDelegate().showControlsPersistent();

mContainerCoordinator.showMessageContainer();
final Tab tab = mActivityTabProvider.get();
if (TabBrowserControlsConstraintsHelper.getConstraints(tab) == BrowserControlsState.HIDDEN
|| BrowserControlsUtils.areBrowserControlsFullyVisible(mBrowserControlsManager)) {
if (areBrowserControlsReady()) {
mBrowserControlsObserver.setOneTimeRunnableOnControlsFullyVisible(null);
runnable.run();
} else {
mBrowserControlsObserver.setOneTimeRunnableOnControlsFullyVisible(runnable);
}
}

@Override
public boolean isReadyForShowing() {
return mBrowserControlsToken != TokenHolder.INVALID_TOKEN && areBrowserControlsReady();
}

@Override
public boolean isPendingShow() {
return mBrowserControlsObserver.isRequesting();
}

@Override
public void onFinishHiding() {
if (mBrowserControlsManager == null) return;
mBrowserControlsManager.getBrowserVisibilityDelegate().releasePersistentShowingToken(
mBrowserControlsToken);
mBrowserControlsToken = TokenHolder.INVALID_TOKEN;
mContainerCoordinator.hideMessageContainer();
mBrowserControlsObserver.setOneTimeRunnableOnControlsFullyVisible(null);
}
Expand Down Expand Up @@ -225,6 +237,13 @@ void resumeQueue(int token) {
mQueueController.resume(token);
}

private boolean areBrowserControlsReady() {
final Tab tab = mActivityTabProvider.get();
return TabBrowserControlsConstraintsHelper.getConstraints(tab)
== BrowserControlsState.HIDDEN
|| BrowserControlsUtils.areBrowserControlsFullyVisible(mBrowserControlsManager);
}

/**
* @param layoutStateProvider The provider able to add observer to observe overview mode.
*/
Expand Down Expand Up @@ -289,6 +308,10 @@ void setOneTimeRunnableOnControlsFullyVisible(Runnable runnable) {
Runnable getRunnableForTesting() {
return mRunOnControlsFullyVisible;
}

boolean isRequesting() {
return mRunOnControlsFullyVisible != null;
}
}

@VisibleForTesting
Expand Down
41 changes: 40 additions & 1 deletion ...ndroid/junit/src/org/chromium/chrome/browser/messages/ChromeMessageQueueMediatorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@

import org.junit.Assert;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TestRule;
import org.junit.runner.RunWith;
import org.mockito.ArgumentCaptor;
import org.mockito.Mock;
Expand All @@ -29,12 +31,14 @@
import org.chromium.base.test.BaseRobolectricTestRunner;
import org.chromium.chrome.browser.ActivityTabProvider;
import org.chromium.chrome.browser.browser_controls.BrowserStateBrowserControlsVisibilityDelegate;
import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.fullscreen.BrowserControlsManager;
import org.chromium.chrome.browser.layouts.LayoutStateProvider;
import org.chromium.chrome.browser.layouts.LayoutStateProvider.LayoutStateObserver;
import org.chromium.chrome.browser.layouts.LayoutType;
import org.chromium.chrome.browser.lifecycle.ActivityLifecycleDispatcher;
import org.chromium.chrome.browser.lifecycle.PauseResumeWithNativeObserver;
import org.chromium.chrome.test.util.browser.Features;
import org.chromium.components.messages.ManagedMessageDispatcher;
import org.chromium.ui.modaldialog.ModalDialogManager;
import org.chromium.ui.modaldialog.ModalDialogManager.ModalDialogManagerObserver;
Expand All @@ -50,6 +54,9 @@
public class ChromeMessageQueueMediatorTest {
private static final int EXPECTED_TOKEN = 42;

@Rule
public TestRule mProcessor = new Features.JUnitProcessor();

@Mock
private BrowserControlsManager mBrowserControlsManager;

Expand Down Expand Up @@ -145,6 +152,7 @@ public void testActivityStateChange() {
* Test the runnable by #onStartShow is reset correctly.
*/
@Test
@Features.EnableFeatures({ChromeFeatureList.SUPPRESS_TOOLBAR_CAPTURES})
public void testResetOnStartShowRunnable() {
when(mBrowserControlsManager.getBrowserControlHiddenRatio()).thenReturn(0.5f);
OneshotSupplierImpl<LayoutStateProvider> layoutStateProviderOneShotSupplier =
Expand Down Expand Up @@ -177,13 +185,44 @@ public Boolean get() {
mActivityLifecycleDispatcher, mMessageDispatcher);
ChromeMessageQueueMediator.BrowserControlsObserver observer =
observerArgumentCaptor.getValue();
Assert.assertFalse(mMediator.isReadyForShowing());
Runnable runnable = () -> {};
mMediator.onStartShowing(runnable);
mMediator.onRequestShowing(runnable);
Assert.assertNotNull(observer.getRunnableForTesting());
Assert.assertFalse(mMediator.isReadyForShowing());
Assert.assertTrue(mMediator.isPendingShow());

mMediator.onFinishHiding();
Assert.assertNull("Callback should be reset to null after hiding is finished",
observer.getRunnableForTesting());
Assert.assertFalse(mMediator.isReadyForShowing());
}

/**
* Test whether #IsReadyForShowing returns correct value.
*/
@Test
@Features.EnableFeatures({ChromeFeatureList.SUPPRESS_TOOLBAR_CAPTURES})
public void testIsReadyForShowing() {
final ArgumentCaptor<ChromeMessageQueueMediator.BrowserControlsObserver>
observerArgumentCaptor = ArgumentCaptor.forClass(
ChromeMessageQueueMediator.BrowserControlsObserver.class);
doNothing().when(mBrowserControlsManager).addObserver(observerArgumentCaptor.capture());
var visibilitySupplier = new ObservableSupplierImpl<Boolean>();
visibilitySupplier.set(false);
var visibilityDelegate =
new BrowserStateBrowserControlsVisibilityDelegate(visibilitySupplier);
when(mBrowserControlsManager.getBrowserVisibilityDelegate()).thenReturn(visibilityDelegate);
initMediator();
Assert.assertFalse(mMediator.isReadyForShowing());
visibilitySupplier.set(true);
when(mBrowserControlsManager.getBrowserControlHiddenRatio()).thenReturn(0f);

mMediator.onRequestShowing(() -> {});
Assert.assertTrue(mMediator.isReadyForShowing());

mMediator.onFinishHiding();
Assert.assertFalse(mMediator.isReadyForShowing());
}

/**
Expand Down
55 changes: 36 additions & 19 deletions ...droid/internal/java/src/org/chromium/components/messages/MessageAnimationCoordinator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public void updateWithoutStacking(
}
if (mCurrentDisplayedMessage == null) {
mCurrentDisplayedMessage = candidate;
mMessageQueueDelegate.onStartShowing(() -> {
mMessageQueueDelegate.onRequestShowing(() -> {
if (mCurrentDisplayedMessage == null) {
return;
}
Expand Down Expand Up @@ -172,19 +172,48 @@ public void updateWithStacking(
}
return;
}

var currentFront = mCurrentDisplayedMessages.get(0); // Currently front.
var currentBack = mCurrentDisplayedMessages.get(1); // Currently back.
var nextFront = candidates.get(0); // Next front.
var nextBack = candidates.get(1); // Next back.

// If front message is null, then the back one is definitely null.
assert !(nextFront == null && nextBack != null);
assert !(currentFront == null && currentBack != null);
assert !isSuspended || nextFront == null : "when suspending, all messages should be hidden";
if (currentFront == nextFront && currentBack == nextBack) {
assert currentFront != null
|| !mMessageQueueDelegate.isReadyForShowing()
: "onFinishHiding should have been executed if no message is showing.";
return;
}

if (!isSuspended && !mMessageQueueDelegate.isReadyForShowing()) {
// Make sure everything is ready for showing a message, unless messages are about to
// be removed immediately. By "showing", it does mean not just triggering a showing
// animation, but also holding a message view on screen.
// https://crbug.com/1408627: when showing a second message, it is possible the first
// message is still waiting for message queue delegate to be ready.
// Only request to show a message if not requested yet.
if (!mMessageQueueDelegate.isPendingShow()) {
mMessageQueueDelegate.onRequestShowing(onFinished);
}
return;
}

// Similar to above scenario, second message is about trigger another animation while first
// message is still waiting its animation to be triggered. Early return to avoid cancelling
// that animation accidentally. Second message will be added after its animation is done.
if (mContainer.isIsInitializingLayout()) {
return;
}

// If both animators will be modified, modify FrontAnimator first, because the back message
// relies on the first message in order to adjust its size.
mFrontAnimator = mBackAnimator = null;
boolean animate = !isSuspended;

// If front message is null, then the back one is definitely null.
assert !(nextFront == null && nextBack != null);
assert !(currentFront == null && currentBack != null);
if (currentFront == nextFront && currentBack == nextBack) return;
if (currentFront == null) { // Implies that currently back is also null.
recordAnimationAction(StackingAnimationAction.INSERT_AT_FRONT, nextFront);
mFrontAnimator = nextFront.handler.show(Position.INVISIBLE, Position.FRONT);
Expand Down Expand Up @@ -267,20 +296,7 @@ public void updateWithStacking(
}
}

if (currentFront == null) {
// No message is being displayed now: trigger #onStartShowing.
mCurrentDisplayedMessages = new ArrayList<>(candidates);
// Use ref because when startShowing is finished, other animation might have been
// triggered such that those two member variables have been mutated.
var frontAnimator = mFrontAnimator;
var backAnimator = mBackAnimator;
mMessageQueueDelegate.onStartShowing(() -> {
if (candidates.get(0) == mCurrentDisplayedMessages.get(0)
&& candidates.get(1) == mCurrentDisplayedMessages.get(1)) {
triggerStackingAnimation(candidates, onFinished, frontAnimator, backAnimator);
}
});
} else if (nextFront == null) {
if (nextFront == null) {
// All messages will be hidden: trigger #onFinishHiding.
Runnable runnable = () -> {
mMessageQueueDelegate.onFinishHiding();
Expand All @@ -289,6 +305,7 @@ public void updateWithStacking(
};
triggerStackingAnimation(candidates, runnable, mFrontAnimator, mBackAnimator);
} else {
assert mMessageQueueDelegate.isReadyForShowing();
mCurrentDisplayedMessages = new ArrayList<>(candidates);
triggerStackingAnimation(candidates, onFinished, mFrontAnimator, mBackAnimator);
}
Expand Down

0 comments on commit 9a0ee02

Please sign in to comment.