History API throws when serializing a SharedArrayBuffer

History API throws when serializing a SharedArrayBuffer

See https://html.spec.whatwg.org/multipage/infrastructure.html#safe-passing-of-structured-data

And the web platform test here:
https://github.com/w3c/web-platform-tests/blob/master/html/infrastructure/safe-passing-of-structured-data/shared-array-buffers/serialization-via-history.html

BUG=chromium:718193
Review-Url: https://codereview.chromium.org/2857303003
Cr-Commit-Position: refs/heads/master@{#471133}
Committed: https://chromium.googlesource.com/chromium/src/+/2c08e4da26268c0297c10e0579f3851d6c922179

[binji, 2017-05-03 23:42:50]

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

[binji, 2017-05-03 23:42:51]

I originally had the for_storage flag set inside
NativeValueTraits<SerializedScriptValue>::NativeValue directly, but I thought
that was a bit surprising.

This way at least has the generated V8History.cpp file refer to
SerializeOptions::CreateForStorage(), which is more explicit.

But I don't really like having a static creation func on SerializeOptions -- I
considered just using a normal constructor, but then the callsite would be
something like SerializeOptions(true), Which isn't great either. I could add an
enum, but then it would have to convert from the enum to bool, which is a bit
strange. I suppose we could change all uses of for_storage to the enum. What do
you think?

[binji, 2017-05-03 23:51:11]

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

[commit-bot: I haz the power, 2017-05-03 23:51:40]

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-05-04 03:40:17]

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

[commit-bot: I haz the power, 2017-05-04 03:40:18]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-05-04 09:42:56]

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

[haraken, 2017-05-04 09:42:57]

> But I don't really like having a static creation func on SerializeOptions -- I
> considered just using a normal constructor, but then the callsite would be
> something like SerializeOptions(true), Which isn't great either. I could add
an
> enum, but then it would have to convert from the enum to bool, which is a bit
> strange. I suppose we could change all uses of for_storage to the enum. What
do
> you think?

In general, an enum class is encouraged (over a boolean flag).

https://codereview.chromium.org/2857303003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/2857303003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/v8_types.py:609: elif base_idl_type
== 'SerializedScriptValue':

Won't this change affect (not only V8History.cpp but also) all DOM
attributes/methods that use SerializedScriptValue?

[binji, 2017-05-04 19:05:35]

On 2017/05/04 at 09:42:57, haraken wrote:
> > But I don't really like having a static creation func on SerializeOptions --
I
> > considered just using a normal constructor, but then the callsite would be
> > something like SerializeOptions(true), Which isn't great either. I could add
an
> > enum, but then it would have to convert from the enum to bool, which is a
bit
> > strange. I suppose we could change all uses of for_storage to the enum. What
do
> > you think?
> 
> In general, an enum class is encouraged (over a boolean flag).

OK, I'll switch this over.

> 
>
https://codereview.chromium.org/2857303003/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/scripts/v8_types.py (right):
> 
>
https://codereview.chromium.org/2857303003/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/scripts/v8_types.py:609: elif base_idl_type
== 'SerializedScriptValue':
> 
> Won't this change affect (not only V8History.cpp but also) all DOM
attributes/methods that use SerializedScriptValue?

Yes, but currently the other uses don't use
NativeValueTraits<SerializedScriptValue>::NativeValue. When I did a codesearch I
found:

Worker.postMessage
Internals.serializeObject
Client.postMessage (in serviceworkers)
History.pushState
History.replaceState
ServiceWorker.postMessage
CompositorWorker.postMessage

the calls to postMessage have their own implementations (from
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/templ...)

Hm, it looks like Internals.serializeObject uses
NativeValueTraits<SerializedScriptValue>::NativeValue too, any idea what this is
used for?

[binji, 2017-05-04 23:06:37]

On 2017/05/04 at 19:05:35, binji wrote:
> On 2017/05/04 at 09:42:57, haraken wrote:
> > > But I don't really like having a static creation func on SerializeOptions
-- I
> > > considered just using a normal constructor, but then the callsite would be
> > > something like SerializeOptions(true), Which isn't great either. I could
add an
> > > enum, but then it would have to convert from the enum to bool, which is a
bit
> > > strange. I suppose we could change all uses of for_storage to the enum.
What do
> > > you think?
> > 
> > In general, an enum class is encouraged (over a boolean flag).
> 
> OK, I'll switch this over.

