Updated FirstRunActivity#exitFirstRun() to skip FRE for pending intent.

The old handling of exitFirstRun() simply relaunched the pending intent.
This turned out to be a problem because the intent dispatcher read the
durable settings for if the FRE was needed, and would decide it was.
Interestingly, there can only be one FirstRunActivity active at a time,
and so the relaunch was actually ignored. This would cause the existing
FirstRunActivity get stuck because the replacement activity was never
started.

This fixes the problem by introducing a new "ephemeral" static variable
that remembers we just decided to skip the FRE, so the next intent
dispatch/routing will be aware of this decision.

Bug: 1106987
Change-Id: Id8502956236f76c93091452fe580ae0d6fea2c80
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2343553Reviewed-by: Yusuf Ozuysal <yusufo@chromium.org>
Reviewed-by: Theresa  <twellington@chromium.org>
Reviewed-by: Wenyu Fu <wenyufu@chromium.org>
Commit-Queue: Sky Malice <skym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#799483}

chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionService.java

@@ -117,7 +117,7 @@ public class CustomTabsConnectionService extends CustomTabsService {
 
     private boolean isFirstRunDone() {
         if (mBindIntent == null) return true;
-        boolean firstRunNecessary = FirstRunFlowSequencer.checkIfFirstRunIsNecessary(false);
+        boolean firstRunNecessary = FirstRunFlowSequencer.checkIfFirstRunIsNecessary(false, true);
         if (!firstRunNecessary) {
             mBindIntent = null;
             return true;

chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java

@@ -401,11 +401,19 @@ public class FirstRunActivity extends FirstRunActivityBase implements FirstRunPa
         SearchWidgetProvider.updateCachedEngineName();
         if (sObserver != null) sObserver.onUpdateCachedEngineName();
 
-        exitFirstRun();
+        launchPendingIntentAndFinish();
     }
 
     @Override
     public void exitFirstRun() {
+        // This is important because the first run, when completed, will re-launch the original
+        // intent. The re-launched intent will still need to know to avoid the FRE.
+        FirstRunStatus.setEphemeralSkipFirstRun(true);
+
+        launchPendingIntentAndFinish();
+    }
+
+    private void launchPendingIntentAndFinish() {
         if (!sendFirstRunCompletePendingIntent()) {
             finish();
         } else {

chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencer.java

@@ -18,6 +18,7 @@ import org.chromium.base.CommandLine;
 import org.chromium.base.IntentUtils;
 import org.chromium.base.Log;
 import org.chromium.chrome.browser.IntentHandler;
+import org.chromium.chrome.browser.LaunchIntentDispatcher;
 import org.chromium.chrome.browser.flags.CachedFeatureFlags;
 import org.chromium.chrome.browser.flags.ChromeFeatureList;
 import org.chromium.chrome.browser.flags.ChromeSwitches;
@@ -220,11 +221,26 @@ public abstract class FirstRunFlowSequencer  {
     }
 
     /**
-     * Checks if the First Run needs to be launched.
+     * Checks if the First Run Experience needs to be launched.
      * @param preferLightweightFre Whether to prefer the Lightweight First Run Experience.
+     * @param fromIntent Intent used to launch the caller.
      * @return Whether the First Run Experience needs to be launched.
      */
-    public static boolean checkIfFirstRunIsNecessary(boolean preferLightweightFre) {
+    public static boolean checkIfFirstRunIsNecessary(
+            boolean preferLightweightFre, Intent fromIntent) {
+        boolean isCct = fromIntent.getBooleanExtra(
+                                FirstRunActivityBase.EXTRA_CHROME_LAUNCH_INTENT_IS_CCT, false)
+                || LaunchIntentDispatcher.isCustomTabIntent(fromIntent);
+        return checkIfFirstRunIsNecessary(preferLightweightFre, isCct);
+    }
+
+    /**
+     * Checks if the First Run Experience needs to be launched.
+     * @param preferLightweightFre Whether to prefer the Lightweight First Run Experience.
+     * @param isCct Whether this check is being made in the context of a CCT.
+     * @return Whether the First Run Experience needs to be launched.
+     */
+    public static boolean checkIfFirstRunIsNecessary(boolean preferLightweightFre, boolean isCct) {
         // If FRE is disabled (e.g. in tests), proceed directly to the intent handling.
         if (CommandLine.getInstance().hasSwitch(ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE)
                 || ApiCompatibilityUtils.isDemoUser()
@@ -235,6 +251,13 @@ public abstract class FirstRunFlowSequencer  {
             // Promo pages are removed, so there is nothing else to show in FRE.
             return false;
         }
+        if (isCct && FirstRunStatus.isEphemeralSkipFirstRun()) {
+            // Domain policies may have caused CCTs to skip the FRE. While this needs to be figured
+            // out at runtime for each app restart, it should apply to all CCTs for the duration of
+            // the app's lifetime.
+            // TODO(https://crbug.com/1108582): Replace this with a shared pref.
+            return false;
+        }
         return !preferLightweightFre
                 || (!FirstRunStatus.shouldSkipWelcomePage()
                            && !FirstRunStatus.getLightweightFirstRunFlowComplete());
@@ -253,7 +276,7 @@ public abstract class FirstRunFlowSequencer  {
     public static boolean launch(Context caller, Intent fromIntent, boolean requiresBroadcast,
             boolean preferLightweightFre) {
         // Check if the user needs to go through First Run at all.
-        if (!checkIfFirstRunIsNecessary(preferLightweightFre)) return false;
+        if (!checkIfFirstRunIsNecessary(preferLightweightFre, fromIntent)) return false;
 
         String intentUrl = IntentHandler.getUrlFromIntent(fromIntent);
         Uri uri = intentUrl != null ? Uri.parse(intentUrl) : null;

chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunStatus.java

@@ -16,6 +16,36 @@ public class FirstRunStatus {
     // Whether the first run flow is triggered in the current browser session.
     private static boolean sFirstRunTriggered;
 
+    // Whether the first run flow should be skipped for the current browser session.
+    private static boolean sEphemeralSkipFirstRun;
+
+    /** @param triggered whether the first run flow is triggered in the current browser session. */
+    public static void setFirstRunTriggered(boolean triggered) {
+        sFirstRunTriggered = triggered;
+    }
+
+    /** @return whether first run flow is triggered in the current browser session. */
+    public static boolean isFirstRunTriggered() {
+        return sFirstRunTriggered;
+    }
+
+    /**
+     * @param skip Whether the first run flow should be skipped for the current session for app
+     *             entry points that allow for this (e.g. CCTs via Enterprise policy). Not saved to
+     *             durable storage, and will be erased when the process is restarted.
+     */
+    public static void setEphemeralSkipFirstRun(boolean skip) {
+        sEphemeralSkipFirstRun = skip;
+    }
+
+    /**
+     * @return Whether the first run flow should be skipped for the current session for app entry
+     *         points that allow for this.
+     */
+    public static boolean isEphemeralSkipFirstRun() {
+        return sEphemeralSkipFirstRun;
+    }
+
     /**
      * Sets the "main First Run Experience flow complete" preference.
      * @param isComplete Whether the main First Run Experience flow is complete
@@ -39,22 +69,6 @@ public class FirstRunStatus {
                 ChromeSwitches.FORCE_FIRST_RUN_FLOW_COMPLETE_FOR_TESTING);
     }
 
-    /**
-     * Sets whether the first run flow is triggered in the current browser session.
-     * @param triggered
-     * @return
-     */
-    public static void setFirstRunTriggered(boolean triggered) {
-        sFirstRunTriggered = triggered;
-    }
-
-    /**
-     * Returns whether first run flow is triggered in the current browser session.
-     */
-    public static boolean isFirstRunTriggered() {
-        return sFirstRunTriggered;
-    }
-
     /**
     * Sets the preference to skip the welcome page from the main First Run Experience.
      * @param isSkip Whether the welcome page should be skpped

chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java

@@ -76,8 +76,11 @@ class NativeInitializationController {
             return;
         }
 
-        assert mBackgroundTasksComplete == null;
-        boolean fetchVariationsSeed = FirstRunFlowSequencer.checkIfFirstRunIsNecessary(false);
+        // This is a fairly low cost way to check if fetching the variations seed is needed. It can
+        // produces false positives, but that's okay. There's a later mechanism that checks a
+        // dedicated durable field to make sure the actual network request is only made once.
+        boolean fetchVariationsSeed = FirstRunFlowSequencer.checkIfFirstRunIsNecessary(
+                false, mActivityDelegate.getInitialIntent());
 
         mBackgroundTasksComplete = false;
         new AsyncInitTaskRunner() {

chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java

@@ -390,7 +390,7 @@ public class SearchWidgetProvider extends AppWidgetProvider {
     }
 
     static boolean shouldShowFullString() {
-        boolean freIsNotNecessary = !FirstRunFlowSequencer.checkIfFirstRunIsNecessary(false);
+        boolean freIsNotNecessary = !FirstRunFlowSequencer.checkIfFirstRunIsNecessary(false, false);
         boolean noNeedToCheckForSearchDialog =
                 !LocaleManager.getInstance().needToCheckForSearchEnginePromo();
         return freIsNotNecessary && noNeedToCheckForSearchDialog;

chrome/android/javatests/src/org/chromium/chrome/browser/firstrun/FirstRunIntegrationTest.java

@@ -4,6 +4,8 @@
 
 package org.chromium.chrome.browser.firstrun;
 
+import static org.mockito.ArgumentMatchers.any;
+
 import android.app.Activity;
 import android.app.Instrumentation;
 import android.app.Instrumentation.ActivityMonitor;
@@ -24,11 +26,17 @@ import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.MockitoAnnotations;
 
+import org.chromium.base.Callback;
+import org.chromium.base.CollectionUtil;
 import org.chromium.base.task.PostTask;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.ChromeTabbedActivity;
 import org.chromium.chrome.browser.customtabs.CustomTabActivity;
+import org.chromium.chrome.browser.customtabs.CustomTabsTestUtils;
 import org.chromium.chrome.browser.document.ChromeLauncherActivity;
 import org.chromium.chrome.browser.locale.DefaultSearchEngineDialogHelperUtils;
 import org.chromium.chrome.browser.locale.LocaleManager;
@@ -40,8 +48,12 @@ import org.chromium.components.search_engines.TemplateUrl;
 import org.chromium.content_public.browser.UiThreadTaskTraits;
 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 java.util.HashMap;
 import java.util.List;
+import java.util.Map;
+import java.util.Set;
 
 /**
  * Integration test suite for the first run experience.
@@ -52,80 +64,102 @@ public class FirstRunIntegrationTest {
     @Rule
     public MultiActivityTestRule mTestRule = new MultiActivityTestRule();
 
+    @Mock
+    public FirstRunAppRestrictionInfo mMockAppRestrictionInfo;
+
+    private final Set<Class> mSupportedActivities =
+            CollectionUtil.newHashSet(ChromeLauncherActivity.class, FirstRunActivity.class,
+                    ChromeTabbedActivity.class, CustomTabActivity.class);
+    private final Map<Class, ActivityMonitor> mMonitorMap = new HashMap<>();
+    private Instrumentation mInstrumentation;
+    private Context mContext;
+
     private FirstRunActivityTestObserver mTestObserver = new FirstRunActivityTestObserver();
-    private Activity mActivity;
+    private Activity mLastActivity;
 
     @Before
     public void setUp() {
+        MockitoAnnotations.initMocks(this);
         FirstRunActivity.setObserverForTest(mTestObserver);
+
+        mInstrumentation = InstrumentationRegistry.getInstrumentation();
+        mContext = mInstrumentation.getTargetContext();
+        for (Class clazz : mSupportedActivities) {
+            ActivityMonitor monitor = new ActivityMonitor(clazz.getName(), null, false);
+            mMonitorMap.put(clazz, monitor);
+            mInstrumentation.addMonitor(monitor);
+        }
     }
 
     @After
     public void tearDown() {
-        if (mActivity != null) mActivity.finish();
+        FirstRunActivity.setEnableEnterpriseCCTForTest(false);
+        FirstRunAppRestrictionInfo.setInstanceForTest(null);
+        TosAndUmaFirstRunFragmentWithEnterpriseSupport.setBlockPolicyLoadingForTest(false);
+        if (mLastActivity != null) mLastActivity.finish();
+    }
+
+    private ActivityMonitor getMonitor(Class activityClass) {
+        Assert.assertTrue(mSupportedActivities.contains(activityClass));
+        return mMonitorMap.get(activityClass);
+    }
+
+    private <T extends Activity> T waitForActivity(Class<T> activityClass) {
+        Assert.assertTrue(mSupportedActivities.contains(activityClass));
+        ActivityMonitor monitor = getMonitor(activityClass);
+        mLastActivity = mInstrumentation.waitForMonitorWithTimeout(
+                monitor, CriteriaHelper.DEFAULT_MAX_TIME_TO_POLL);
+        Assert.assertNotNull(mLastActivity);
+        return (T) mLastActivity;
+    }
+
+    private void setHasAppRestrictionForMock() {
+        Mockito.doAnswer(invocation -> {
+                   Callback<Boolean> callback = invocation.getArgument(0);
+                   callback.onResult(true);
+                   return null;
+               })
+                .when(mMockAppRestrictionInfo)
+                .getHasAppRestriction(any());
+        FirstRunAppRestrictionInfo.setInstanceForTest(mMockAppRestrictionInfo);
     }
 
     @Test
     @SmallTest
     public void testHelpPageSkipsFirstRun() {
-        final ActivityMonitor customTabActivityMonitor =
-                new ActivityMonitor(CustomTabActivity.class.getName(), null, false);
-        final ActivityMonitor freMonitor =
-                new ActivityMonitor(FirstRunActivity.class.getName(), null, false);
-
-        Instrumentation instrumentation = InstrumentationRegistry.getInstrumentation();
-        instrumentation.addMonitor(customTabActivityMonitor);
-        instrumentation.addMonitor(freMonitor);
-
         // Fire an Intent to load a generic URL.
-        final Context context = instrumentation.getTargetContext();
-        CustomTabActivity.showInfoPage(context, "http://google.com");
+        CustomTabActivity.showInfoPage(mContext, "http://google.com");
 
         // The original activity should be started because it's a "help page".
-        final Activity original = instrumentation.waitForMonitorWithTimeout(
-                customTabActivityMonitor, CriteriaHelper.DEFAULT_MAX_TIME_TO_POLL);
-        Assert.assertNotNull(original);
-        Assert.assertFalse(original.isFinishing());
+        waitForActivity(CustomTabActivity.class);
+        Assert.assertFalse(mLastActivity.isFinishing());
 
         // First run should be skipped for this Activity.
-        Assert.assertEquals(0, freMonitor.getHits());
+        Assert.assertEquals(0, getMonitor(FirstRunActivity.class).getHits());
     }
 
     @Test
     @SmallTest
     public void testAbortFirstRun() throws Exception {
-        Instrumentation instrumentation = InstrumentationRegistry.getInstrumentation();
-        final ActivityMonitor launcherActivityMonitor =
-                new ActivityMonitor(ChromeLauncherActivity.class.getName(), null, false);
-        instrumentation.addMonitor(launcherActivityMonitor);
-        final ActivityMonitor freMonitor =
-                new ActivityMonitor(FirstRunActivity.class.getName(), null, false);
-        instrumentation.addMonitor(freMonitor);
-
-        final Context context = instrumentation.getTargetContext();
         Intent intent = new Intent(Intent.ACTION_VIEW, Uri.parse("http://test.com"));
-        intent.setPackage(context.getPackageName());
+        intent.setPackage(mContext.getPackageName());
         intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
-        context.startActivity(intent);
+        mContext.startActivity(intent);
 
-        final Activity chromeLauncherActivity = instrumentation.waitForMonitorWithTimeout(
-                launcherActivityMonitor, CriteriaHelper.DEFAULT_MAX_TIME_TO_POLL);
-        Assert.assertNotNull(chromeLauncherActivity);
+        Activity chromeLauncherActivity = waitForActivity(ChromeLauncherActivity.class);
 
         // Because the ChromeLauncherActivity notices that the FRE hasn't been run yet, it
         // redirects to it.
-        mActivity = instrumentation.waitForMonitorWithTimeout(
-                freMonitor, CriteriaHelper.DEFAULT_MAX_TIME_TO_POLL);
-        Assert.assertNotNull(mActivity);
+        waitForActivity(FirstRunActivity.class);
 
         // Once the user closes the FRE, the user should be kicked back into the
         // startup flow where they were interrupted.
         Assert.assertEquals(0, mTestObserver.abortFirstRunExperienceCallback.getCallCount());
-        mActivity.onBackPressed();
+        mLastActivity.onBackPressed();
         mTestObserver.abortFirstRunExperienceCallback.waitForCallback(
                 "FirstRunActivity didn't abort", 0);
 
-        CriteriaHelper.pollInstrumentationThread(() -> mActivity.isFinishing());
+        CriteriaHelper.pollInstrumentationThread(() -> mLastActivity.isFinishing());
 
         // ChromeLauncherActivity should finish if FRE was aborted.
         CriteriaHelper.pollInstrumentationThread(() -> chromeLauncherActivity.isFinishing());
@@ -159,34 +193,22 @@ public class FirstRunIntegrationTest {
         };
         LocaleManager.setInstanceForTest(mockManager);
 
-        final ActivityMonitor freMonitor =
-                new ActivityMonitor(FirstRunActivity.class.getName(), null, false);
-        Instrumentation instrumentation = InstrumentationRegistry.getInstrumentation();
-        instrumentation.addMonitor(freMonitor);
-
-        final Context context = instrumentation.getTargetContext();
         Intent intent = new Intent(Intent.ACTION_VIEW, Uri.parse("http://test.com"));
-        intent.setPackage(context.getPackageName());
+        intent.setPackage(mContext.getPackageName());
         intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
-        context.startActivity(intent);
+        mContext.startActivity(intent);
 
         // Because the AsyncInitializationActivity notices that the FRE hasn't been run yet, it
         // redirects to it.  Once the user closes the FRE, the user should be kicked back into the
         // startup flow where they were interrupted.
-        mActivity = instrumentation.waitForMonitorWithTimeout(
-                freMonitor, CriteriaHelper.DEFAULT_MAX_TIME_TO_POLL);
-        Assert.assertNotNull(mActivity);
-
-        ActivityMonitor activityMonitor =
-                new ActivityMonitor(ChromeTabbedActivity.class.getName(), null, false);
-        instrumentation.addMonitor(activityMonitor);
+        waitForActivity(FirstRunActivity.class);
 
         mTestObserver.flowIsKnownCallback.waitForCallback("Failed to finalize the flow", 0);
         Bundle freProperties = mTestObserver.freProperties;
         Assert.assertEquals(0, mTestObserver.updateCachedEngineCallback.getCallCount());
 
         // Accept the ToS.
-        clickButton(mActivity, R.id.terms_accept, "Failed to accept ToS");
+        clickButton(mLastActivity, R.id.terms_accept, "Failed to accept ToS");
         mTestObserver.jumpToPageCallback.waitForCallback(
                 "Failed to try moving to the next screen", 0);
         mTestObserver.acceptTermsOfServiceCallback.waitForCallback("Failed to accept the ToS", 0);
@@ -194,7 +216,7 @@ public class FirstRunIntegrationTest {
         // Acknowledge that Data Saver will be enabled.
         if (freProperties.getBoolean(FirstRunActivityBase.SHOW_DATA_REDUCTION_PAGE)) {
             int jumpCallCount = mTestObserver.jumpToPageCallback.getCallCount();
-            clickButton(mActivity, R.id.next_button, "Failed to skip data saver");
+            clickButton(mLastActivity, R.id.next_button, "Failed to skip data saver");
             mTestObserver.jumpToPageCallback.waitForCallback(
                     "Failed to try moving to next screen", jumpCallCount);
         }
@@ -208,7 +230,7 @@ public class FirstRunIntegrationTest {
                     freProperties.getBoolean(FirstRunActivityBase.SHOW_SEARCH_ENGINE_PAGE));
             int jumpCallCount = mTestObserver.jumpToPageCallback.getCallCount();
             DefaultSearchEngineDialogHelperUtils.clickOnFirstEngine(
-                    mActivity.findViewById(android.R.id.content));
+                    mLastActivity.findViewById(android.R.id.content));
 
             mTestObserver.jumpToPageCallback.waitForCallback(
                     "Failed to try moving to next screen", jumpCallCount);
@@ -217,7 +239,7 @@ public class FirstRunIntegrationTest {
         // Don't sign in the user.
         if (freProperties.getBoolean(FirstRunActivityBase.SHOW_SIGNIN_PAGE)) {
             int jumpCallCount = mTestObserver.jumpToPageCallback.getCallCount();
-            clickButton(mActivity, R.id.negative_button, "Failed to skip signing-in");
+            clickButton(mLastActivity, R.id.negative_button, "Failed to skip signing-in");
             mTestObserver.jumpToPageCallback.waitForCallback(
                     "Failed to try moving to next screen", jumpCallCount);
         }
@@ -227,9 +249,38 @@ public class FirstRunIntegrationTest {
         // processed by ChromeLauncherActivity.
         mTestObserver.updateCachedEngineCallback.waitForCallback(
                 "Failed to alert search widgets that an update is necessary", 0);
-        mActivity = instrumentation.waitForMonitorWithTimeout(
-                activityMonitor, CriteriaHelper.DEFAULT_MAX_TIME_TO_POLL);
-        Assert.assertNotNull(mActivity);
+        waitForActivity(ChromeTabbedActivity.class);
+    }
+
+    @Test
+    @MediumTest
+    public void testExitFirstRunWithPolicy() {
+        setHasAppRestrictionForMock();
+        FirstRunActivity.setEnableEnterpriseCCTForTest(true);
+        TosAndUmaFirstRunFragmentWithEnterpriseSupport.setBlockPolicyLoadingForTest(true);
+
+        Intent intent =
+                CustomTabsTestUtils.createMinimalCustomTabIntent(mContext, "https://test.com");
+        mContext.startActivity(intent);
+
+        FirstRunActivity freActivity = waitForActivity(FirstRunActivity.class);
+        CriteriaHelper.pollUiThread(
+                () -> freActivity.getSupportFragmentManager().getFragments().size() > 0);
+        TosAndUmaFirstRunFragmentWithEnterpriseSupport fragment =
+                (TosAndUmaFirstRunFragmentWithEnterpriseSupport) freActivity
+                        .getSupportFragmentManager()
+                        .getFragments()
+                        .get(0);
+
+        // Make sure native is initialized so that the subseuqent transition doesn't get blocked.
+        CriteriaHelper.pollUiThread((() -> freActivity.isNativeSideIsInitializedForTest()),
+                "native never initialized.");
+
+        // This policy cause the FRE to be skipped. It responds by relaunching the original intent,
+        // which should cause a {@link CustomTabActivity} because this was originally a CCT intent.
+        TestThreadUtils.runOnUiThreadBlocking(() -> fragment.onCctTosPolicyDetected(false));
+
+        waitForActivity(CustomTabActivity.class);
     }
 
     private void clickButton(final Activity activity, final int id, final String message) {