V8ScriptValueSerializer: Add a separate version 'envelope' for Blink format version.

V8ScriptValueSerializer: Add a separate version 'envelope' for Blink format version.

With this change, Blink will write out a wire format version of its own before,
and separate from, V8's. This will enable Blink to change the format it uses
when writing DOM objects, without requiring a corresponding V8-side change to
increment the wire format version.

Versions less than 16 will continue to have only one envelope, with a version
shared between V8 and Blink.

BUG=686159
TBR=skyostil@chromium.org

Review-Url: https://codereview.chromium.org/2712783002
Cr-Commit-Position: refs/heads/master@{#453123}
Committed: https://chromium.googlesource.com/chromium/src/+/faff3132ee3d5a89a85baab94d99d80fa54559d1

[jbroman, 2017-02-22 19:56:15]

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 19:57:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbroman, 2017-02-22 20:00:45]

jbroman@chromium.org changed reviewers:
+ haraken@chromium.org

[jbroman, 2017-02-22 20:00:46]

This, plus a v8-side change to prefix all host objects encoded by Blink with a
special tag, should allow Blink and v8 to change wire format versions and
serialization tags without needing to worry about interfering with one another.
(At present, they have to worry about changing the common version number, and
about not colliding tags.)

[jbroman, 2017-02-22 21:04:03]

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 21:04:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 22:39:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 22:39:45]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jbroman, 2017-02-23 20:36:43]

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 20:37:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbroman, 2017-02-23 22:09:04]

Note that two changes were needed due to things that are sensitive to the wire
format: darin's independent implementation for WebView message ports, and the
WebCrypto tests. Both changes are small.

[commit-bot: I haz the power, 2017-02-23 22:14:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 22:14:31]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-02-24 02:45:13]

Sorry, I missed reviewing this... LGTM.

https://codereview.chromium.org/2712783002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp
(right):

https://codereview.chromium.org/2712783002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp:44:
// kMinVersionForSeparateEnveloped.

For safety, I'd prefer adding DCHECK to check that the version number is within
one byte.

[jbroman, 2017-02-24 15:38:47]

https://codereview.chromium.org/2712783002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp
(right):

https://codereview.chromium.org/2712783002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp:44:
// kMinVersionForSeparateEnveloped.
On 2017/02/24 at 02:45:13, haraken wrote:
> For safety, I'd prefer adding DCHECK to check that the version number is
within one byte.

I'm not sure I understand what unsafe behavior you would like the DCHECK to
catch. Something like DCHECK(!(rawData[1] & 0x80)) seems like it would break
when/if we reach version 128 (though the code can handle that), but I'm not sure
what bugs it would catch.

[haraken, 2017-02-25 00:14:15]

On 2017/02/24 15:38:47, jbroman wrote:
>
https://codereview.chromium.org/2712783002/diff/40001/third_party/WebKit/Sour...
> File
>
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp
> (right):
> 
>
https://codereview.chromium.org/2712783002/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueDeserializer.cpp:44:
> // kMinVersionForSeparateEnveloped.
> On 2017/02/24 at 02:45:13, haraken wrote:
> > For safety, I'd prefer adding DCHECK to check that the version number is
> within one byte.
> 
> I'm not sure I understand what unsafe behavior you would like the DCHECK to
> catch. Something like DCHECK(!(rawData[1] & 0x80)) seems like it would break
> when/if we reach version 128 (though the code can handle that), but I'm not
sure
> what bugs it would catch.

Ah, sorry, I was confused! LGTM!

[jbroman, 2017-02-25 03:38:25]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2017-02-25 03:38:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-25 03:45:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-25 03:45:16]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jbroman, 2017-02-25 06:16:42]

jbroman@chromium.org changed reviewers:
+ skyostil@chromium.org

[jbroman, 2017-02-25 06:16:43]

+skyostil for content/browser/android/

[jbroman, 2017-02-26 18:49:05]

On second thought, I'm going to TBR the content change because it's a simple
consequence of the rest of the change, and this is blocking another change.

[jbroman, 2017-02-26 18:49:14]

Description was changed from

==========
V8ScriptValueSerializer: Add a separate version 'envelope' for Blink format
version.

With this change, Blink will write out a wire format version of its own before,
and separate from, V8's. This will enable Blink to change the format it uses
when writing DOM objects, without requiring a corresponding V8-side change to
increment the wire format version.

Versions less than 16 will continue to have only one envelope, with a version
shared between V8 and Blink.

BUG=686159
==========

to

==========
V8ScriptValueSerializer: Add a separate version 'envelope' for Blink format
version.

With this change, Blink will write out a wire format version of its own before,
and separate from, V8's. This will enable Blink to change the format it uses
when writing DOM objects, without requiring a corresponding V8-side change to
increment the wire format version.

Versions less than 16 will continue to have only one envelope, with a version
shared between V8 and Blink.

BUG=686159
TBR=skyostil@chromium.org
==========

[jbroman, 2017-02-26 18:49:15]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2017-02-26 18:49:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-26 20:48:48]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488134955949450,
"parent_rev": "cf4d55cb4fdbcd75a00b70f32fbd13eda955ae2d", "commit_rev":
"faff3132ee3d5a89a85baab94d99d80fa54559d1"}

[commit-bot: I haz the power, 2017-02-26 20:49:25]

Description was changed from

==========
V8ScriptValueSerializer: Add a separate version 'envelope' for Blink format
version.

With this change, Blink will write out a wire format version of its own before,
and separate from, V8's. This will enable Blink to change the format it uses
when writing DOM objects, without requiring a corresponding V8-side change to
increment the wire format version.

Versions less than 16 will continue to have only one envelope, with a version
shared between V8 and Blink.

BUG=686159
TBR=skyostil@chromium.org
==========

to

==========
V8ScriptValueSerializer: Add a separate version 'envelope' for Blink format
version.

With this change, Blink will write out a wire format version of its own before,
and separate from, V8's. This will enable Blink to change the format it uses
when writing DOM objects, without requiring a corresponding V8-side change to
increment the wire format version.

Versions less than 16 will continue to have only one envelope, with a version
shared between V8 and Blink.

BUG=686159
TBR=skyostil@chromium.org

Review-Url: https://codereview.chromium.org/2712783002
Cr-Commit-Position: refs/heads/master@{#453123}
Committed:
https://chromium.googlesource.com/chromium/src/+/faff3132ee3d5a89a85baab94d99...
==========

[commit-bot: I haz the power, 2017-02-26 20:49:26]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/faff3132ee3d5a89a85baab94d99...

[Sami, 2017-02-27 17:35:04]

content/browser/android/ lgtm.

https://codereview.chromium.org/2712783002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2712783002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:71:
m_serializer.WriteHeader();
Should this still be inside the try-catch?

[jbroman, 2017-02-27 18:59:46]

https://codereview.chromium.org/2712783002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2712783002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:71:
m_serializer.WriteHeader();
On 2017/02/27 at 17:35:04, Sami wrote:
> Should this still be inside the try-catch?

It can't throw, so it doesn't matter.