Reland: Make LayoutManager observer notification async

The process of supplying OverviewModeBehavior is now async to prevent re-entrancy in OneshotSupplier (as of https://crrev.com/c/2321067). Since observer notification in LayoutManager is synchronous, this creates a race condition where observers miss an update because an event (e.g. a touch event, as in the linked bug) queued before the call to set() can trigger synchronous observer notification before observers can add themselves.

Reland note: StatusBarColorController failed on L due to a difference in how colors should be calculated. I'm not positive why TabbedAppMenuTest failed, but it seemed to be a race condition triggered (or uncovered) by the change in timing caused by making observer notification async. I think I've fixed it by blocking on the call to showAppMenu rather than polling the status via the instrumentation thread.

Bug: 1129783
Change-Id: I88ac8efcc845826f0396cb624450ef1ba8d5b399
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2427428Reviewed-by: Xi Han <hanxi@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Commit-Queue: Patrick Noland <pnoland@chromium.org>
Cr-Commit-Position: refs/heads/master@{#810429}

chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java

@@ -561,9 +561,8 @@ public class ChromeTabbedActivity extends ChromeActivity<ChromeActivityComponent
                 }
             }
             mLayoutManager = new LayoutManagerChromePhone(compositorViewHolder, mContentContainer,
-                    mStartSurface, getTabContentManagerSupplier());
+                    mStartSurface, getTabContentManagerSupplier(), mOverviewModeBehaviorSupplier);
             mOverviewModeController = mLayoutManager;
-            mOverviewModeBehaviorSupplier.set(mOverviewModeController);
         }
     }
 
@@ -572,10 +571,10 @@ public class ChromeTabbedActivity extends ChromeActivity<ChromeActivityComponent
 
         try (TraceEvent e = TraceEvent.scoped(
                      "ChromeTabbedActivity.setupCompositorContentPreNativeForTablet")) {
-            mLayoutManager = new LayoutManagerChromeTablet(
-                    getCompositorViewHolder(), mContentContainer, getTabContentManagerSupplier());
+            mLayoutManager =
+                    new LayoutManagerChromeTablet(getCompositorViewHolder(), mContentContainer,
+                            getTabContentManagerSupplier(), mOverviewModeBehaviorSupplier);
             mOverviewModeController = mLayoutManager;
-            mOverviewModeBehaviorSupplier.set(mOverviewModeController);
         }
     }
 
@@ -839,7 +838,7 @@ public class ChromeTabbedActivity extends ChromeActivity<ChromeActivityComponent
         if (!mPendingInitialTabCreation
                 && !(TabUiFeatureUtilities.supportInstantStart(isTablet())
                         && shouldShowTabSwitcherOnStart())) {
-            setInitialOverviewStateAsync();
+            setInitialOverviewState();
         }
 
         if (TabUiFeatureUtilities.isConditionalTabStripEnabled()
@@ -916,14 +915,7 @@ public class ChromeTabbedActivity extends ChromeActivity<ChromeActivityComponent
         }
     }
 
-    // Posts a call to setInitialOverviewStateSync that will trigger after observers of
-    // mOverviewModeBehaviorSupplier have a chance to process the supplied OverviewModeBehavior and
-    // add themselves as observers.
-    private void setInitialOverviewStateAsync() {
-        mOverviewModeBehaviorSupplier.onAvailable((unused) -> setInitalOverviewStateSync());
-    }
-
-    private void setInitalOverviewStateSync() {
+    private void setInitialOverviewState() {
         boolean isOverviewVisible = mOverviewModeController.overviewVisible();
 
         if (shouldShowTabSwitcherOnStart() && !isOverviewVisible) {
@@ -1135,7 +1127,7 @@ public class ChromeTabbedActivity extends ChromeActivity<ChromeActivityComponent
         if (hasStartWithNativeBeenCalled()
                 && !(TabUiFeatureUtilities.supportInstantStart(isTablet())
                         && shouldShowTabSwitcherOnStart())) {
-            setInitialOverviewStateAsync();
+            setInitialOverviewState();
         }
     }
 
@@ -1520,7 +1512,7 @@ public class ChromeTabbedActivity extends ChromeActivity<ChromeActivityComponent
             if (shouldShowTabSwitcherOnStart()) {
                 mLayoutManager.setTabModelSelector(mTabModelSelectorImpl);
                 mIsAccessibilityTabSwitcherEnabled = DeviceClassManager.enableAccessibilityLayout();
-                setInitialOverviewStateAsync();
+                setInitialOverviewState();
             }
         }
     }

chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerChrome.java

@@ -14,6 +14,7 @@ import org.chromium.base.Callback;
 import org.chromium.base.ObserverList;
 import org.chromium.base.metrics.RecordUserAction;
 import org.chromium.base.supplier.ObservableSupplier;
+import org.chromium.base.supplier.OneshotSupplierImpl;
 import org.chromium.chrome.browser.ActivityTabProvider;
 import org.chromium.chrome.browser.accessibility_tab_switcher.OverviewListLayout;
 import org.chromium.chrome.browser.browser_controls.BrowserControlsStateProvider;
@@ -76,6 +77,7 @@ public class LayoutManagerChrome
     private boolean mCreateOverviewLayout;
 
     protected ObservableSupplier<TabContentManager> mTabContentManagerSupplier;
+    private final OneshotSupplierImpl<OverviewModeBehavior> mOverviewModeBehaviorSupplier;
 
     /**
      * Creates the {@link LayoutManagerChrome} instance.
@@ -87,7 +89,8 @@ public class LayoutManagerChrome
      */
     public LayoutManagerChrome(LayoutManagerHost host, ViewGroup contentContainer,
             boolean createOverviewLayout, @Nullable StartSurface startSurface,
-            ObservableSupplier<TabContentManager> tabContentManagerSupplier) {
+            ObservableSupplier<TabContentManager> tabContentManagerSupplier,
+            OneshotSupplierImpl<OverviewModeBehavior> overviewModeBehaviorSupplier) {
         super(host, contentContainer, tabContentManagerSupplier);
         Context context = host.getContext();
         LayoutRenderHost renderHost = host.getLayoutRenderHost();
@@ -119,10 +122,8 @@ public class LayoutManagerChrome
                     @Override
                     public void onStateChanged(@OverviewModeState int overviewModeState,
                             boolean shouldShowTabSwitcherToolbar) {
-                        for (OverviewModeObserver observer : mOverviewModeObservers) {
-                            observer.onOverviewModeStateChanged(
-                                    overviewModeState, shouldShowTabSwitcherToolbar);
-                        }
+                        notifyObserversStateChanged(
+                                overviewModeState, shouldShowTabSwitcherToolbar);
                     }
                 });
                 final ObservableSupplier<? extends BrowserControlsStateProvider>
@@ -134,6 +135,9 @@ public class LayoutManagerChrome
                 mCreateOverviewLayout = true;
             }
         }
