Merge ~osomon/oxide:fix-1649861-serialized-navigation-entries into oxide:master

[Olivier Tilloy]

Fix session save/restore across oxide versions (LP: #1649861).

Make session save/restore future-proof by storing each navigation entry separately.

[Chris Coulson (chrisccoulson)]

Thanks for this - I've left a comment inline.

Also, there needs to be some tests for the upgrade path too.

review: Needs Fixing

[Olivier Tilloy (osomon)]

Thanks for the review. I’ve replied to your comment with a (hopefully) thorough analysis of the potential issue, and I’ll add a comment in the code for future reference. I’ll also look into adding tests for the upgrade path.

0097afa... by Olivier Tilloy on 2017-01-04

Add a comment for future reference.

554b6ad... by Olivier Tilloy on 2017-01-04

Add a unit test for the upgrade path.

[Olivier Tilloy (osomon)]

Updated with a comment for future reference and a unit test for the upgrade path.

802c915... by Olivier Tilloy on 2017-01-04

Add missing test files.

[Olivier Tilloy (osomon)]

Thanks for your review Santosh. See my answers to your comments inline.

[Chris Coulson (chrisccoulson)]

Ok, I've replied inline.

One other thing to consider - 1.19 has already shipped to the archive with this bug. I'm not sure if there's a way to handle upgrades from the current 1.19 release without breaking restoration for a second time, as state saved with the current 1.19 release at least has an extra integer for each navigation entry. Given the extra integer is always zero, and each subsequent serialized navigation entry starts with the index (which should always be non-zero), then there's probably a clever way to detect this...

[Chris Coulson (chrisccoulson)]

> Ok, I've replied inline.
>
> One other thing to consider - 1.19 has already shipped to the archive with
> this bug. I'm not sure if there's a way to handle upgrades from the current
> 1.19 release without breaking restoration for a second time, as state saved
> with the current 1.19 release at least has an extra integer for each
> navigation entry. Given the extra integer is always zero, and each subsequent
> serialized navigation entry starts with the index (which should always be non-
> zero), then there's probably a clever way to detect this...

Also, given this, it might be worth serializing the Oxide version in the new format.

46a689b... by Olivier Tilloy on 2017-01-12

Also serialize oxide version in new state format.

b90c7c8... by Olivier Tilloy on 2017-01-12

Also handle the upgrade path from 1.19.6 to 1.19.7.

[Olivier Tilloy (osomon)]

All addressed. Comments welcome.

[Chris Coulson (chrisccoulson)]

review: Approve

diff --git a/qt/core/browser/oxide_qt_web_view.cc b/qt/core/browser/oxide_qt_web_view.cc
index 975c937..af07a4e 100644
--- a/qt/core/browser/oxide_qt_web_view.cc
+++ b/qt/core/browser/oxide_qt_web_view.cc
@@ -34,6 +34,7 @@
 #include "base/macros.h"
 #include "base/pickle.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/version.h"
 #include "cc/output/compositor_frame_metadata.h"
 #include "content/public/browser/host_zoom_map.h"
 #include "content/public/browser/native_web_keyboard_event.h"
@@ -50,6 +51,7 @@
 #include "url/gurl.h"
 
 #include "qt/core/api/oxideqdownloadrequest.h"
+#include "qt/core/api/oxideqglobal.h"
 #include "qt/core/api/oxideqloadevent.h"
 #include "qt/core/api/oxideqhttpauthenticationrequest_p.h"
 #include "qt/core/api/oxideqnavigationrequest.h"
@@ -142,7 +144,7 @@ OxideQLoadEvent::ErrorDomain ErrorDomainFromErrorCode(int error_code) {
 }
 
 static const char* STATE_SERIALIZER_MAGIC_NUMBER = "oxide";
-static uint16_t STATE_SERIALIZER_VERSION = 1;
+static uint16_t STATE_SERIALIZER_VERSION = 2;
 
 void CreateRestoreEntriesFromRestoreState(
     const QByteArray& state,
@@ -167,28 +169,157 @@ void CreateRestoreEntriesFromRestoreState(
     WARN_INVALID_DATA;
     return;
   }
-  if (version != STATE_SERIALIZER_VERSION) {
+  if (version < 1 || version > STATE_SERIALIZER_VERSION) {
     WARN_INVALID_DATA;
     return;
   }
+  base::Version oxide_version;
+  if (version >= 2) {
+    std::string v;
+    if (!i.ReadString(&v)) {
+      WARN_INVALID_DATA;
+      return;
+    }
+    oxide_version = base::Version(v);
+    if (oxide_version.CompareTo(base::Version(std::string("1.19.7"))) == -1) {
+      // The oxide version in the serialized state was added in oxide 1.19.7
+      WARN_INVALID_DATA;
+      return;
+    }
+  }
   int count;
   if (!i.ReadLength(&count)) {
     WARN_INVALID_DATA;
     return;
   }
   entries.resize(count);
-  for (int j = 0; j < count; ++j) {
-    sessions::SerializedNavigationEntry entry;
-    if (!entry.ReadFromPickle(&i)) {
-      WARN_INVALID_DATA;
-      return;
-    }
-    entries[j] = entry;
-  }
   int index;
-  if (!i.ReadInt(&index)) {
-    WARN_INVALID_DATA; 
-    return;
+  switch (version) {
+    case 1: {
+      // In chromium < 55, the serialized navigation
+      // entries did not store extended info
+      // (https://chromium.googlesource.com/chromium/src/+/f62ca6f).
+      // Because of the way oxide concatenated all serialized navigation
+      // entries, oxide 1.19 would fail to deserialize entries stored with
+      // oxide < 1.19 (https://launchpad.net/bugs/1649861).
+      // To work around that issue, a bit of careful pointer arithmetic is
+      // required.
+      // Note that states serialized with oxide >=1.19.0 and <=1.19.6 still
+      // advertise version 1, but used chromium 55, so they store extended info,
+      // and therefore the pointer arithmetic is not needed. To detect that, we
+      // rely on the fact that each navigation entry will have an extra integer
+      // whose value is always 0 (the size of the extended info map), and if
+      // read as part of the next entry, it will be mistaken for the entry's
+      // index which should never be 0 (except maybe for the first entry).
+      bool reading_too_much_data = true;
+      base::PickleSizer sizer;
+      sizer.AddString(magic_number); // magic number
+      sizer.AddUInt16(); // version
+      sizer.AddInt(); // count
+      sizer.AddInt(); // index
+      const char* raw_data = nullptr;
+      int length = state.size() - sizer.payload_size();
+      if (!i.ReadBytes(&raw_data, length)) {
+        WARN_INVALID_DATA;
+        return;
+      }
+      for (int j = 0; j < count; ++j) {
+        base::Pickle pickled_entry;
+        pickled_entry.WriteBytes(raw_data, length);
+        base::PickleIterator k(pickled_entry);
+        sessions::SerializedNavigationEntry entry;
+        // This will potentially try to read too much data (the inexistent
+        // extended_info_map_), so we need to compute the actual size of the
+        // serialized entry to correctly advance the pointer to the beginning
+        // of the next entry.
+        // Reading too much data will potentially add garbage entries to the
+        // extended info map (if for any invalid entry two strings can be read,
+        // key and value). For each entry in the resulting map,
+        // ContentSerializedNavigationBuilder::ToNavigationEntry() looks up an
+        // extended info handler (by key). If it can't find one, it just skips
+        // the unknown entry (most likely case). As of today (January 2017),
+        // there's only one handler registered by chrome on android, which is
+        // not relevant here. This would become a concern if oxide registered
+        // its own handlers, and their implementation would need to check for
+        // value correctness and silently ignore garbage values.
+        if (j == 1) {
+          // Try to read an integer first, if its value is 0 (invalid entry
+          // index) then we were not actually reading too much data (the
+          // extended info map had been serialized).
+          int m = 0;
+          if (!k.ReadInt(&m)) {
+            WARN_INVALID_DATA;
+            return;
+          }
+          if (m == 0) {
+            reading_too_much_data = false;
+            base::PickleSizer int_sizer;
+            int_sizer.AddInt();
+            raw_data += int_sizer.payload_size();
+            length -= int_sizer.payload_size();
+          } else {
+            k = base::PickleIterator(pickled_entry);
+          }
+        }
+        if (!entry.ReadFromPickle(&k)) {
+          WARN_INVALID_DATA;
+          return;
+        }
+        entries[j] = entry;
+        base::PickleSizer entry_sizer;
+        entry_sizer.AddInt(); // index
+        entry_sizer.AddString(entry.virtual_url().spec());
+        entry_sizer.AddString16(entry.title());
+        entry_sizer.AddString(entry.encoded_page_state());
+        entry_sizer.AddInt(); // transition type
+        entry_sizer.AddInt(); // type mask
+        entry_sizer.AddString(entry.referrer_url().spec());
+        entry_sizer.AddInt(); // mapped referrer policy
+        entry_sizer.AddString(entry.original_request_url().spec());
+        entry_sizer.AddBool(); // is overriding user agent
+        entry_sizer.AddInt64(); // timestamp
+        entry_sizer.AddString16(entry.search_terms());
+        entry_sizer.AddInt(); // HTTP status code
+        entry_sizer.AddInt(); // referrer policy
+        if (!reading_too_much_data) {
+          entry_sizer.AddInt(); // extended info map size, always 0
+        }
+        raw_data += entry_sizer.payload_size();
+        length -= entry_sizer.payload_size();
+      }
+      base::Pickle remainder;
+      remainder.WriteBytes(raw_data, length);
+      i = base::PickleIterator(remainder);
+      if (!i.ReadInt(&index)) {
+        WARN_INVALID_DATA;
+        return;
+      }
+      break;
+    }
+    case 2:
+    default: {
+      for (int j = 0; j < count; ++j) {
+        const char* data;
+        int length;
+        if (!i.ReadData(&data, &length)) {
+          WARN_INVALID_DATA;
+          return;
+        }
+        base::Pickle pickled_entry(data, length);
+        base::PickleIterator k(pickled_entry);
+        sessions::SerializedNavigationEntry entry;
+        if (!entry.ReadFromPickle(&k)) {
+          WARN_INVALID_DATA;
+          return;
+        }
+        entries[j] = entry;
+      }
+      if (!i.ReadInt(&index)) {
+        WARN_INVALID_DATA;
+        return;
+      }
+      break;
+    }
   }
 #undef WARN_INVALID_DATA
 
@@ -765,12 +896,15 @@ QByteArray WebView::currentState() const {
   base::Pickle pickle;
   pickle.WriteString(STATE_SERIALIZER_MAGIC_NUMBER);
   pickle.WriteUInt16(STATE_SERIALIZER_VERSION);
+  pickle.WriteString(oxideGetVersion().toStdString());
   pickle.WriteInt(entries.size());
   std::vector<sessions::SerializedNavigationEntry>::const_iterator i;
   static const size_t max_state_size =
       std::numeric_limits<uint16_t>::max() - 1024;
   for (i = entries.begin(); i != entries.end(); ++i) {
-    i->WriteToPickle(max_state_size, &pickle);
+    base::Pickle entry;
+    i->WriteToPickle(max_state_size, &entry);
+    pickle.WriteData(static_cast<const char*>(entry.data()), entry.size());
   }
   pickle.WriteInt(
       web_view_->GetWebContents()->GetController().GetCurrentEntryIndex());
diff --git a/qt/tests/qmltests/api/tst_WebView_save_restore_entry1.html b/qt/tests/qmltests/api/tst_WebView_save_restore_entry1.html
new file mode 100644
index 0000000..5add6f1
--- /dev/null
+++ b/qt/tests/qmltests/api/tst_WebView_save_restore_entry1.html
@@ -0,0 +1,5 @@
+<html>
+<body>
+  page 1
+</body>
+</html>
diff --git a/qt/tests/qmltests/api/tst_WebView_save_restore_entry2.html b/qt/tests/qmltests/api/tst_WebView_save_restore_entry2.html
new file mode 100644
index 0000000..ff2f811
--- /dev/null
+++ b/qt/tests/qmltests/api/tst_WebView_save_restore_entry2.html
@@ -0,0 +1,5 @@
+<html>
+<body>
+  page 2
+</body>
+</html>
diff --git a/qt/tests/qmltests/api/tst_WebView_save_restore_entry3.html b/qt/tests/qmltests/api/tst_WebView_save_restore_entry3.html
new file mode 100644
index 0000000..c8af8f4
--- /dev/null
+++ b/qt/tests/qmltests/api/tst_WebView_save_restore_entry3.html
@@ -0,0 +1,5 @@
+<html>
+<body>
+  page 3
+</body>
+</html>
diff --git a/qt/tests/qmltests/api/tst_WebView_save_restore_state.qml b/qt/tests/qmltests/api/tst_WebView_save_restore_state.qml
index 51743df..5665eb2 100644
--- a/qt/tests/qmltests/api/tst_WebView_save_restore_state.qml
+++ b/qt/tests/qmltests/api/tst_WebView_save_restore_state.qml
@@ -193,5 +193,87 @@ Item {
       });
       restored.destroy();
     }
+
+    function test_WebView_restore_from_old_state_data() {
+      // All serialized state have version 1 (oxide 1.19.6 and older) and they
+      // contain three navigation entries (current index is 2).
+      return [
+        // State serialized with oxide 1.18 or older and restored with oxide
+        // 1.19 or newer (https://launchpad.net/bugs/1649861).
+        {state:
+          "sAQAAAUAAABveGlkZQAAAAEAAAADAAAAAAAAADUAAABodHRwOi8vdGVzdHN1aXRlL3Rz\
+           dF9XZWJWaWV3X3NhdmVfcmVzdG9yZV9lbnRyeTEuaHRtbAAAAAAAAADcAAAA2AAAABcA\
+           AAAAAAAAagAAAGgAdAB0AHAAOgAvAC8AdABlAHMAdABzAHUAaQB0AGUALwB0AHMAdABf\
+           AFcAZQBiAFYAaQBlAHcAXwBzAGEAdgBlAF8AcgBlAHMAdABvAHIAZQBfAGUAbgB0AHIA\
+           eQAxAC4AaAB0AG0AbAAAAP////8AAAAAAAAAAP////8AAAAACAAAAAAAAAAAAPA/bZlL\
+           pUxFBQBumUulTEUFAAIAAAAIAAAAAAAAAAAAAAAIAAAAAAAAAAAAAAAAAAAAAAAAAAAA\
+           AAD/////AAAAAAEAAAgAAAAAAAAAAAEAAAA1AAAAaHR0cDovL3Rlc3RzdWl0ZS90c3Rf\
+           V2ViVmlld19zYXZlX3Jlc3RvcmVfZW50cnkxLmh0bWwAAAAAAAAANwPS7eKjLgAAAAAA\
+           yAAAAAEAAAABAAAANQAAAGh0dHA6Ly90ZXN0c3VpdGUvdHN0X1dlYlZpZXdfc2F2ZV9y\
+           ZXN0b3JlX2VudHJ5Mi5odG1sAAAAAAAAANwAAADYAAAAFwAAAAAAAABqAAAAaAB0AHQA\
+           cAA6AC8ALwB0AGUAcwB0AHMAdQBpAHQAZQAvAHQAcwB0AF8AVwBlAGIAVgBpAGUAdwBf\
+           AHMAYQB2AGUAXwByAGUAcwB0AG8AcgBlAF8AZQBuAHQAcgB5ADIALgBoAHQAbQBsAAAA\
+           /////wAAAAA\AAAAA/////wAAAAAIAAAAAAAAAAAA8D9vmUulTEUFAHCZS6VMRQUAAgA\
+           AAAgAAAAAAAAAAAAAAAgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAP////8AAAAAAQAACAA\
+           AAAAAAAAAAQAAADUAAABodHRwOi8vdGVzdHN1aXRlL3RzdF9XZWJWaWV3X3NhdmVfcmV\
+           zdG9yZV9lbnRyeTIuaHRtbAAAAAAAAAACKdPt4qMuAAAAAADIAAAAAQAAAAIAAAA1AAA\
+           AaHR0cDovL3Rlc3RzdWl0ZS90c3RfV2ViVmlld19zYXZlX3Jlc3RvcmVfZW50cnkzLmh\
+           0bWwAAAAAAAAA3AAAANgAAAAXAAAAAAAAAGoAAABoAHQAdABwADoALwAvAHQAZQBzAHQ\
+           AcwB1AGkAdABlAC8AdABzAHQAXwBXAGUAYgBWAGkAZQB3AF8AcwBhAHYAZQBfAHIAZQB\
+           zAHQAbwByAGUAXwBlAG4AdAByAHkAMwAuAGgAdABtAGwAAAD/////AAAAAAAAAAD////\
+           /AAAAAAgAAAAAAAAAAAAAAHGZS6VMRQUAcplLpUxFBQACAAAACAAAAAAAAAAAAAAACAA\
+           AAAAAAAAAAAAAAAAAAAAAAAAAAAAA/////wAAAAABAAAIAAAAAAAAAAABAAAANQAAAGh\
+           0dHA6Ly90ZXN0c3VpdGUvdHN0X1dlYlZpZXdfc2F2ZV9yZXN0b3JlX2VudHJ5My5odG1\
+           sAAAAAAAAAB8q1O3ioy4AAAAAAMgAAAABAAAAAgAAAA=="},
+
+        // State serialized with oxide >=1.19.0 and <=1.19.6 and restored with
+        // oxide 1.19.7 or newer.
+        {state:
+          "vAQAAAUAAABveGlkZQAAAAEAAAADAAAAAAAAADUAAABodHRwOi8vdGVzdHN1aXRlL3Rz\
+           dF9XZWJWaWV3X3NhdmVfcmVzdG9yZV9lbnRyeTEuaHRtbAAAAAAAAADcAAAA2AAAABcA\
+           AAAAAAAAagAAAGgAdAB0AHAAOgAvAC8AdABlAHMAdABzAHUAaQB0AGUALwB0AHMAdABf\
+           AFcAZQBiAFYAaQBlAHcAXwBzAGEAdgBlAF8AcgBlAHMAdABvAHIAZQBfAGUAbgB0AHIA\
+           eQAxAC4AaAB0AG0AbAAAAP////8AAAAAAAAAAP////8AAAAACAAAAAAAAAAAAPA/xS5g\
+           QuhFBQDGLmBC6EUFAAIAAAAIAAAAAAAAAAAAAAAIAAAAAAAAAAAAAAAAAAAAAAAAAAAA\
+           AAD/////AAAAAAEAAAgAAAAAAAAAAAEAAAA1AAAAaHR0cDovL3Rlc3RzdWl0ZS90c3Rf\
+           V2ViVmlld19zYXZlX3Jlc3RvcmVfZW50cnkxLmh0bWwAAAAAAAAA55Pmin6kLgAAAAAA\
+           yAAAAAEAAAAAAAAAAQAAADUAAABodHRwOi8vdGVzdHN1aXRlL3RzdF9XZWJWaWV3X3Nh\
+           dmVfcmVzdG9yZV9lbnRyeTIuaHRtbAAAAAAAAADcAAAA2AAAABcAAAAAAAAAagAAAGgA\
+           dAB0AHAAOgAvAC8AdABlAHMAdABzAHUAaQB0AGUALwB0AHMAdABfAFcAZQBiAFYAaQBl\
+           AHcAXwBzAGEAdgBlAF8AcgBlAHMAdABvAHIAZQBfAGUAbgB0AHIAeQAyAC4AaAB0AG0A\
+           bAAAAP////8AAAAAAAAAAP////8AAAAACAAAAAAAAAAAAPA/xy5gQuhFBQDILmBC6EUF\
+           AAIAAAAIAAAAAAAAAAAAAAAIAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD/////AAAAAAEA\
+           AAgAAAAAAAAAAAEAAAA1AAAAaHR0cDovL3Rlc3RzdWl0ZS90c3RfV2ViVmlld19zYXZl\
+           X3Jlc3RvcmVfZW50cnkyLmh0bWwAAAAAAAAA9eLnin6kLgAAAAAAyAAAAAEAAAAAAAAA\
+           AgAAADUAAABodHRwOi8vdGVzdHN1aXRlL3RzdF9XZWJWaWV3X3NhdmVfcmVzdG9yZV9l\
+           bnRyeTMuaHRtbAAAAAAAAADcAAAA2AAAABcAAAAAAAAAagAAAGgAdAB0AHAAOgAvAC8A\
+           dABlAHMAdABzAHUAaQB0AGUALwB0AHMAdABfAFcAZQBiAFYAaQBlAHcAXwBzAGEAdgBl\
+           AF8AcgBlAHMAdABvAHIAZQBfAGUAbgB0AHIAeQAzAC4AaAB0AG0AbAAAAP////8AAAAA\
+           AAAAAP////8AAAAACAAAAAAAAAAAAAAAyS5gQuhFBQDKLmBC6EUFAAIAAAAIAAAAAAAA\
+           AAAAAAAIAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD/////AAAAAAEAAAgAAAAAAAAAAAEA\
+           AAA1AAAAaHR0cDovL3Rlc3RzdWl0ZS90c3RfV2ViVmlld19zYXZlX3Jlc3RvcmVfZW50\
+           cnkzLmh0bWwAAAAAAAAA5o/oin6kLgAAAAAAyAAAAAEAAAAAAAAAAgAAAA=="}
+      ];
+    }
+
+    function test_WebView_restore_from_old_state(data) {
+      var restored = webViewComponent.createObject(
+          root,
+          {"restoreType": WebView.RestoreCurrentSession,
+           "restoreState": data.state});
+      verify(restored !== null);
+      tryCompare(restored, "url",
+                 "http://testsuite/tst_WebView_save_restore_entry3.html");
+      verify(restored.waitForLoadSucceeded(),
+             "Timed out waiting for successful load");
+      compare(restored.navigationHistory.currentItemIndex, 2);
+      var items = restored.navigationHistory.items;
+      for (var i = 0; i < 3; ++i) {
+        compare(items[i].url,
+                "http://testsuite/tst_WebView_save_restore_entry%1.html"
+                    .arg(i + 1));
+      }
+      restored.destroy();
+    }
   }
 }