Fix flaky CriticalPersistedTabData tests

I believe the problem is we are not synchronizing the
delete and save operations - we try to verify one
is done without waiting for the operation to complete.
I solved this by adding in some callbacks

Bug: 1101760
Change-Id: I851e9488df971728738b47292de96710d00a4bc3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2300811Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Commit-Queue: David Maunder <davidjm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#795660}

chrome/android/javatests/src/org/chromium/chrome/browser/tab/state/CriticalPersistedTabDataTest.java

@@ -16,7 +16,6 @@ import org.mockito.MockitoAnnotations;
 import org.chromium.base.Callback;
 import org.chromium.base.ThreadUtils;
 import org.chromium.base.test.BaseJUnit4ClassRunner;
-import org.chromium.base.test.util.DisabledTest;
 import org.chromium.chrome.browser.tab.MockTab;
 import org.chromium.chrome.browser.tab.Tab;
 import org.chromium.chrome.browser.tab.WebContentsState;
@@ -52,6 +51,8 @@ public class CriticalPersistedTabDataTest {
 
     private CriticalPersistedTabData mCriticalPersistedTabData;
 
+    private MockPersistedTabDataStorage mStorage;
+
     private static Tab mockTab(int id, boolean isEncrypted) {
         return new MockTab(id, isEncrypted);
     }
@@ -59,18 +60,19 @@ public class CriticalPersistedTabDataTest {
     @Before
     public void setUp() throws Exception {
         MockitoAnnotations.initMocks(this);
+        PersistedTabDataConfiguration.setUseTestConfig(true);
+        mStorage =
+                (MockPersistedTabDataStorage) PersistedTabDataConfiguration.getTestConfig().storage;
     }
 
     @SmallTest
     @Test
-    @DisabledTest(message = "https://crbug.com/1101760")
     public void testNonEncryptedSaveRestore() throws InterruptedException {
         testSaveRestoreDelete(false);
     }
 
     @SmallTest
     @Test
-    @DisabledTest(message = "https://crbug.com/1101760")
     public void testEncryptedSaveRestoreDelete() throws InterruptedException {
         testSaveRestoreDelete(true);
     }
@@ -84,14 +86,14 @@ public class CriticalPersistedTabDataTest {
                 semaphore.release();
             }
         };
+        final Semaphore saveSemaphore = new Semaphore(0);
         ThreadUtils.runOnUiThreadBlocking(() -> {
-            PersistedTabDataConfiguration config =
-                    PersistedTabDataConfiguration.get(CriticalPersistedTabData.class, isEncrypted);
-            CriticalPersistedTabData criticalPersistedTabData =
-                    new CriticalPersistedTabData(mockTab(TAB_ID, isEncrypted), PARENT_ID, ROOT_ID,
-                            TIMESTAMP, WEB_CONTENTS_STATE, CONTENT_STATE_VERSION, OPENER_APP_ID,
-                            THEME_COLOR, LAUNCH_TYPE_AT_CREATION, config.storage, config.id);
-            criticalPersistedTabData.save();
+            CriticalPersistedTabData criticalPersistedTabData = new CriticalPersistedTabData(
+                    mockTab(TAB_ID, isEncrypted), PARENT_ID, ROOT_ID, TIMESTAMP, WEB_CONTENTS_STATE,
+                    CONTENT_STATE_VERSION, OPENER_APP_ID, THEME_COLOR, LAUNCH_TYPE_AT_CREATION);
+            mStorage.setSemaphore(saveSemaphore);
+            criticalPersistedTabData.saveForTesting();
+            acquireSemaphore(saveSemaphore);
             CriticalPersistedTabData.from(mockTab(TAB_ID, isEncrypted), callback);
         });
         semaphore.acquire();
@@ -108,8 +110,11 @@ public class CriticalPersistedTabDataTest {
         Assert.assertArrayEquals(mCriticalPersistedTabData.getWebContentsState().buffer().array(),
                 WEB_CONTENTS_STATE_BYTES);
 
+        Semaphore deleteSemaphore = new Semaphore(0);
         ThreadUtils.runOnUiThreadBlocking(() -> {
+            mStorage.setSemaphore(deleteSemaphore);
             mCriticalPersistedTabData.delete();
+            acquireSemaphore(deleteSemaphore);
             CriticalPersistedTabData.from(mockTab(TAB_ID, isEncrypted), callback);
         });
         semaphore.acquire();
@@ -122,4 +127,13 @@ public class CriticalPersistedTabDataTest {
         //  - restore() multiple times
         //  - delete() multiple times
     }
+
+    private static void acquireSemaphore(Semaphore semaphore) {
+        try {
+            semaphore.acquire();
+        } catch (InterruptedException e) {
+            // Throw Runtime exception to make catching InterruptedException unnecessary
+            throw new RuntimeException(e);
+        }
+    }
 }

chrome/browser/tab/BUILD.gn

@@ -28,6 +28,7 @@ android_library("java") {
     "java/src/org/chromium/chrome/browser/tab/state/EncryptedFilePersistedTabDataStorage.java",
     "java/src/org/chromium/chrome/browser/tab/state/FilePersistedTabDataStorage.java",
     "java/src/org/chromium/chrome/browser/tab/state/MockPersistedTabData.java",
+    "java/src/org/chromium/chrome/browser/tab/state/MockPersistedTabDataStorage.java",
     "java/src/org/chromium/chrome/browser/tab/state/PersistedTabData.java",
     "java/src/org/chromium/chrome/browser/tab/state/PersistedTabDataConfiguration.java",
     "java/src/org/chromium/chrome/browser/tab/state/PersistedTabDataFactory.java",

chrome/browser/tab/java/src/org/chromium/chrome/browser/tab/state/CriticalPersistedTabData.java

@@ -75,9 +75,8 @@ public class CriticalPersistedTabData extends PersistedTabData {
     @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
     CriticalPersistedTabData(Tab tab, int parentId, int rootId, long timestampMillis,
             WebContentsState webContentsState, int contentStateVersion, String openerAppId,
-            int themeColor, int launchTypeAtCreation,
-            PersistedTabDataStorage persistedTabDataStorage, String persistedTabDataId) {
-        super(tab, persistedTabDataStorage, persistedTabDataId);
+            int themeColor, int launchTypeAtCreation) {
+        this(tab);
         mParentId = parentId;
         mRootId = rootId;
         mTimestampMillis = timestampMillis;
@@ -136,16 +135,13 @@ public class CriticalPersistedTabData extends PersistedTabData {
 
     @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
     public static CriticalPersistedTabData build(Tab tab) {
-        PersistedTabDataConfiguration config = PersistedTabDataConfiguration.get(
-                CriticalPersistedTabData.class, tab.isIncognito());
         // CriticalPersistedTabData is initialized with default values
         CriticalPersistedTabData criticalPersistedTabData =
                 new CriticalPersistedTabData(tab, Tab.INVALID_TAB_ID, tab.getId(),
                         INVALID_TIMESTAMP, null, -1, "", UNSPECIFIED_THEME_COLOR,
                         tab.getLaunchTypeAtInitialTabCreation() == null
                                 ? TabLaunchType.FROM_LINK
-                                : tab.getLaunchTypeAtInitialTabCreation(),
-                        config.storage, config.id);
+                                : tab.getLaunchTypeAtInitialTabCreation());
         return criticalPersistedTabData;
     }
 
@@ -268,7 +264,7 @@ public class CriticalPersistedTabData extends PersistedTabData {
                 .setParentId(mParentId)
                 .setRootId(mRootId)
                 .setTimestampMillis(mTimestampMillis)
-                .setWebContentsStateBytes(ByteString.copyFrom(mWebContentsState.buffer()))
+                .setWebContentsStateBytes(ByteString.copyFrom(mWebContentsState.buffer().array()))
                 .setContentStateVersion(mContentStateVersion)
                 .setOpenerAppId(mOpenerAppId)
                 .setThemeColor(mThemeColor)
@@ -277,6 +273,7 @@ public class CriticalPersistedTabData extends PersistedTabData {
                 .toByteArray();
     }
 
+    // TODO(crbug.com/1113814) remove save() override
     @Override
     public void save() {
         mTab.setIsTabStateDirty(true);
@@ -285,6 +282,17 @@ public class CriticalPersistedTabData extends PersistedTabData {
         // without saving (i.e. as a regular UserData object).
     }
 
+    /**
+     * Used in tests when we need to save a CriticalPersistedTabData object.
+     * Ultimately will be deprecated by this.save() - for now this.save()
+     * is not saving while fields are being migrated to CriticalPersistedTabData
+     * TODO(crbug.com/1113813) Remove saveForTesting()
+     */
+    @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
+    protected void saveForTesting() {
+        super.save();
+    }
+
     @Override
     public void destroy() {}
 

chrome/browser/tab/java/src/org/chromium/chrome/browser/tab/state/MockPersistedTabDataStorage.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.tab.state;
+
+import org.chromium.base.Callback;
+import org.chromium.base.task.PostTask;
+import org.chromium.content_public.browser.UiThreadTaskTraits;
+
+import java.util.HashMap;
+import java.util.Locale;
+import java.util.Map;
+import java.util.concurrent.Semaphore;
+
+/**
+ * Mock implementation of {@link PersistedTabDataStorage} for tests
+ */
+public class MockPersistedTabDataStorage implements PersistedTabDataStorage {
+    private Semaphore mSemaphore;
+    private final Map<String, byte[]> mStorage = new HashMap<>();
+
+    @Override
+    public void save(int tabId, String tabDataId, byte[] data) {
+        mStorage.put(getKey(tabId), data);
+        if (mSemaphore != null) {
+            mSemaphore.release();
+        }
+    }
+
+    @Override
+    public void restore(int tabId, String tabDataId, Callback<byte[]> callback) {
+        PostTask.runOrPostTask(UiThreadTaskTraits.DEFAULT,
+                () -> { callback.onResult(mStorage.get(getKey(tabId))); });
+        if (mSemaphore != null) {
+            mSemaphore.release();
+        }
+    }
+
+    @Override
+    public void delete(int tabId, String tabDataId) {
+        mStorage.remove(getKey(tabId));
+        if (mSemaphore != null) {
+            mSemaphore.release();
+        }
+    }
+
+    @Override
+    public String getUmaTag() {
+        return "MPTDS";
+    }
+
+    private static String getKey(int tabId) {
+        return String.format(Locale.US, "%d", tabId);
+    }
+
+    public void setSemaphore(Semaphore semaphore) {
+        mSemaphore = semaphore;
+    }
+}

chrome/browser/tab/java/src/org/chromium/chrome/browser/tab/state/PersistedTabDataConfiguration.java

@@ -4,6 +4,8 @@
 
 package org.chromium.chrome.browser.tab.state;
 
+import androidx.annotation.VisibleForTesting;
+
 import java.util.HashMap;
 import java.util.Map;
 
@@ -17,13 +19,17 @@ public enum PersistedTabDataConfiguration {
     CRITICAL_PERSISTED_TAB_DATA("CPTD", new FilePersistedTabDataStorage()),
     ENCRYPTED_CRITICAL_PERSISTED_TAB_DATA("ECPTD", new EncryptedFilePersistedTabDataStorage()),
     MOCK_PERSISTED_TAB_DATA("MPTD", new FilePersistedTabDataStorage()),
-    ENCRYPTED_MOCK_PERSISTED_TAB_DATA("EMPTD", new EncryptedFilePersistedTabDataStorage());
+    ENCRYPTED_MOCK_PERSISTED_TAB_DATA("EMPTD", new EncryptedFilePersistedTabDataStorage()),
+    // TODO(crbug.com/1113828) investigate separating test from prod test implementations
+    TEST_CONFIG("TC", new MockPersistedTabDataStorage());
 
     private static final Map<Class<? extends PersistedTabData>, PersistedTabDataConfiguration>
             sLookup = new HashMap<>();
     private static final Map<Class<? extends PersistedTabData>, PersistedTabDataConfiguration>
             sEncryptedLookup = new HashMap<>();
 
+    private static boolean sUseTestConfig;
+
     static {
         // TODO(crbug.com/1060187) remove static initializer and initialization lazy
         sLookup.put(CriticalPersistedTabData.class, CRITICAL_PERSISTED_TAB_DATA);
@@ -49,9 +55,22 @@ public enum PersistedTabDataConfiguration {
      */
     public static PersistedTabDataConfiguration get(
             Class<? extends PersistedTabData> clazz, boolean isEncrypted) {
+        if (sUseTestConfig) {
+            return TEST_CONFIG;
+        }
         if (isEncrypted) {
             return sEncryptedLookup.get(clazz);
         }
         return sLookup.get(clazz);
     }
+
+    @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
+    protected static void setUseTestConfig(boolean useTestConfig) {
+        sUseTestConfig = useTestConfig;
+    }
+
+    @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
+    protected static PersistedTabDataConfiguration getTestConfig() {
+        return TEST_CONFIG;
+    }
 }