+
+        mOverviewModeBehaviorSupplier = overviewModeBehaviorSupplier;
+        mOverviewModeBehaviorSupplier.set(this);
     }
 
     /**
@@ -278,9 +282,7 @@ public class LayoutManagerChrome
         if (isOverviewLayout(layoutBeingShown)) {
             boolean showToolbar = animate && (!mEnableAnimations
                     || getTabModelSelector().getCurrentModel().getCount() <= 0);
-            for (OverviewModeObserver observer : mOverviewModeObservers) {
-                observer.onOverviewModeStartedShowing(showToolbar);
-            }
+            notifyObserversStartedShowing(showToolbar);
         }
     }
 
@@ -298,9 +300,7 @@ public class LayoutManagerChrome
 
             boolean creatingNtp = layoutBeingHidden == mOverviewLayout && mCreatingNtp;
 
-            for (OverviewModeObserver observer : mOverviewModeObservers) {
-                observer.onOverviewModeStartedHiding(showToolbar, creatingNtp);
-            }
+            notifyObserversStartedHiding(showToolbar, creatingNtp);
         }
     }
 
@@ -309,9 +309,7 @@ public class LayoutManagerChrome
         super.doneShowing();
 
         if (isOverviewLayout(getActiveLayout())) {
-            for (OverviewModeObserver observer : mOverviewModeObservers) {
-                observer.onOverviewModeFinishedShowing();
-            }
+            notifyObserversFinishedShowing();
         }
     }
 
@@ -327,9 +325,7 @@ public class LayoutManagerChrome
         super.doneHiding();
 
         if (isOverviewLayout(layoutBeingHidden)) {
-            for (OverviewModeObserver observer : mOverviewModeObservers) {
-                observer.onOverviewModeFinishedHiding();
-            }
+            notifyObserversFinishedHiding();
         }
     }
 
@@ -596,4 +592,46 @@ public class LayoutManagerChrome
                     .closeTab(lastTab, tab, false, false, false);
         }
     }
+
+    private void notifyObserversStartedShowing(boolean showToolbar) {
+        mOverviewModeBehaviorSupplier.onAvailable((unused) -> {
+            for (OverviewModeObserver overviewModeObserver : mOverviewModeObservers) {
+                overviewModeObserver.onOverviewModeStartedShowing(showToolbar);
+            }
+        });
+    }
+
+    private void notifyObserversFinishedShowing() {
+        mOverviewModeBehaviorSupplier.onAvailable((unused) -> {
+            for (OverviewModeObserver overviewModeObserver : mOverviewModeObservers) {
+                overviewModeObserver.onOverviewModeFinishedShowing();
+            }
+        });
+    }
+
+    private void notifyObserversStartedHiding(boolean showToolbar, boolean creatingNtp) {
+        mOverviewModeBehaviorSupplier.onAvailable((unused) -> {
+            for (OverviewModeObserver overviewModeObserver : mOverviewModeObservers) {
+                overviewModeObserver.onOverviewModeStartedHiding(showToolbar, creatingNtp);
+            }
+        });
+    }
+
+    private void notifyObserversFinishedHiding() {
+        mOverviewModeBehaviorSupplier.onAvailable((unused) -> {
+            for (OverviewModeObserver overviewModeObserver : mOverviewModeObservers) {
+                overviewModeObserver.onOverviewModeFinishedHiding();
+            }
+        });
+    }
+
+    private void notifyObserversStateChanged(
+            @OverviewModeState int overviewModeState, boolean shouldShowTabSwitcherToolbar) {
+        mOverviewModeBehaviorSupplier.onAvailable((unused) -> {
+            for (OverviewModeObserver overviewModeObserver : mOverviewModeObservers) {
+                overviewModeObserver.onOverviewModeStateChanged(
+                        overviewModeState, shouldShowTabSwitcherToolbar);
+            }
+        });
+    }
 }

chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerChromePhone.java

@@ -8,6 +8,7 @@ import android.content.Context;
 import android.view.ViewGroup;
 
 import org.chromium.base.supplier.ObservableSupplier;
+import org.chromium.base.supplier.OneshotSupplierImpl;
 import org.chromium.chrome.browser.ActivityTabProvider;
 import org.chromium.chrome.browser.compositor.layouts.content.TabContentManager;
 import org.chromium.chrome.browser.compositor.layouts.phone.SimpleAnimationLayout;
@@ -41,8 +42,10 @@ public class LayoutManagerChromePhone extends LayoutManagerChrome {
      */
     public LayoutManagerChromePhone(LayoutManagerHost host, ViewGroup contentContainer,
             StartSurface startSurface,
-            ObservableSupplier<TabContentManager> tabContentManagerSupplier) {
-        super(host, contentContainer, true, startSurface, tabContentManagerSupplier);
+            ObservableSupplier<TabContentManager> tabContentManagerSupplier,
+            OneshotSupplierImpl<OverviewModeBehavior> overviewModeBehaviorSupplier) {
+        super(host, contentContainer, true, startSurface, tabContentManagerSupplier,
+                overviewModeBehaviorSupplier);
     }
 
     @Override

chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerChromeTablet.java

@@ -7,6 +7,7 @@ package org.chromium.chrome.browser.compositor.layouts;
 import android.view.ViewGroup;
 
 import org.chromium.base.supplier.ObservableSupplier;
+import org.chromium.base.supplier.OneshotSupplierImpl;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.ActivityTabProvider;
 import org.chromium.chrome.browser.compositor.layouts.content.TabContentManager;