Done.

> 
> > 
> >
https://codereview.chromium.org/2857303003/diff/1/third_party/WebKit/Source/b...
> > File third_party/WebKit/Source/bindings/scripts/v8_types.py (right):
> > 
> >
https://codereview.chromium.org/2857303003/diff/1/third_party/WebKit/Source/b...
> > third_party/WebKit/Source/bindings/scripts/v8_types.py:609: elif
base_idl_type == 'SerializedScriptValue':
> > 
> > Won't this change affect (not only V8History.cpp but also) all DOM
attributes/methods that use SerializedScriptValue?
> 
> Yes, but currently the other uses don't use
NativeValueTraits<SerializedScriptValue>::NativeValue. When I did a codesearch I
found:
> 
> Worker.postMessage
> Internals.serializeObject
> Client.postMessage (in serviceworkers)
> History.pushState
> History.replaceState
> ServiceWorker.postMessage
> CompositorWorker.postMessage
> 
> the calls to postMessage have their own implementations (from
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/templ...)
> 
> Hm, it looks like Internals.serializeObject uses
NativeValueTraits<SerializedScriptValue>::NativeValue too, any idea what this is
used for?

Ah, I see this is used for testing only.

[binji, 2017-05-04 23:06:45]

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

[commit-bot: I haz the power, 2017-05-04 23:07:44]

Dry run: CQ is trying da patch.

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

[haraken, 2017-05-05 01:05:18]

On 2017/05/04 23:06:37, binji wrote:
> On 2017/05/04 at 19:05:35, binji wrote:
> > On 2017/05/04 at 09:42:57, haraken wrote:
> > > > But I don't really like having a static creation func on
SerializeOptions
> -- I
> > > > considered just using a normal constructor, but then the callsite would
be
> > > > something like SerializeOptions(true), Which isn't great either. I could
> add an
> > > > enum, but then it would have to convert from the enum to bool, which is
a
> bit
> > > > strange. I suppose we could change all uses of for_storage to the enum.
> What do
> > > > you think?
> > > 
> > > In general, an enum class is encouraged (over a boolean flag).
> > 
> > OK, I'll switch this over.
> 
> Done.
> 
> > 
> > > 
> > >
>
https://codereview.chromium.org/2857303003/diff/1/third_party/WebKit/Source/b...
> > > File third_party/WebKit/Source/bindings/scripts/v8_types.py (right):
> > > 
> > >
>
https://codereview.chromium.org/2857303003/diff/1/third_party/WebKit/Source/b...
> > > third_party/WebKit/Source/bindings/scripts/v8_types.py:609: elif
> base_idl_type == 'SerializedScriptValue':
> > > 
> > > Won't this change affect (not only V8History.cpp but also) all DOM
> attributes/methods that use SerializedScriptValue?
> > 
> > Yes, but currently the other uses don't use
> NativeValueTraits<SerializedScriptValue>::NativeValue. When I did a codesearch
I
> found:
> > 
> > Worker.postMessage
> > Internals.serializeObject
> > Client.postMessage (in serviceworkers)
> > History.pushState
> > History.replaceState
> > ServiceWorker.postMessage
> > CompositorWorker.postMessage
> > 
> > the calls to postMessage have their own implementations (from
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/templ...)
> > 
> > Hm, it looks like Internals.serializeObject uses
> NativeValueTraits<SerializedScriptValue>::NativeValue too, any idea what this
is
> used for?
> 
> Ah, I see this is used for testing only.

Conceptually on which cases do you want to use
SerializedScriptValue::SerializeForStorage::kYes? If you want to use it for all
DOM attributes/methods that use SerializedScriptValue, the CL looks fine.
Otherwise (e.g., if this is a specific behavior for History) we should be more
specific about the condition in v8_types.py.

[commit-bot: I haz the power, 2017-05-05 01:06:54]

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

[commit-bot: I haz the power, 2017-05-05 01:06:56]

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_...)

[binji, 2017-05-05 19:14:32]

