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

Proposed by Olivier Tilloy on 2016-12-15
Status: Merged
Approved by: Chris Coulson on 2017-01-16
Approved revision: b90c7c8c4bb669c2a64fe7d8e1e67e6bbd3dfc08
Merged at revision: f40a17d2841c51309f605fab1a229b08e85d5305
Proposed branch: ~osomon/oxide:fix-1649861-serialized-navigation-entries
Merge into: oxide:master
Diff against target: 341 lines (+245/-14)
5 files modified
qt/core/browser/oxide_qt_web_view.cc (+148/-14)
qt/tests/qmltests/api/tst_WebView_save_restore_entry1.html (+5/-0)
qt/tests/qmltests/api/tst_WebView_save_restore_entry2.html (+5/-0)
qt/tests/qmltests/api/tst_WebView_save_restore_entry3.html (+5/-0)
qt/tests/qmltests/api/tst_WebView_save_restore_state.qml (+82/-0)
Reviewer Review Type Date Requested Status
Chris Coulson 2016-12-15 Approve on 2017-01-16
Review via email: mp+313310@code.launchpad.net

Commit message

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

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

To post a comment you must log in.
Chris Coulson (chrisccoulson) wrote :

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) wrote :

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) wrote :

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.

Santosh (santoshbit2007) :
Olivier Tilloy (osomon) wrote :

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

Chris Coulson (chrisccoulson) wrote :

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) wrote :

> 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) wrote :

