Fix serialization bug in ShoppingPersistedTabData

It was found that after deserialization, fields in
ShoppingPersistedTabData were set to their default values, despite the
deseralization being performed correctly. I believe the reason is fields
in the child can't be set in the parent constructor (although
interestingly the same thing didn't happen in CriticalPersistedTabData).
The problem can be solved in ShoppingPersistedTabData by deserializing
in the child - not the parent. I wrote an identical unit test for
CriticalPersistedTabData to guard against this popping up in the future.

Bug: 1160316
Change-Id: Id7d2d257448b975fd2780ae61a19d311892ecb7c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2598656
Commit-Queue: David Maunder <davidjm@chromium.org>
Reviewed-by: Yusuf Ozuysal <yusufo@chromium.org>
Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#839129}

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

@@ -217,6 +217,7 @@ public class CriticalPersistedTabDataTest {
         spyCriticalPersistedTabData.setUrl(new GURL("https://www.google.com"));
         Assert.assertTrue(spyCriticalPersistedTabData.shouldSave());
     }
+
     private CriticalPersistedTabData prepareCPTDShouldTabSave(
             boolean canGoBack, boolean canGoForward) {
         TabImpl tab = new MockTab(1, false);
@@ -225,4 +226,30 @@ public class CriticalPersistedTabDataTest {
         doReturn(canGoForward).when(spyTab).canGoForward();
         return spy(CriticalPersistedTabData.from(spyTab));
     }
+
+    @UiThreadTest
+    @SmallTest
+    @Test
+    public void testSerializationBug() throws InterruptedException {
+        Tab tab = mockTab(TAB_ID, false);
+        CriticalPersistedTabData criticalPersistedTabData = new CriticalPersistedTabData(tab, "",
+                "", PARENT_ID, ROOT_ID, TIMESTAMP, WEB_CONTENTS_STATE, CONTENT_STATE_VERSION,
+                OPENER_APP_ID, THEME_COLOR, LAUNCH_TYPE_AT_CREATION);
+        byte[] serialized = criticalPersistedTabData.serialize();
+        PersistedTabDataConfiguration config = PersistedTabDataConfiguration.get(
+                ShoppingPersistedTabData.class, tab.isIncognito());
+        CriticalPersistedTabData deserialized =
+                new CriticalPersistedTabData(tab, serialized, config.getStorage(), config.getId());
+        Assert.assertNotNull(deserialized);
+        Assert.assertEquals(PARENT_ID, deserialized.getParentId());
+        Assert.assertEquals(ROOT_ID, deserialized.getRootId());
+        Assert.assertEquals(TIMESTAMP, deserialized.getTimestampMillis());
+        Assert.assertEquals(CONTENT_STATE_VERSION, deserialized.getContentStateVersion());
+        Assert.assertEquals(OPENER_APP_ID, deserialized.getOpenerAppId());
+        Assert.assertEquals(THEME_COLOR, deserialized.getThemeColor());
+        Assert.assertEquals(LAUNCH_TYPE_AT_CREATION, deserialized.getTabLaunchTypeAtCreation());
+        Assert.assertArrayEquals(WEB_CONTENTS_STATE_BYTES,
+                CriticalPersistedTabData.getContentStateByteArray(
+                        deserialized.getWebContentsState().buffer()));
+    }
 }

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

@@ -560,6 +560,21 @@ public class ShoppingPersistedTabDataTest {
         acquireSemaphore(semaphore);
     }
 
+    @UiThreadTest
+    @SmallTest
+    @Test
+    public void testSerializationBug() {
+        Tab tab = createTabOnUiThread(TAB_ID, IS_INCOGNITO);
+        ShoppingPersistedTabData shoppingPersistedTabData = new ShoppingPersistedTabData(tab);
+        shoppingPersistedTabData.setPriceMicros(42_000_000L, null);
+        byte[] serialized = shoppingPersistedTabData.serialize();
+        PersistedTabDataConfiguration config = PersistedTabDataConfiguration.get(
+                ShoppingPersistedTabData.class, tab.isIncognito());
+        ShoppingPersistedTabData deserialized =
+                new ShoppingPersistedTabData(tab, serialized, config.getStorage(), config.getId());
+        Assert.assertEquals(42_000_000L, deserialized.getPriceMicros());
+    }
+
     private void verifyEndpointFetcherCalled(int numTimes) {
         verify(mEndpointFetcherJniMock, times(numTimes))
                 .nativeFetchChromeAPIKey(any(Profile.class), anyString(), anyString(), anyString(),

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

@@ -124,9 +124,11 @@ public class CriticalPersistedTabData extends PersistedTabData {
      * This constructor is public because that is needed for the reflection
      * used in PersistedTabData.java
      */
-    private CriticalPersistedTabData(
+    @VisibleForTesting
+    protected CriticalPersistedTabData(
             Tab tab, byte[] data, PersistedTabDataStorage storage, String persistedTabDataId) {
-        super(tab, data, storage, persistedTabDataId);
+        super(tab, storage, persistedTabDataId);
+        deserializeAndLog(data);
     }
 
     /**

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

@@ -29,7 +29,8 @@ public class MockPersistedTabData extends PersistedTabData {
     }
 
     private MockPersistedTabData(Tab tab, byte[] data, PersistedTabDataStorage storage, String id) {
-        super(tab, data, storage, id);
+        super(tab, storage, id);
+        deserializeAndLog(data);
     }
 
     /**

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

@@ -47,18 +47,6 @@ public abstract class PersistedTabData implements UserData {
     private Callback<Boolean> mTabSaveEnabledToggleCallback;
     private boolean mFirstSaveDone;
 
-    /**
-     * @param tab {@link Tab} {@link PersistedTabData} is being stored for
-     * @param data serialized {@link Tab} metadata
-     * @param persistedTabDataStorage storage for {@link PersistedTabData}
-     * @param persistedTabDataId identifier for {@link PersistedTabData} in storage
-     */
-    PersistedTabData(Tab tab, byte[] data, PersistedTabDataStorage persistedTabDataStorage,
-            String persistedTabDataId) {
-        this(tab, persistedTabDataStorage, persistedTabDataId);
-        deserializeAndLog(data);
-    }
-
     /**
      * @param tab {@link Tab} {@link PersistedTabData} is being stored for
      * @param persistedTabDataStorage storage for {@link PersistedTabData}
@@ -272,7 +260,7 @@ public abstract class PersistedTabData implements UserData {
      */
     abstract boolean deserialize(@Nullable byte[] bytes);
 
-    private void deserializeAndLog(@Nullable byte[] bytes) {
+    protected void deserializeAndLog(@Nullable byte[] bytes) {
         boolean success;
         try (TraceEvent e = TraceEvent.scoped("PersistedTabData.Deserialize")) {
             success = deserialize(bytes);

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

@@ -131,9 +131,11 @@ public class ShoppingPersistedTabData extends PersistedTabData {
         setupPersistence(tab);
     }
 
-    private ShoppingPersistedTabData(
+    @VisibleForTesting
+    protected ShoppingPersistedTabData(
             Tab tab, byte[] data, PersistedTabDataStorage storage, String persistedTabDataId) {
-        super(tab, data, storage, persistedTabDataId);
+        super(tab, storage, persistedTabDataId);
+        deserializeAndLog(data);
         setupPersistence(tab);
     }