> Conceptually on which cases do you want to use
SerializedScriptValue::SerializeForStorage::kYes? If you want to use it for all
DOM attributes/methods that use SerializedScriptValue, the CL looks fine.
Otherwise (e.g., if this is a specific behavior for History) we should be more
specific about the condition in v8_types.py.

It should probably be specific to History, but I can't figure out the best way
to do that. In v8_types, we only know that it is a SerializedScriptValue, but I
don't think we know which interface/method it's attached to. We could use an
extended attribute, I suppose...?

[haraken, 2017-05-06 14:52:53]

On 2017/05/05 19:14:32, binji wrote:
> > Conceptually on which cases do you want to use
> SerializedScriptValue::SerializeForStorage::kYes? If you want to use it for
all
> DOM attributes/methods that use SerializedScriptValue, the CL looks fine.
> Otherwise (e.g., if this is a specific behavior for History) we should be more
> specific about the condition in v8_types.py.
> 
> It should probably be specific to History, but I can't figure out the best way
> to do that. In v8_types, we only know that it is a SerializedScriptValue, but
I
> don't think we know which interface/method it's attached to. We could use an
> extended attribute, I suppose...?

Can you hard-code 'interface_name == "History" and (attribute_name ==
"pushState" or attribute_name == "replaceState")' in v8_methods.py rather than
changing v8_types.py? Given that this is a special casing explicitly mentioned
in the HTML spec, it would be fine to hard-code it with a good comment.

I want to avoid changing v8_types.py because if someone adds a new feature that
uses SerializedScriptValue in the future, she may end up with getting a wrong
behavior.

[jbroman, 2017-05-08 14:34:02]

https://codereview.chromium.org/2857303003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h (right):

https://codereview.chromium.org/2857303003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:96: kNo,
nit: I'd prefer to call these something besides "yes" and "no". Maybe something
like:

enum class Storage {
  // Not persisted; used only during the execution of the browser.
  kEphemeral,
  // May be written to disk and read during a subsequent execution of the
browser.
  kPersistedToDisk, 
};

Or to more closely match the spec (which uses the wording "for storage" at
present):

enum StoragePolicy {
  // Not persisted; used only during the execution of the browser.
  kNotForStorage,
  // May be written to disk and read during a subsequent execution of the
browser.
  kForStorage,
};

(not super picky about names here, but something descriptive would be great)

https://codereview.chromium.org/2857303003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/2857303003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_types.py:612:
'SerializedScriptValue::SerializeOptions(SerializedScriptValue::SerializeForStorage::kYes)',
It's not apparent why this v8_value_to_cpp_value should always serialize as if
going to disk. At the very least, this needs a justifying comment, but possibly
it should be done differently (see, e.g., haraken's comments).

[jsbell, 2017-05-08 19:27:02]

jsbell@chromium.org changed reviewers:
+ jsbell@chromium.org

[jsbell, 2017-05-08 19:27:03]

https://codereview.chromium.org/2857303003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h (right):

https://codereview.chromium.org/2857303003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:96: kNo,
We should note (via comment) that this flag - whatever we call it - corresponds
to the "forStorage" flag in the spec:

https://html.spec.whatwg.org/multipage/infrastructure.html#safe-passing-of-st...

(which is a boolean BTW)

[binji, 2017-05-09 01:07:09]

> Can you hard-code 'interface_name == "History" and (attribute_name ==
"pushState" or attribute_name == "replaceState")' in v8_methods.py rather than
changing v8_types.py? Given that this is a special casing explicitly mentioned
in the HTML spec, it would be fine to hard-code it with a good comment.

I ended up doing it this way.

https://codereview.chromium.org/2857303003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h (right):

https://codereview.chromium.org/2857303003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:96: kNo,
On 2017/05/08 at 14:34:01, jbroman wrote:
> nit: I'd prefer to call these something besides "yes" and "no". Maybe
something like:
> 
> ...

Done.

https://codereview.chromium.org/2857303003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/2857303003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_types.py:612:
'SerializedScriptValue::SerializeOptions(SerializedScriptValue::SerializeForStorage::kYes)',
On 2017/05/08 at 14:34:02, jbroman wrote:
> It's not apparent why this v8_value_to_cpp_value should always serialize as if
going to disk. At the very least, this needs a justifying comment, but possibly
it should be done differently (see, e.g., haraken's comments).

I needed to keep this code for the default case, but now it does kNotForStorage
by default (which it did before).

[binji, 2017-05-09 01:07:21]

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

[commit-bot: I haz the power, 2017-05-09 01:17:45]

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-05-09 03:47:39]

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