@@ -37,8 +38,10 @@ public class LayoutManagerChromeTablet extends LayoutManagerChrome {
      * @param tabContentManagerSupplier Supplier of the {@link TabContentManager} instance.
      */
     public LayoutManagerChromeTablet(LayoutManagerHost host, ViewGroup contentContainer,
-            ObservableSupplier<TabContentManager> tabContentManagerSupplier) {
-        super(host, contentContainer, false, null, tabContentManagerSupplier);
+            ObservableSupplier<TabContentManager> tabContentManagerSupplier,
+            OneshotSupplierImpl<OverviewModeBehavior> overviewModeBehaviorSupplier) {
+        super(host, contentContainer, false, null, tabContentManagerSupplier,
+                overviewModeBehaviorSupplier);
 
         mTabStripLayoutHelperManager = new StripLayoutHelperManager(
                 host.getContext(), this, mHost.getLayoutRenderHost(), () -> mTitleCache);

chrome/android/javatests/src/org/chromium/chrome/browser/app/appmenu/TabbedAppMenuTest.java

@@ -237,7 +237,8 @@ public class TabbedAppMenuTest {
                 () -> mActivityTestRule.getActivity().getLayoutManager().hideOverview(false));
         Assert.assertFalse("Overview shouldn't be showing.",
                 mActivityTestRule.getActivity().getOverviewModeBehavior().overviewVisible());
-        Assert.assertFalse("App menu shouldn't be showing.", mAppMenuHandler.isAppMenuShowing());
+        CriteriaHelper.pollUiThread(
+                () -> !mAppMenuHandler.isAppMenuShowing(), "App menu shouldn't be showing.");
     }
 
     @Test
@@ -374,12 +375,10 @@ public class TabbedAppMenuTest {
     }
 
     private void showAppMenuAndAssertMenuShown() {
-        PostTask.runOrPostTask(UiThreadTaskTraits.DEFAULT,
-                ()
-                        -> AppMenuTestSupport.showAppMenu(
-                                mActivityTestRule.getAppMenuCoordinator(), null, false));
-        CriteriaHelper.pollInstrumentationThread(
-                () -> mAppMenuHandler.isAppMenuShowing(), "AppMenu did not show");
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            AppMenuTestSupport.showAppMenu(mActivityTestRule.getAppMenuCoordinator(), null, false);
+            Assert.assertTrue(mAppMenuHandler.isAppMenuShowing());
+        });
     }
 
     private void hitEnterAndAssertAppMenuDismissed() {

chrome/android/javatests/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerTest.java

@@ -30,6 +30,7 @@ import org.mockito.MockitoAnnotations;
 
 import org.chromium.base.MathUtils;
 import org.chromium.base.supplier.ObservableSupplierImpl;
+import org.chromium.base.supplier.OneshotSupplierImpl;
 import org.chromium.base.test.UiThreadTest;
 import org.chromium.base.test.util.CommandLineFlags;
 import org.chromium.base.test.util.DisabledTest;
@@ -166,8 +167,10 @@ public class LayoutManagerTest implements MockTabModelDelegate {
 
         ObservableSupplierImpl<TabContentManager> tabContentManagerSupplier =
                 new ObservableSupplierImpl<>();
-        mManagerPhone = new LayoutManagerChromePhone(
-                layoutManagerHost, container, null, tabContentManagerSupplier);
+        OneshotSupplierImpl<OverviewModeBehavior> overviewModeBehaviorSupplier =
+                new OneshotSupplierImpl<>();
+        mManagerPhone = new LayoutManagerChromePhone(layoutManagerHost, container, null,
+                tabContentManagerSupplier, overviewModeBehaviorSupplier);
         tabContentManagerSupplier.set(tabContentManager);
         mManager = mManagerPhone;
         CompositorAnimationHandler.setTestingMode(true);

chrome/android/javatests/src/org/chromium/chrome/browser/ui/system/StatusBarColorControllerTest.java

@@ -4,6 +4,7 @@
 
 package org.chromium.chrome.browser.ui.system;
 
+import android.app.Activity;
 import android.content.res.Resources;
 import android.graphics.Color;
 import android.os.Build;
@@ -11,6 +12,7 @@ import android.os.Build;
 import androidx.annotation.ColorInt;
 import androidx.test.filters.LargeTest;
 
+import org.hamcrest.Matchers;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Rule;
@@ -34,10 +36,15 @@ import org.chromium.chrome.test.ChromeTabbedActivityTestRule;
 import org.chromium.chrome.test.util.browser.Features;
 import org.chromium.chrome.test.util.browser.ThemeTestUtils;
 import org.chromium.components.browser_ui.styles.ChromeColors;
+import org.chromium.content_public.browser.test.util.Criteria;
+import org.chromium.content_public.browser.test.util.CriteriaHelper;
 import org.chromium.content_public.browser.test.util.TestThreadUtils;
 import org.chromium.ui.test.util.UiRestriction;
 import org.chromium.ui.util.ColorUtils;
 
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeoutException;
+
 /**
  * {@link StatusBarColorController} tests.
  * There are additional status bar color tests in {@link BrandColorTest}.
@@ -68,7 +75,7 @@ public class StatusBarColorControllerTest {
     @Feature({"StatusBar"})
     @MinAndroidSdkLevel(Build.VERSION_CODES.LOLLIPOP_MR1)
     @Restriction({UiRestriction.RESTRICTION_TYPE_PHONE}) // Status bar is always black on tablets
-    public void testColorToggleIncongitoInOverview() {
+    public void testColorToggleIncongitoInOverview() throws Exception {
         ChromeTabbedActivity activity = mActivityTestRule.getActivity();
         Resources resources = activity.getResources();
         final int expectedOverviewStandardColor = defaultColorFallbackToBlack(
@@ -84,8 +91,7 @@ public class StatusBarColorControllerTest {
         TestThreadUtils.runOnUiThreadBlocking(
                 () -> { activity.getLayoutManager().showOverview(false /* animate */); });
 