All addressed. Comments welcome.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/qt/core/browser/oxide_qt_web_view.cc b/qt/core/browser/oxide_qt_web_view.cc
2index 975c937..af07a4e 100644
3--- a/qt/core/browser/oxide_qt_web_view.cc
4+++ b/qt/core/browser/oxide_qt_web_view.cc
5@@ -34,6 +34,7 @@
6 #include "base/macros.h"
7 #include "base/pickle.h"
8 #include "base/strings/utf_string_conversions.h"
9+#include "base/version.h"
10 #include "cc/output/compositor_frame_metadata.h"
11 #include "content/public/browser/host_zoom_map.h"
12 #include "content/public/browser/native_web_keyboard_event.h"
13@@ -50,6 +51,7 @@
14 #include "url/gurl.h"
15
16 #include "qt/core/api/oxideqdownloadrequest.h"
17+#include "qt/core/api/oxideqglobal.h"
18 #include "qt/core/api/oxideqloadevent.h"
19 #include "qt/core/api/oxideqhttpauthenticationrequest_p.h"
20 #include "qt/core/api/oxideqnavigationrequest.h"
21@@ -142,7 +144,7 @@ OxideQLoadEvent::ErrorDomain ErrorDomainFromErrorCode(int error_code) {
22 }
23
24 static const char* STATE_SERIALIZER_MAGIC_NUMBER = "oxide";
25-static uint16_t STATE_SERIALIZER_VERSION = 1;
26+static uint16_t STATE_SERIALIZER_VERSION = 2;
27
28 void CreateRestoreEntriesFromRestoreState(
29 const QByteArray& state,
30@@ -167,28 +169,157 @@ void CreateRestoreEntriesFromRestoreState(
31 WARN_INVALID_DATA;
32 return;
33 }
34- if (version != STATE_SERIALIZER_VERSION) {
35+ if (version < 1 || version > STATE_SERIALIZER_VERSION) {
36 WARN_INVALID_DATA;
37 return;
38 }
39+ base::Version oxide_version;
40+ if (version >= 2) {
41+ std::string v;
42+ if (!i.ReadString(&v)) {
43+ WARN_INVALID_DATA;
44+ return;
45+ }
46+ oxide_version = base::Version(v);
47+ if (oxide_version.CompareTo(base::Version(std::string("1.19.7"))) == -1) {
48+ // The oxide version in the serialized state was added in oxide 1.19.7
49+ WARN_INVALID_DATA;
50+ return;
51+ }
52+ }
53 int count;
54 if (!i.ReadLength(&count)) {
55 WARN_INVALID_DATA;
56 return;
57 }
58 entries.resize(count);
59- for (int j = 0; j < count; ++j) {
60- sessions::SerializedNavigationEntry entry;
61- if (!entry.ReadFromPickle(&i)) {
62- WARN_INVALID_DATA;
63- return;
64- }
65- entries[j] = entry;
66- }
67 int index;
68- if (!i.ReadInt(&index)) {
69- WARN_INVALID_DATA;
70- return;
71+ switch (version) {
72+ case 1: {
73+ // In chromium < 55, the serialized navigation
74+ // entries did not store extended info
75+ // (https://chromium.googlesource.com/chromium/src/+/f62ca6f).
76+ // Because of the way oxide concatenated all serialized navigation
77+ // entries, oxide 1.19 would fail to deserialize entries stored with
78+ // oxide < 1.19 (https://launchpad.net/bugs/1649861).
79+ // To work around that issue, a bit of careful pointer arithmetic is
80+ // required.
81+ // Note that states serialized with oxide >=1.19.0 and <=1.19.6 still
82+ // advertise version 1, but used chromium 55, so they store extended info,
83+ // and therefore the pointer arithmetic is not needed. To detect that, we
84+ // rely on the fact that each navigation entry will have an extra integer
85+ // whose value is always 0 (the size of the extended info map), and if
86+ // read as part of the next entry, it will be mistaken for the entry's
87+ // index which should never be 0 (except maybe for the first entry).
88+ bool reading_too_much_data = true;
89+ base::PickleSizer sizer;
90+ sizer.AddString(magic_number); // magic number
91+ sizer.AddUInt16(); // version
92+ sizer.AddInt(); // count
93+ sizer.AddInt(); // index
94+ const char* raw_data = nullptr;
95+ int length = state.size() - sizer.payload_size();
96+ if (!i.ReadBytes(&raw_data, length)) {
97+ WARN_INVALID_DATA;
98+ return;
99+ }
100+ for (int j = 0; j < count; ++j) {
101+ base::Pickle pickled_entry;
102+ pickled_entry.WriteBytes(raw_data, length);
103+ base::PickleIterator k(pickled_entry);
104+ sessions::SerializedNavigationEntry entry;
105+ // This will potentially try to read too much data (the inexistent
106+ // extended_info_map_), so we need to compute the actual size of the
107+ // serialized entry to correctly advance the pointer to the beginning
108+ // of the next entry.
109+ // Reading too much data will potentially add garbage entries to the
110+ // extended info map (if for any invalid entry two strings can be read,
111+ // key and value). For each entry in the resulting map,
112+ // ContentSerializedNavigationBuilder::ToNavigationEntry() looks up an
113+ // extended info handler (by key). If it can't find one, it just skips
114+ // the unknown entry (most likely case). As of today (January 2017),
115+ // there's only one handler registered by chrome on android, which is
116+ // not relevant here. This would become a concern if oxide registered
117+ // its own handlers, and their implementation would need to check for
118+ // value correctness and silently ignore garbage values.
119+ if (j == 1) {
120+ // Try to read an integer first, if its value is 0 (invalid entry
121+ // index) then we were not actually reading too much data (the
122+ // extended info map had been serialized).
123+ int m = 0;
124+ if (!k.ReadInt(&m)) {
125+ WARN_INVALID_DATA;
126+ return;
127+ }
128+ if (m == 0) {
129+ reading_too_much_data = false;
130+ base::PickleSizer int_sizer;
131+ int_sizer.AddInt();
132+ raw_data += int_sizer.payload_size();
133+ length -= int_sizer.payload_size();
134+ } else {
135+ k = base::PickleIterator(pickled_entry);
136+ }
137+ }
138+ if (!entry.ReadFromPickle(&k)) {
139+ WARN_INVALID_DATA;
140+ return;
141+ }
142+ entries[j] = entry;
143+ base::PickleSizer entry_sizer;
144+ entry_sizer.AddInt(); // index
145+ entry_sizer.AddString(entry.virtual_url().spec());
146+ entry_sizer.AddString16(entry.title());
147+ entry_sizer.AddString(entry.encoded_page_state());
148+ entry_sizer.AddInt(); // transition type
149+ entry_sizer.AddInt(); // type mask
150+ entry_sizer.AddString(entry.referrer_url().spec());
151+ entry_sizer.AddInt(); // mapped referrer policy
152+ entry_sizer.AddString(entry.original_request_url().spec());
153+ entry_sizer.AddBool(); // is overriding user agent
154+ entry_sizer.AddInt64(); // timestamp
155+ entry_sizer.AddString16(entry.search_terms());
156+ entry_sizer.AddInt(); // HTTP status code
157+ entry_sizer.AddInt(); // referrer policy
158+ if (!reading_too_much_data) {
159+ entry_sizer.AddInt(); // extended info map size, always 0
160+ }
161+ raw_data += entry_sizer.payload_size();
162+ length -= entry_sizer.payload_size();
163+ }
164+ base::Pickle remainder;
165+ remainder.WriteBytes(raw_data, length);
166+ i = base::PickleIterator(remainder);
167+ if (!i.ReadInt(&index)) {
168+ WARN_INVALID_DATA;
169+ return;
170+ }
171+ break;
172+ }
173+ case 2:
174+ default: {
175+ for (int j = 0; j < count; ++j) {
176+ const char* data;
177+ int length;
178+ if (!i.ReadData(&data, &length)) {
179+ WARN_INVALID_DATA;
180+ return;
181+ }
182+ base::Pickle pickled_entry(data, length);
183+ base::PickleIterator k(pickled_entry);
184+ sessions::SerializedNavigationEntry entry;
185+ if (!entry.ReadFromPickle(&k)) {
186+ WARN_INVALID_DATA;
187+ return;
188+ }
189+ entries[j] = entry;
190+ }
191+ if (!i.ReadInt(&index)) {
192+ WARN_INVALID_DATA;
193+ return;
194+ }
195+ break;
196+ }
197 }
198 #undef WARN_INVALID_DATA
199
200@@ -765,12 +896,15 @@ QByteArray WebView::currentState() const {
201 base::Pickle pickle;
202 pickle.WriteString(STATE_SERIALIZER_MAGIC_NUMBER);
203 pickle.WriteUInt16(STATE_SERIALIZER_VERSION);
204+ pickle.WriteString(oxideGetVersion().toStdString());
205 pickle.WriteInt(entries.size());
206 std::vector<sessions::SerializedNavigationEntry>::const_iterator i;
207 static const size_t max_state_size =
208 std::numeric_limits<uint16_t>::max() - 1024;
209 for (i = entries.begin(); i != entries.end(); ++i) {
210- i->WriteToPickle(max_state_size, &pickle);
211+ base::Pickle entry;
212+ i->WriteToPickle(max_state_size, &entry);
213+ pickle.WriteData(static_cast<const char*>(entry.data()), entry.size());
214 }
215 pickle.WriteInt(
216 web_view_->GetWebContents()->GetController().GetCurrentEntryIndex());
217diff --git a/qt/tests/qmltests/api/tst_WebView_save_restore_entry1.html b/qt/tests/qmltests/api/tst_WebView_save_restore_entry1.html
218new file mode 100644
219index 0000000..5add6f1
220--- /dev/null
221+++ b/qt/tests/qmltests/api/tst_WebView_save_restore_entry1.html
222@@ -0,0 +1,5 @@
223+<html>
224+<body>
225+ page 1
226+</body>
227+</html>
228diff --git a/qt/tests/qmltests/api/tst_WebView_save_restore_entry2.html b/qt/tests/qmltests/api/tst_WebView_save_restore_entry2.html
229new file mode 100644
230index 0000000..ff2f811
231--- /dev/null
232+++ b/qt/tests/qmltests/api/tst_WebView_save_restore_entry2.html
233@@ -0,0 +1,5 @@
234+<html>
235+<body>
236+ page 2
237+</body>
238+</html>
239diff --git a/qt/tests/qmltests/api/tst_WebView_save_restore_entry3.html b/qt/tests/qmltests/api/tst_WebView_save_restore_entry3.html
240new file mode 100644
241index 0000000..c8af8f4
242--- /dev/null
243+++ b/qt/tests/qmltests/api/tst_WebView_save_restore_entry3.html
244@@ -0,0 +1,5 @@
245+<html>
246+<body>
247+ page 3
248+</body>
249+</html>
250diff --git a/qt/tests/qmltests/api/tst_WebView_save_restore_state.qml b/qt/tests/qmltests/api/tst_WebView_save_restore_state.qml
251index 51743df..5665eb2 100644
252--- a/qt/tests/qmltests/api/tst_WebView_save_restore_state.qml
253+++ b/qt/tests/qmltests/api/tst_WebView_save_restore_state.qml
254@@ -193,5 +193,87 @@ Item {
255 });
256 restored.destroy();
257 }
258+
259+ function test_WebView_restore_from_old_state_data() {
260+ // All serialized state have version 1 (oxide 1.19.6 and older) and they
261+ // contain three navigation entries (current index is 2).
262+ return [
263+ // State serialized with oxide 1.18 or older and restored with oxide
264+ // 1.19 or newer (https://launchpad.net/bugs/1649861).
265+ {state:
266+ "sAQAAAUAAABveGlkZQAAAAEAAAADAAAAAAAAADUAAABodHRwOi8vdGVzdHN1aXRlL3Rz\
267+ dF9XZWJWaWV3X3NhdmVfcmVzdG9yZV9lbnRyeTEuaHRtbAAAAAAAAADcAAAA2AAAABcA\
268+ AAAAAAAAagAAAGgAdAB0AHAAOgAvAC8AdABlAHMAdABzAHUAaQB0AGUALwB0AHMAdABf\
269+ AFcAZQBiAFYAaQBlAHcAXwBzAGEAdgBlAF8AcgBlAHMAdABvAHIAZQBfAGUAbgB0AHIA\
270+ eQAxAC4AaAB0AG0AbAAAAP////8AAAAAAAAAAP////8AAAAACAAAAAAAAAAAAPA/bZlL\
271+ pUxFBQBumUulTEUFAAIAAAAIAAAAAAAAAAAAAAAIAAAAAAAAAAAAAAAAAAAAAAAAAAAA\
272+ AAD/////AAAAAAEAAAgAAAAAAAAAAAEAAAA1AAAAaHR0cDovL3Rlc3RzdWl0ZS90c3Rf\
273+ V2ViVmlld19zYXZlX3Jlc3RvcmVfZW50cnkxLmh0bWwAAAAAAAAANwPS7eKjLgAAAAAA\
274+ yAAAAAEAAAABAAAANQAAAGh0dHA6Ly90ZXN0c3VpdGUvdHN0X1dlYlZpZXdfc2F2ZV9y\
275+ ZXN0b3JlX2VudHJ5Mi5odG1sAAAAAAAAANwAAADYAAAAFwAAAAAAAABqAAAAaAB0AHQA\
276+ cAA6AC8ALwB0AGUAcwB0AHMAdQBpAHQAZQAvAHQAcwB0AF8AVwBlAGIAVgBpAGUAdwBf\
277+ AHMAYQB2AGUAXwByAGUAcwB0AG8AcgBlAF8AZQBuAHQAcgB5ADIALgBoAHQAbQBsAAAA\
278+ /////wAAAAA\AAAAA/////wAAAAAIAAAAAAAAAAAA8D9vmUulTEUFAHCZS6VMRQUAAgA\
279+ AAAgAAAAAAAAAAAAAAAgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAP////8AAAAAAQAACAA\
280+ AAAAAAAAAAQAAADUAAABodHRwOi8vdGVzdHN1aXRlL3RzdF9XZWJWaWV3X3NhdmVfcmV\
281+ zdG9yZV9lbnRyeTIuaHRtbAAAAAAAAAACKdPt4qMuAAAAAADIAAAAAQAAAAIAAAA1AAA\
282+ AaHR0cDovL3Rlc3RzdWl0ZS90c3RfV2ViVmlld19zYXZlX3Jlc3RvcmVfZW50cnkzLmh\
283+ 0bWwAAAAAAAAA3AAAANgAAAAXAAAAAAAAAGoAAABoAHQAdABwADoALwAvAHQAZQBzAHQ\
284+ AcwB1AGkAdABlAC8AdABzAHQAXwBXAGUAYgBWAGkAZQB3AF8AcwBhAHYAZQBfAHIAZQB\
285+ zAHQAbwByAGUAXwBlAG4AdAByAHkAMwAuAGgAdABtAGwAAAD/////AAAAAAAAAAD////\
286+ /AAAAAAgAAAAAAAAAAAAAAHGZS6VMRQUAcplLpUxFBQACAAAACAAAAAAAAAAAAAAACAA\
287+ AAAAAAAAAAAAAAAAAAAAAAAAAAAAA/////wAAAAABAAAIAAAAAAAAAAABAAAANQAAAGh\
288+ 0dHA6Ly90ZXN0c3VpdGUvdHN0X1dlYlZpZXdfc2F2ZV9yZXN0b3JlX2VudHJ5My5odG1\
289+ sAAAAAAAAAB8q1O3ioy4AAAAAAMgAAAABAAAAAgAAAA=="},
290+
291+ // State serialized with oxide >=1.19.0 and <=1.19.6 and restored with
292+ // oxide 1.19.7 or newer.
293+ {state:
294+ "vAQAAAUAAABveGlkZQAAAAEAAAADAAAAAAAAADUAAABodHRwOi8vdGVzdHN1aXRlL3Rz\
295+ dF9XZWJWaWV3X3NhdmVfcmVzdG9yZV9lbnRyeTEuaHRtbAAAAAAAAADcAAAA2AAAABcA\
296+ AAAAAAAAagAAAGgAdAB0AHAAOgAvAC8AdABlAHMAdABzAHUAaQB0AGUALwB0AHMAdABf\
297+ AFcAZQBiAFYAaQBlAHcAXwBzAGEAdgBlAF8AcgBlAHMAdABvAHIAZQBfAGUAbgB0AHIA\
298+ eQAxAC4AaAB0AG0AbAAAAP////8AAAAAAAAAAP////8AAAAACAAAAAAAAAAAAPA/xS5g\
299+ QuhFBQDGLmBC6EUFAAIAAAAIAAAAAAAAAAAAAAAIAAAAAAAAAAAAAAAAAAAAAAAAAAAA\
300+ AAD/////AAAAAAEAAAgAAAAAAAAAAAEAAAA1AAAAaHR0cDovL3Rlc3RzdWl0ZS90c3Rf\
301+ V2ViVmlld19zYXZlX3Jlc3RvcmVfZW50cnkxLmh0bWwAAAAAAAAA55Pmin6kLgAAAAAA\
302+ yAAAAAEAAAAAAAAAAQAAADUAAABodHRwOi8vdGVzdHN1aXRlL3RzdF9XZWJWaWV3X3Nh\
303+ dmVfcmVzdG9yZV9lbnRyeTIuaHRtbAAAAAAAAADcAAAA2AAAABcAAAAAAAAAagAAAGgA\
304+ dAB0AHAAOgAvAC8AdABlAHMAdABzAHUAaQB0AGUALwB0AHMAdABfAFcAZQBiAFYAaQBl\
305+ AHcAXwBzAGEAdgBlAF8AcgBlAHMAdABvAHIAZQBfAGUAbgB0AHIAeQAyAC4AaAB0AG0A\
306+ bAAAAP////8AAAAAAAAAAP////8AAAAACAAAAAAAAAAAAPA/xy5gQuhFBQDILmBC6EUF\
307+ AAIAAAAIAAAAAAAAAAAAAAAIAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD/////AAAAAAEA\
308+ AAgAAAAAAAAAAAEAAAA1AAAAaHR0cDovL3Rlc3RzdWl0ZS90c3RfV2ViVmlld19zYXZl\
309+ X3Jlc3RvcmVfZW50cnkyLmh0bWwAAAAAAAAA9eLnin6kLgAAAAAAyAAAAAEAAAAAAAAA\
310+ AgAAADUAAABodHRwOi8vdGVzdHN1aXRlL3RzdF9XZWJWaWV3X3NhdmVfcmVzdG9yZV9l\
311+ bnRyeTMuaHRtbAAAAAAAAADcAAAA2AAAABcAAAAAAAAAagAAAGgAdAB0AHAAOgAvAC8A\
312+ dABlAHMAdABzAHUAaQB0AGUALwB0AHMAdABfAFcAZQBiAFYAaQBlAHcAXwBzAGEAdgBl\
313+ AF8AcgBlAHMAdABvAHIAZQBfAGUAbgB0AHIAeQAzAC4AaAB0AG0AbAAAAP////8AAAAA\
314+ AAAAAP////8AAAAACAAAAAAAAAAAAAAAyS5gQuhFBQDKLmBC6EUFAAIAAAAIAAAAAAAA\
315+ AAAAAAAIAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD/////AAAAAAEAAAgAAAAAAAAAAAEA\
316+ AAA1AAAAaHR0cDovL3Rlc3RzdWl0ZS90c3RfV2ViVmlld19zYXZlX3Jlc3RvcmVfZW50\
317+ cnkzLmh0bWwAAAAAAAAA5o/oin6kLgAAAAAAyAAAAAEAAAAAAAAAAgAAAA=="}
318+ ];
319+ }
320+
321+ function test_WebView_restore_from_old_state(data) {
322+ var restored = webViewComponent.createObject(
323+ root,
324+ {"restoreType": WebView.RestoreCurrentSession,
325+ "restoreState": data.state});
326+ verify(restored !== null);
327+ tryCompare(restored, "url",
328+ "http://testsuite/tst_WebView_save_restore_entry3.html");
329+ verify(restored.waitForLoadSucceeded(),
330+ "Timed out waiting for successful load");
331+ compare(restored.navigationHistory.currentItemIndex, 2);
332+ var items = restored.navigationHistory.items;
333+ for (var i = 0; i < 3; ++i) {
334+ compare(items[i].url,
335+ "http://testsuite/tst_WebView_save_restore_entry%1.html"
336+ .arg(i + 1));
337+ }
338+ restored.destroy();
339+ }
340 }
341 }

Subscribers

People subscribed via source and target branches