[commit-bot: I haz the power, 2017-05-09 03:47:40]

Dry run: This issue passed the CQ dry run.

[jbroman, 2017-05-09 15:37:23]

lgtm

https://codereview.chromium.org/2857303003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/SerializedScriptValue.h
(right):

https://codereview.chromium.org/2857303003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/SerializedScriptValue.h:98:
enum class StoragePolicy {
I'd be okay with this being an ordinary enum (given its names are scoped by
SerializedScriptValue), which would make referring to these enumerators slightly
shorter. But I'm okay with this, too.

https://codereview.chromium.org/2857303003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/SerializedScriptValue.h:101:
// Maybe be written to disk and read during a subsequent execution of the
nit: "May be" or "Might be", rather than "Maybe be"

[binji, 2017-05-09 18:53:34]

https://codereview.chromium.org/2857303003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/SerializedScriptValue.h
(right):

https://codereview.chromium.org/2857303003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/SerializedScriptValue.h:98:
enum class StoragePolicy {
On 2017/05/09 at 15:37:23, jbroman wrote:
> I'd be okay with this being an ordinary enum (given its names are scoped by
SerializedScriptValue), which would make referring to these enumerators slightly
shorter. But I'm okay with this, too.

Done.

https://codereview.chromium.org/2857303003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/SerializedScriptValue.h:101:
// Maybe be written to disk and read during a subsequent execution of the
On 2017/05/09 at 15:37:23, jbroman wrote:
> nit: "May be" or "Might be", rather than "Maybe be"

Whoops! Done.

[binji, 2017-05-09 18:56:15]

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

[commit-bot: I haz the power, 2017-05-09 18:57:42]

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-05-09 21:37:44]

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

[commit-bot: I haz the power, 2017-05-09 21:37:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[haraken, 2017-05-10 00:24:37]

LGTM

[binji, 2017-05-10 20:15:10]

The CQ bit was checked by binji@chromium.org

[binji, 2017-05-10 20:15:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbroman@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2857303003/#ps80001
(title: "merge HEAD")

[commit-bot: I haz the power, 2017-05-10 20:16:52]

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-05-10 22:30:17]

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

[commit-bot: I haz the power, 2017-05-10 22:30:18]

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

[binji, 2017-05-11 01:47:10]

The CQ bit was checked by binji@chromium.org

[commit-bot: I haz the power, 2017-05-11 01:48:41]

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-05-11 04:20:44]

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

[commit-bot: I haz the power, 2017-05-11 04:20:45]

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

[binji, 2017-05-11 21:59:25]

The CQ bit was checked by binji@chromium.org

[binji, 2017-05-11 21:59:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2857303003/#ps100001
(title: "merge HEAD")

[commit-bot: I haz the power, 2017-05-11 22:00:32]

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-05-12 00:11:55]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494539965796680,
"parent_rev": "2cd6ba0a7a139061183c70387239a6fd55973094", "commit_rev":
"2c08e4da26268c0297c10e0579f3851d6c922179"}

[commit-bot: I haz the power, 2017-05-12 00:12:04]

Description was changed from

==========
History API throws when serializing a SharedArrayBuffer

See
https://html.spec.whatwg.org/multipage/infrastructure.html#safe-passing-of-st...

And the web platform test here:
https://github.com/w3c/web-platform-tests/blob/master/html/infrastructure/saf...

BUG=chromium:718193
==========

to

==========
History API throws when serializing a SharedArrayBuffer

See
https://html.spec.whatwg.org/multipage/infrastructure.html#safe-passing-of-st...

And the web platform test here:
https://github.com/w3c/web-platform-tests/blob/master/html/infrastructure/saf...

BUG=chromium:718193

Review-Url: https://codereview.chromium.org/2857303003
Cr-Commit-Position: refs/heads/master@{#471133}
Committed:
https://chromium.googlesource.com/chromium/src/+/2c08e4da26268c0297c10e0579f3...
==========

[commit-bot: I haz the power, 2017-05-12 00:12:05]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/2c08e4da26268c0297c10e0579f3...