-        ThemeTestUtils.assertStatusBarColor(activity, expectedOverviewIncognitoColor);
-
+        waitForStatusBarColor(activity, expectedOverviewIncognitoColor);
         TestThreadUtils.runOnUiThreadBlocking(
                 () -> { tabModelSelector.selectModel(false /* incongito */); });
         ThemeTestUtils.assertStatusBarColor(activity, expectedOverviewStandardColor);
@@ -109,11 +115,11 @@ public class StatusBarColorControllerTest {
                 "/chrome/test/data/android/theme_color_test.html");
         mActivityTestRule.loadUrl(pageWithBrandColorUrl);
         ThemeTestUtils.waitForThemeColor(activity, Color.RED);
-        ThemeTestUtils.assertStatusBarColor(activity, Color.RED);
+        waitForStatusBarColor(activity, Color.RED);
 
         TestThreadUtils.runOnUiThreadBlocking(
                 () -> { activity.getLayoutManager().showOverview(false /* animate */); });
-        ThemeTestUtils.assertStatusBarColor(activity, expectedDefaultStandardColor);
+        waitForStatusBarColor(activity, expectedDefaultStandardColor);
     }
 
     /**
@@ -191,4 +197,15 @@ public class StatusBarColorControllerTest {
         final int scrimColorOpaque = mScrimColor & 0xFF000000;
         return ColorUtils.getColorWithOverlay(color, scrimColorOpaque, fraction * scrimColorAlpha);
     }
+
+    private void waitForStatusBarColor(Activity activity, int expectedColor)
+            throws ExecutionException, TimeoutException {
+        final int actualExpectedColor = Build.VERSION.SDK_INT < Build.VERSION_CODES.M
+                ? ColorUtils.getDarkenedColorForStatusBar(expectedColor)
+                : expectedColor;
+        CriteriaHelper.pollUiThread(() -> {
+            Criteria.checkThat(
+                    activity.getWindow().getStatusBarColor(), Matchers.is(actualExpectedColor));
+        }, CriteriaHelper.DEFAULT_MAX_TIME_TO_POLL, CriteriaHelper.DEFAULT_POLLING_INTERVAL);
+    }
 }