Fix bookmark editing partner customization init issue.

If you were viewing partner bookmarks and left Chrome, and the
process is killed while in the background (triggering a locale
change forces this), then return to Chrome, the partner bookmarks
will be editable.

To solve this, change the partner bookmark reader pull the edit
state value instead of relying on push from an external source.

BUG=815276

Change-Id: I2fdc3f095be14c790a6a7eec79f79f79107acd67
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2129213
Commit-Queue: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#756164}

chrome/android/chrome_junit_test_java_sources.gni

@@ -174,6 +174,7 @@ chrome_junit_test_java_sources = [
   "junit/src/org/chromium/chrome/browser/omnibox/voice/AssistantVoiceSearchServiceUnitTest.java",
   "junit/src/org/chromium/chrome/browser/page_info/PermissionParamsListBuilderUnitTest.java",
   "junit/src/org/chromium/chrome/browser/partnerbookmarks/PartnerBookmarksFaviconThrottleTest.java",
+  "junit/src/org/chromium/chrome/browser/partnerbookmarks/PartnerBookmarksReaderTest.java",
   "junit/src/org/chromium/chrome/browser/password_manager/settings/DialogManagerTest.java",
   "junit/src/org/chromium/chrome/browser/password_manager/settings/EnsureAsyncPostingRule.java",
   "junit/src/org/chromium/chrome/browser/password_manager/settings/ExportWarningDialogFragmentTest.java",

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

@@ -209,11 +209,6 @@ public abstract class ChromeActivity<C extends ChromeActivityComponent>
      */
     static final int NO_TOOLBAR_LAYOUT = -1;
 
-    /**
-     * Timeout in ms for reading PartnerBrowserCustomizations provider.
-     */
-    private static final int PARTNER_BROWSER_CUSTOMIZATIONS_TIMEOUT_MS = 10000;
-
     private C mComponent;
 
     protected ObservableSupplierImpl<TabModelSelector> mTabModelSelectorSupplier =
@@ -1099,8 +1094,7 @@ public abstract class ChromeActivity<C extends ChromeActivityComponent>
 
         if (mPartnerBrowserRefreshNeeded) {
             mPartnerBrowserRefreshNeeded = false;
-            PartnerBrowserCustomizations.getInstance().initializeAsync(
-                    getApplicationContext(), PARTNER_BROWSER_CUSTOMIZATIONS_TIMEOUT_MS);
+            PartnerBrowserCustomizations.getInstance().initializeAsync(getApplicationContext());
             PartnerBrowserCustomizations.getInstance().setOnInitializeAsyncFinished(() -> {
                 if (PartnerBrowserCustomizations.isIncognitoDisabled()) {
                     terminateIncognitoSession();

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

@@ -68,12 +68,6 @@ public class LaunchIntentDispatcher implements IntentHandler.IntentHandlerDelega
 
     private static final String TAG = "ActivitiyDispatcher";
 
-    /**
-     * Timeout in ms for reading PartnerBrowserCustomizations provider. We do not trust third party
-     * provider by default.
-     */
-    private static final int PARTNER_BROWSER_CUSTOMIZATIONS_TIMEOUT_MS = 10000;
-
     private final Activity mActivity;
     private final Intent mIntent;
     private final boolean mIsCustomTabIntent;
@@ -151,7 +145,7 @@ public class LaunchIntentDispatcher implements IntentHandler.IntentHandlerDelega
         // We want to initialize early because when there are no tabs to restore, we should possibly
         // show homepage, which might require reading PartnerBrowserCustomizations provider.
         PartnerBrowserCustomizations.getInstance().initializeAsync(
-                mActivity.getApplicationContext(), PARTNER_BROWSER_CUSTOMIZATIONS_TIMEOUT_MS);
+                mActivity.getApplicationContext());
 
         int tabId = IntentUtils.safeGetIntExtra(
                 mIntent, IntentHandler.TabOpenType.BRING_TAB_TO_FRONT_STRING, Tab.INVALID_TAB_ID);

chrome/android/java/src/org/chromium/chrome/browser/partnerbookmarks/PartnerBookmarksReader.java

@@ -12,6 +12,7 @@ import org.chromium.base.annotations.NativeMethods;
 import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.base.task.AsyncTask;
 import org.chromium.chrome.browser.AppHooks;
+import org.chromium.chrome.browser.partnercustomizations.PartnerBrowserCustomizations;
 import org.chromium.ui.base.ViewUtils;
 
 import java.util.HashSet;
@@ -57,6 +58,7 @@ public class PartnerBookmarksReader {
     @GuardedBy("mProgressLock")
     private boolean mFaviconsFetchedFromServer;
     private boolean mFinishedReading;
+    private boolean mFinishedResolvingBrowserCustomizations;
 
     /**
      * Observer for listeners to receive updates when changes are made to the favicon cache.
@@ -90,12 +92,23 @@ public class PartnerBookmarksReader {
     /**
      * Creates the instance of the reader.
      * @param context A Context object.
+     * @param browserCustomizations Provides status of partner customizations.
      */
-    public PartnerBookmarksReader(Context context) {
+    public PartnerBookmarksReader(
+            Context context, PartnerBrowserCustomizations browserCustomizations) {
         mContext = context;
         mNativePartnerBookmarksReader =
                 PartnerBookmarksReaderJni.get().init(PartnerBookmarksReader.this);
-        initializeAndDisableEditingIfNecessary();
+        if (!browserCustomizations.isInitialized()) {
+            browserCustomizations.initializeAsync(context);
+        }
+        browserCustomizations.setOnInitializeAsyncFinished(() -> {
+            if (browserCustomizations.isBookmarksEditingDisabled()) {
+                PartnerBookmarksReaderJni.get().disablePartnerBookmarksEditing();
+            }
+            mFinishedResolvingBrowserCustomizations = true;
+            maybeMarkCreationComplete();
+        });
     }
 
     /**
@@ -159,9 +172,7 @@ public class PartnerBookmarksReader {
                     }
                     mFaviconThrottle.onFaviconFetched(url, result);
                     --mNumFaviconsInProgress;
-                    if (mNumFaviconsInProgress == 0 && mFinishedReading) {
-                        shutDown();
-                    }
+                    if (canShutdown()) shutDown();
                 }
             }
 
@@ -183,16 +194,25 @@ public class PartnerBookmarksReader {
      * down the bookmark reader.
      */
     protected void onBookmarksRead() {
-        PartnerBookmarksReaderJni.get().partnerBookmarksCreationComplete(
-                mNativePartnerBookmarksReader, PartnerBookmarksReader.this);
         mFinishedReading = true;
+        maybeMarkCreationComplete();
         synchronized (mProgressLock) {
-            if (mNumFaviconsInProgress == 0) {
-                shutDown();
-            }
+            if (canShutdown()) shutDown();
         }
     }
 
+    private void maybeMarkCreationComplete() {
+        if (!mFinishedReading || !mFinishedResolvingBrowserCustomizations) return;
+        PartnerBookmarksReaderJni.get().partnerBookmarksCreationComplete(
+                mNativePartnerBookmarksReader, PartnerBookmarksReader.this);
+    }
+
+    @GuardedBy("mProgressLock")
+    private boolean canShutdown() {
+        return mNumFaviconsInProgress == 0 && mFinishedReading
+                && mFinishedResolvingBrowserCustomizations;
+    }
+
     /**
      * Notifies the reader is complete, refreshes the partner bookmarks if necessary, and kills the
      * native object
@@ -357,19 +377,6 @@ public class PartnerBookmarksReader {
         }
     }
 
-    /**
-     * Disables partner bookmarks editing.
-     */
-    public static void disablePartnerBookmarksEditing() {
-        sForceDisableEditing = true;
-        if (sInitialized) PartnerBookmarksReaderJni.get().disablePartnerBookmarksEditing();
-    }
-
-    private static void initializeAndDisableEditingIfNecessary() {
-        sInitialized = true;
-        if (sForceDisableEditing) disablePartnerBookmarksEditing();
-    }
-
     @NativeMethods
     interface Natives {
         long init(PartnerBookmarksReader caller);

chrome/android/java/src/org/chromium/chrome/browser/partnerbookmarks/PartnerBookmarksShim.java

@@ -8,6 +8,7 @@ import android.content.Context;
 import android.content.pm.ApplicationInfo;
 
 import org.chromium.chrome.browser.ChromeVersionInfo;
+import org.chromium.chrome.browser.partnercustomizations.PartnerBrowserCustomizations;
 
 /**
  * The Java counterpart for the C++ partner bookmarks shim.
@@ -30,7 +31,8 @@ public class PartnerBookmarksShim {
         if (sIsReadingAttempted) return;
         sIsReadingAttempted = true;
 
-        PartnerBookmarksReader reader = new PartnerBookmarksReader(context);
+        PartnerBookmarksReader reader =
+                new PartnerBookmarksReader(context, PartnerBrowserCustomizations.getInstance());
 
         boolean systemOrPreStable =
                 (context.getApplicationInfo().flags & ApplicationInfo.FLAG_SYSTEM) == 1

chrome/android/java/src/org/chromium/chrome/browser/partnercustomizations/PartnerBrowserCustomizations.java

@@ -24,7 +24,6 @@ import org.chromium.chrome.browser.AppHooks;
 import org.chromium.chrome.browser.ChromeVersionInfo;
 import org.chromium.chrome.browser.flags.ChromeSwitches;
 import org.chromium.chrome.browser.ntp.NewTabPage;
-import org.chromium.chrome.browser.partnerbookmarks.PartnerBookmarksReader;
 import org.chromium.components.embedder_support.util.UrlConstants;
 import org.chromium.components.embedder_support.util.UrlUtilities;
 import org.chromium.content_public.browser.UiThreadTaskTraits;
@@ -39,6 +38,9 @@ public class PartnerBrowserCustomizations {
     private static final String TAG = "PartnerCustomize";
     private static final String PROVIDER_AUTHORITY = "com.android.partnerbrowsercustomizations";
 
+    /** Default timeout in ms for reading PartnerBrowserCustomizations provider. */
+    private static final int DEFAULT_TIMEOUT_MS = 10_000;
+
     private static final int HOMEPAGE_URL_MAX_LENGTH = 1000;
     // Private homepage structure.
     @VisibleForTesting
@@ -211,8 +213,7 @@ public class PartnerBrowserCustomizations {
     /**
      * @return Whether partner bookmarks editing is disabled by the partner.
      */
-    @VisibleForTesting
-    boolean isBookmarksEditingDisabled() {
+    public boolean isBookmarksEditingDisabled() {
         return mBookmarksEditingDisabled;
     }
 
@@ -251,18 +252,27 @@ public class PartnerBrowserCustomizations {
                 .build();
     }
 
+    /**
+     * Constructs an async task that reads PartnerBrowserCustomization provider.
+     *
+     * @param context   The current application context.
+     */
+    public void initializeAsync(final Context context) {
+        initializeAsync(context, DEFAULT_TIMEOUT_MS);
+    }
+
     /**
      * Constructs an async task that reads PartnerBrowserCustomization provider.
      *
      * @param context   The current application context.
      * @param timeoutMs If initializing takes more than this time, cancels it. The unit is ms.
      */
-    public void initializeAsync(final Context context, long timeoutMs) {
+    @VisibleForTesting
+    void initializeAsync(final Context context, long timeoutMs) {
         mIsInitialized = false;
         Provider provider = AppHooks.get().getCustomizationProvider();
         // Setup an initializing async task.
         final AsyncTask<Void> initializeAsyncTask = new AsyncTask<Void>() {
-            private boolean mDisablePartnerBookmarksShim;
             private boolean mHomepageUriChanged;
 
             private void refreshHomepage() {
@@ -290,13 +300,7 @@ public class PartnerBrowserCustomizations {
 
             private void refreshBookmarksEditingDisabled() {
                 try {
-                    boolean disabled = provider.isBookmarksEditingDisabled();
-                    // Only need to disable it once.
-                    if (disabled != mBookmarksEditingDisabled) {
-                        assert disabled;
-                        mDisablePartnerBookmarksShim = true;
-                    }
-                    mBookmarksEditingDisabled = disabled;
+                    mBookmarksEditingDisabled = provider.isBookmarksEditingDisabled();
                 } catch (Exception e) {
                     Log.w(TAG, "Partner disable bookmarks editing read failed : ", e);
                 }
@@ -355,11 +359,6 @@ public class PartnerBrowserCustomizations {
                 if (mHomepageUriChanged && mListener != null) {
                     mListener.onHomepageUpdate();
                 }
-
-                // Disable partner bookmarks editing if necessary.
-                if (mDisablePartnerBookmarksShim) {
-                    PartnerBookmarksReader.disablePartnerBookmarksEditing();
-                }
             }
         };
 

chrome/android/junit/src/org/chromium/chrome/browser/partnerbookmarks/PartnerBookmarksReaderTest.java

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.partnerbookmarks;
+
+import android.content.Context;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Captor;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.MockitoAnnotations;
+import org.robolectric.annotation.Config;
+
+import org.chromium.base.test.BaseRobolectricTestRunner;
+import org.chromium.base.test.util.JniMocker;
+import org.chromium.chrome.browser.partnercustomizations.PartnerBrowserCustomizations;
+
+/**
+ * Unit tests for PartnerBookmarksReader.
+ */
+@RunWith(BaseRobolectricTestRunner.class)
+@Config(manifest = Config.NONE)
+public class PartnerBookmarksReaderTest {
+    @Rule
+    public JniMocker mocker = new JniMocker();
+
+    @Mock
+    Context mContextMock;
+
+    @Mock
+    PartnerBookmarksReader.Natives mJniMock;
+
+    @Mock
+    PartnerBrowserCustomizations mBrowserCustomizations;
+
+    @Captor
+    ArgumentCaptor<Runnable> mBrowserCustomizationsInitCallback;
+
+    @Before
+    public void setUp() {
+        MockitoAnnotations.initMocks(this);
+        mocker.mock(PartnerBookmarksReaderJni.TEST_HOOKS, mJniMock);
+        Mockito.doNothing()
+                .when(mBrowserCustomizations)
+                .setOnInitializeAsyncFinished(mBrowserCustomizationsInitCallback.capture());
+    }
+
+    @Test
+    public void partnerBrowserCustomizations_BookmarkEditingDisabled_AlreadyInitialized() {
+        Mockito.when(mBrowserCustomizations.isInitialized()).thenReturn(true);
+
+        @SuppressWarnings("unused")
+        PartnerBookmarksReader reader =
+                new PartnerBookmarksReader(mContextMock, mBrowserCustomizations);
+
+        Mockito.verify(mBrowserCustomizations, Mockito.never()).initializeAsync(mContextMock);
+
+        Mockito.when(mBrowserCustomizations.isBookmarksEditingDisabled()).thenReturn(true);
+        mBrowserCustomizationsInitCallback.getValue().run();
+        Mockito.verify(mJniMock).disablePartnerBookmarksEditing();
+    }
+
+    @Test
+    public void partnerBrowserCustomizations_BookmarkEditingDisabled_NotAlreadyInitialized() {
+        Mockito.when(mBrowserCustomizations.isInitialized()).thenReturn(false);
+
+        @SuppressWarnings("unused")
+        PartnerBookmarksReader reader =
+                new PartnerBookmarksReader(mContextMock, mBrowserCustomizations);
+
+        Mockito.verify(mBrowserCustomizations).initializeAsync(mContextMock);
+
+        Mockito.when(mBrowserCustomizations.isBookmarksEditingDisabled()).thenReturn(true);
+        mBrowserCustomizationsInitCallback.getValue().run();
+        Mockito.verify(mJniMock).disablePartnerBookmarksEditing();
+    }
+
+    @Test
+    public void partnerBrowserCustomizations_BookmarkEditingAllowed_AlreadyInitialized() {
+        Mockito.when(mBrowserCustomizations.isInitialized()).thenReturn(true);
+
+        @SuppressWarnings("unused")
+        PartnerBookmarksReader reader =
+                new PartnerBookmarksReader(mContextMock, mBrowserCustomizations);
+
+        Mockito.verify(mBrowserCustomizations, Mockito.never()).initializeAsync(mContextMock);
+
+        Mockito.when(mBrowserCustomizations.isBookmarksEditingDisabled()).thenReturn(false);
+        mBrowserCustomizationsInitCallback.getValue().run();
+        Mockito.verify(mJniMock, Mockito.never()).disablePartnerBookmarksEditing();
+    }
+
+    @Test
+    public void partnerBrowserCustomizations_BookmarkEditingAllowed_NotAlreadyInitialized() {
+        Mockito.when(mBrowserCustomizations.isInitialized()).thenReturn(false);
+
+        @SuppressWarnings("unused")
+        PartnerBookmarksReader reader =
+                new PartnerBookmarksReader(mContextMock, mBrowserCustomizations);
+
+        Mockito.verify(mBrowserCustomizations).initializeAsync(mContextMock);
+
+        Mockito.when(mBrowserCustomizations.isBookmarksEditingDisabled()).thenReturn(false);
+        mBrowserCustomizationsInitCallback.getValue().run();
+        Mockito.verify(mJniMock, Mockito.never()).disablePartnerBookmarksEditing();
+    }
+
+    @Test
+    public void partnerBookmarksCreationComplete_NotCalledWithoutBrowserCustomizations() {
+        Mockito.when(mBrowserCustomizations.isInitialized()).thenReturn(true);
+
+        @SuppressWarnings("unused")
+        PartnerBookmarksReader reader =
+                new PartnerBookmarksReader(mContextMock, mBrowserCustomizations);
+        reader.onBookmarksRead();
+
+        Mockito.verify(mJniMock, Mockito.never())
+                .partnerBookmarksCreationComplete(Mockito.anyLong(), Mockito.any());
+    }
+
+    @Test
+    public void partnerBookmarksCreationComplete_NotCalledWithoutBookmarksRead() {
+        Mockito.when(mBrowserCustomizations.isInitialized()).thenReturn(true);
+
+        @SuppressWarnings("unused")
+        PartnerBookmarksReader reader =
+                new PartnerBookmarksReader(mContextMock, mBrowserCustomizations);
+        Mockito.when(mBrowserCustomizations.isBookmarksEditingDisabled()).thenReturn(false);
+        mBrowserCustomizationsInitCallback.getValue().run();
+
+        Mockito.verify(mJniMock, Mockito.never())
+                .partnerBookmarksCreationComplete(Mockito.anyLong(), Mockito.any());
+    }
+
+    @Test
+    public void partnerBookmarksCreationComplete_Called_WithCustomizationsFirstThenBookmarks() {
+        Mockito.when(mBrowserCustomizations.isInitialized()).thenReturn(true);
+
+        @SuppressWarnings("unused")
+        PartnerBookmarksReader reader =
+                new PartnerBookmarksReader(mContextMock, mBrowserCustomizations);
+        Mockito.when(mBrowserCustomizations.isBookmarksEditingDisabled()).thenReturn(false);
+        mBrowserCustomizationsInitCallback.getValue().run();
+        reader.onBookmarksRead();
+
+        Mockito.verify(mJniMock).partnerBookmarksCreationComplete(
+                Mockito.anyLong(), Mockito.eq(reader));
+    }
+
+    @Test
+    public void partnerBookmarksCreationComplete_Called_WithBookmarksFirstThenCustomizations() {
+        Mockito.when(mBrowserCustomizations.isInitialized()).thenReturn(true);
+
+        @SuppressWarnings("unused")
+        PartnerBookmarksReader reader =
+                new PartnerBookmarksReader(mContextMock, mBrowserCustomizations);
+        Mockito.when(mBrowserCustomizations.isBookmarksEditingDisabled()).thenReturn(false);
+        reader.onBookmarksRead();
+        mBrowserCustomizationsInitCallback.getValue().run();
+
+        Mockito.verify(mJniMock).partnerBookmarksCreationComplete(
+                Mockito.anyLong(), Mockito.eq(reader));
+    }
+}