Creating contents state byte array before opening stream

Changed TabState.saveState to be able to safely write
mapped byte buffers by opening FileOutputStream after
retrieving buffer contents. Removed unnecessary call to
FileChannel.write which is not guaranteed to write all
bytes.

BUG=626815

Review-Url: https://codereview.chromium.org/2138503002
Cr-Commit-Position: refs/heads/master@{#405299}

AUTHORS

@@ -51,6 +51,7 @@ Ancil George <ancilgeorge@samsung.com>
 Andrei Parvu <andrei.prv@gmail.com>
 Andrei Parvu <parvu@adobe.com>
 Andrew Brampton <me@bramp.net>
+Andrew Hung <andrhung@amazon.com>
 Andrew Tulloch <andrew@tullo.ch>
 Anish Patankar <anish.p@samsung.com>
 Ankit Kumar <ankit2.kumar@samsung.com>

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

@@ -286,54 +286,61 @@ public class TabState {
 
     /**
      * Writes the TabState to disk. This method may be called on either the UI or background thread.
-     * @param output Stream to write the tab's state to.
+     * @param file File to write the tab's state to.
      * @param state State object obtained from from {@link Tab#getState()}.
      * @param encrypted Whether or not the TabState should be encrypted.
      */
-    public static void saveState(FileOutputStream output, TabState state, boolean encrypted)
-            throws IOException {
+    public static void saveState(File file, TabState state, boolean encrypted) {
         if (state == null || state.contentsState == null) {
             return;
         }
 
-        DataOutputStream stream;
-        if (encrypted) {
-            Cipher cipher = CipherFactory.getInstance().getCipher(Cipher.ENCRYPT_MODE);
-            if (cipher != null) {
-                stream = new DataOutputStream(new CipherOutputStream(output, cipher));
-            } else {
-                // If cipher is null, getRandomBytes failed, which means encryption is meaningless.
-                // Therefore, do not save anything. This will cause users to lose Incognito state in
-                // certain cases. That is annoying, but is better than failing to provide the
-                // guarantee of Incognito Mode.
-                return;
-            }
-        } else {
-            stream = new DataOutputStream(output);
-        }
+        // Create the byte array from contentsState before opening the FileOutputStream, in case
+        // contentsState.buffer is an instance of MappedByteBuffer that is mapped to
+        // the tab state file.
+        state.contentsState.buffer().rewind();
+        byte[] contentsStateBytes = new byte[state.contentsState.buffer().remaining()];
+        state.contentsState.buffer().get(contentsStateBytes);
 
+        DataOutputStream dataOutputStream = null;
+        FileOutputStream fileOutputStream = null;
         try {
+            fileOutputStream = new FileOutputStream(file);
+
             if (encrypted) {
-                stream.writeLong(KEY_CHECKER);
+                Cipher cipher = CipherFactory.getInstance().getCipher(Cipher.ENCRYPT_MODE);
+                if (cipher != null) {
+                    dataOutputStream = new DataOutputStream(new CipherOutputStream(
+                            fileOutputStream, cipher));
+                } else {
+                    // If cipher is null, getRandomBytes failed, which means encryption is
+                    // meaningless. Therefore, do not save anything. This will cause users
+                    // to lose Incognito state in certain cases. That is annoying, but is
+                    // better than failing to provide the guarantee of Incognito Mode.
+                    return;
+                }
+            } else {
+                dataOutputStream = new DataOutputStream(fileOutputStream);
             }
-            stream.writeLong(state.timestampMillis);
-            state.contentsState.buffer().rewind();
-            stream.writeInt(state.contentsState.buffer().remaining());
             if (encrypted) {
-                byte[] bytes = new byte[state.contentsState.buffer().remaining()];
-                state.contentsState.buffer().get(bytes);
-                stream.write(bytes);
-            } else {
-                output.getChannel().write(state.contentsState.buffer());
+                dataOutputStream.writeLong(KEY_CHECKER);
             }
-            stream.writeInt(state.parentId);
-            stream.writeUTF(state.openerAppId != null ? state.openerAppId : "");
-            stream.writeInt(state.contentsState.version());
-            stream.writeLong(state.syncId);
-            stream.writeBoolean(state.shouldPreserve);
-            stream.writeInt(state.themeColor);
+            dataOutputStream.writeLong(state.timestampMillis);
+            dataOutputStream.writeInt(contentsStateBytes.length);
+            dataOutputStream.write(contentsStateBytes);
+            dataOutputStream.writeInt(state.parentId);
+            dataOutputStream.writeUTF(state.openerAppId != null ? state.openerAppId : "");
+            dataOutputStream.writeInt(state.contentsState.version());
+            dataOutputStream.writeLong(state.syncId);
+            dataOutputStream.writeBoolean(state.shouldPreserve);
+            dataOutputStream.writeInt(state.themeColor);
+        } catch (FileNotFoundException e) {
+            Log.w(TAG, "FileNotFoundException while attempting to save TabState.");
+        } catch (IOException e) {
+            Log.w(TAG, "IOException while attempting to save TabState.");
         } finally {
-            StreamUtil.closeQuietly(stream);
+            StreamUtil.closeQuietly(dataOutputStream);
+            StreamUtil.closeQuietly(fileOutputStream);
         }
     }
 
@@ -467,4 +474,4 @@ public class TabState {
     private static native void nativeFreeWebContentsStateBuffer(ByteBuffer buffer);
 
     private static native void nativeCreateHistoricalTab(ByteBuffer state, int savedStateVersion);
-}
+}
\ No newline at end of file

chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java

@@ -341,20 +341,14 @@ public class TabPersistentStore extends TabPersister {
             for (Tab tab : mTabsToSave) {
                 int id = tab.getId();
                 boolean incognito = tab.isIncognito();
-                FileOutputStream stream = null;
                 try {
                     TabState state = tab.getState();
                     if (state != null) {
-                        stream = openTabStateOutputStream(id, incognito);
-                        TabState.saveState(stream, state, incognito);
+                        TabState.saveState(getTabStateFile(id, incognito), state, incognito);
                     }
-                } catch (IOException e) {
-                    Log.w(TAG, "Error while saving tabs state; will attempt to continue...", e);
                 } catch (OutOfMemoryError e) {
                     Log.w(TAG, "Out of memory error while attempting to save tab state.  Erasing.");
                     deleteTabState(id, incognito);
-                } finally {
-                    StreamUtil.closeQuietly(stream);
                 }
             }
             mTabsToSave.clear();

chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersister.java

@@ -6,13 +6,9 @@ package org.chromium.chrome.browser.tabmodel;
 
 import android.util.Log;
 
-import org.chromium.base.StreamUtil;
 import org.chromium.chrome.browser.TabState;
 
 import java.io.File;
-import java.io.FileNotFoundException;
-import java.io.FileOutputStream;
-import java.io.IOException;
 
 /**
  * Interacts with the file system to persist Tab and TabModel data.
@@ -43,20 +39,12 @@ public abstract class TabPersister {
     public boolean saveTabState(int tabId, boolean encrypted, TabState state) {
         if (state == null) return false;
 
-        FileOutputStream stream = null;
         try {
-            stream = openTabStateOutputStream(tabId, encrypted);
-            TabState.saveState(stream, state, encrypted);
+            TabState.saveState(getTabStateFile(tabId, encrypted), state, encrypted);
             return true;
-        } catch (FileNotFoundException exception) {
-            Log.w(TAG, "FileNotFoundException while attempt to TabState.");
-        } catch (IOException exception) {
-            Log.w(TAG, "IO Exception while attempting to save tab state.");
         } catch (OutOfMemoryError e) {
             Log.w(TAG, "Out of memory error while attempting to save tab state.  Erasing.");
             deleteTabState(tabId, encrypted);
-        } finally {
-            StreamUtil.closeQuietly(stream);
         }
 
         return false;
@@ -70,9 +58,4 @@ public abstract class TabPersister {
     public void deleteTabState(int id, boolean encrypted) {
         TabState.deleteTabState(getStateDirectory(), id, encrypted);
     }
-
-    protected FileOutputStream openTabStateOutputStream(int id, boolean encrypted)
-            throws IOException {
-        return new FileOutputStream(getTabStateFile(id, encrypted));
-    }
 }
\ No newline at end of file

chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java

@@ -24,7 +24,6 @@ import org.chromium.base.ActivityState;
 import org.chromium.base.ApiCompatibilityUtils;
 import org.chromium.base.ApplicationStatus;
 import org.chromium.base.Log;
-import org.chromium.base.StreamUtil;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.blink_public.platform.WebDisplayMode;
@@ -46,9 +45,6 @@ import org.chromium.net.NetworkChangeNotifier;
 import org.chromium.ui.base.PageTransition;
 
 import java.io.File;
-import java.io.FileNotFoundException;
-import java.io.FileOutputStream;
-import java.io.IOException;
 import java.util.concurrent.TimeUnit;
 
 /**
@@ -176,22 +172,15 @@ public class WebappActivity extends FullScreenActivity {
         String tabFileName = TabState.getTabStateFilename(getActivityTab().getId(), false);
         File tabFile = new File(activityDirectory, tabFileName);
 
-        FileOutputStream foutput = null;
         // Temporarily allowing disk access while fixing. TODO: http://crbug.com/525781
         StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskReads();
         StrictMode.allowThreadDiskWrites();
         try {
             long time = SystemClock.elapsedRealtime();
-            foutput = new FileOutputStream(tabFile);
-            TabState.saveState(foutput, getActivityTab().getState(), false);
+            TabState.saveState(tabFile, getActivityTab().getState(), false);
             RecordHistogram.recordTimesHistogram("Android.StrictMode.WebappSaveState",
                     SystemClock.elapsedRealtime() - time, TimeUnit.MILLISECONDS);
-        } catch (FileNotFoundException exception) {
-            Log.e(TAG, "Failed to save out tab state.", exception);
-        } catch (IOException exception) {
-            Log.e(TAG, "Failed to save out tab state.", exception);
         } finally {
-            StreamUtil.closeQuietly(foutput);
             StrictMode.setThreadPolicy(oldPolicy);
         }
     }

chrome/android/java_sources.gni

@@ -1302,6 +1302,7 @@ chrome_junit_test_java_sources = [
   "junit/src/org/chromium/chrome/browser/payments/CurrencyStringFormatterTest.java",
   "junit/src/org/chromium/chrome/browser/snackbar/SnackbarCollectionUnitTest.java",
   "junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderUnitTest.java",
+  "junit/src/org/chromium/chrome/browser/tabstate/TabStateUnitTest.java",
   "junit/src/org/chromium/chrome/browser/util/NonThreadSafeTest.java",
   "junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java",
   "junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java",

chrome/android/junit/src/org/chromium/chrome/browser/tabstate/TabStateUnitTest.java

+// Copyright 2016 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.tabstate;
+
+import static org.junit.Assert.assertEquals;
+
+import org.chromium.base.StreamUtil;
+import org.chromium.chrome.browser.TabState;
+import org.chromium.testing.local.LocalRobolectricTestRunner;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.robolectric.annotation.Config;
+
+import java.io.DataOutputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.nio.channels.FileChannel;
+
+/**
+ * Unit tests for TabState.
+ */
+@RunWith(LocalRobolectricTestRunner.class)
+@Config(manifest = Config.NONE)
+public class TabStateUnitTest {
+
+    private static final byte[] CONTENTS_STATE_BYTES = new byte[] {1, 2, 3};
+    private static final long TIMESTAMP = 10L;
+    private static final long SYNC_ID = 3;
+    private static final int PARENT_ID = 1;
+    private static final int VERSION = 2;
+    private static final int THEME_COLOR = 4;
+    private static final boolean SHOULD_PRESERVE = true;
+    private static final String OPENER_APP_ID = "test";
+
+    @Rule
+    public TemporaryFolder temporaryFolder = new TemporaryFolder();
+
+    @Test
+    public void testSaveTabStateWithMemoryMappedContentsState() throws IOException {
+        File file = createTestTabStateFile();
+        TabState state = createTabStateWithMappedByteBuffer(file);
+        TabState.saveState(file, state, false);
+
+        validateTestTabState(TabState.restoreTabState(file, false));
+    }
+
+    private TabState createTabStateWithMappedByteBuffer(File file) throws IOException {
+        TabState state = new TabState();
+        FileInputStream fileInputStream = null;
+
+        try {
+            fileInputStream = new FileInputStream(file);
+            state.contentsState = new TabState.WebContentsState(fileInputStream.getChannel().map(
+                    FileChannel.MapMode.READ_ONLY, fileInputStream.getChannel().position(),
+                     file.length()));
+            state.contentsState.setVersion(VERSION);
+            state.timestampMillis = TIMESTAMP;
+            state.parentId = PARENT_ID;
+            state.syncId = SYNC_ID;
+            state.themeColor = THEME_COLOR;
+            state.shouldPreserve = SHOULD_PRESERVE;
+            state.openerAppId = OPENER_APP_ID;
+        } finally {
+            StreamUtil.closeQuietly(fileInputStream);
+        }
+        return state;
+    }
+
+    private void validateTestTabState(TabState state) {
+        assertEquals(TIMESTAMP, state.timestampMillis);
+        assertEquals(PARENT_ID, state.parentId);
+        assertEquals(OPENER_APP_ID, state.openerAppId);
+        assertEquals(VERSION, state.contentsState.version());
+        assertEquals(SYNC_ID, state.syncId);
+        assertEquals(SHOULD_PRESERVE, state.shouldPreserve);
+        assertEquals(THEME_COLOR, state.getThemeColor());
+        assertEquals(CONTENTS_STATE_BYTES.length, state.contentsState.buffer().remaining());
+
+        byte[] bytesFromFile = new byte[CONTENTS_STATE_BYTES.length];
+        state.contentsState.buffer().get(bytesFromFile);
+
+        for (int i = 0; i < CONTENTS_STATE_BYTES.length; i++) {
+            assertEquals(bytesFromFile[i], CONTENTS_STATE_BYTES[i]);
+        }
+    }
+
+    private File createTestTabStateFile() throws IOException {
+        File file = temporaryFolder.newFile("tabStateByteBufferTestFile");
+        FileOutputStream fileOutputStream = null;
+        DataOutputStream dataOutputStream = null;
+        try {
+            fileOutputStream = new FileOutputStream(file);
+            dataOutputStream = new DataOutputStream(fileOutputStream);
+            dataOutputStream.write(CONTENTS_STATE_BYTES);
+        } finally {
+            StreamUtil.closeQuietly(fileOutputStream);
+            StreamUtil.closeQuietly(dataOutputStream);
+        }
+        return file;
+    }
+}