SharedArrayBuffer throws serializing to indexeddb

SharedArrayBuffer throws serializing to indexeddb

The StructuredDeserialize spec now has a special case for serializing
for storage: it doesn't allow SharedArrayBuffers to be serialized, and
instead throws a DataCloneError.

BUG=chromium:714820
Review-Url: https://codereview.chromium.org/2838783002
Cr-Commit-Position: refs/heads/master@{#467093}
Committed: https://chromium.googlesource.com/chromium/src/+/08d35f601c7cfbdc66f9a7f8836e32028d344559

[binji, 2017-04-24 23:55:51]

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

[binji, 2017-04-24 23:56:38]

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

[commit-bot: I haz the power, 2017-04-24 23:57:01]

Dry run: CQ is trying da patch.

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

[jsbell, 2017-04-25 00:27:48]

https://codereview.chromium.org/2838783002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/LeakExpectations (right):

https://codereview.chromium.org/2838783002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/LeakExpectations:87: crbug.com/506752
virtual/sharedarraybuffer/storage/indexeddb/ [ Skip ]
Ugh... sorry about all of this overhead for running a virtual copy of
storage/indexeddb - if you want to put this test anywhere more convenient, be my
guest!

https://codereview.chromium.org/2838783002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/storage/indexeddb/shared-array-buffer-throws.html
(right):

https://codereview.chromium.org/2838783002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/shared-array-buffer-throws.html:10:
const open = indexedDB.open('db');
I know I gave you the sample code in the bug, but for the tests, please use the
indexeddb_test() helper in:

resources/testharness-helpers.js

That will ensure the database name is unique to this test case so the test won't
fail if copied/pasted, and it cleans up the test before running.

it should just be:

indexed_db_test(
  (t, db) => { 
    db.createObjectStore('store');
  },
  (t, db) => {
    const sab = new SharedArrayBuffer('256');
    const tx = db.transaction('store', 'readwrite');
    const store = tx.objectStore('store');

    assert_throws("DataCloneError", () => {
      store.put({sab: sab}, 'key');
    });
  'Serializing SharedArrayBuffer throws DataClone error.'
);

[commit-bot: I haz the power, 2017-04-25 01:32:13]

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

[commit-bot: I haz the power, 2017-04-25 01:32:14]

Dry run: This issue passed the CQ dry run.

[binji, 2017-04-25 17:37:18]

https://codereview.chromium.org/2838783002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/LeakExpectations (right):

https://codereview.chromium.org/2838783002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/LeakExpectations:87: crbug.com/506752
virtual/sharedarraybuffer/storage/indexeddb/ [ Skip ]
On 2017/04/25 at 00:27:48, jsbell wrote:
> Ugh... sorry about all of this overhead for running a virtual copy of
storage/indexeddb - if you want to put this test anywhere more convenient, be my
guest!

I guess I could move it to storage/indexeddb/shared-array-buffer, but I dunno.
Seems better to keep the test where it should probably continue to live.

https://codereview.chromium.org/2838783002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/storage/indexeddb/shared-array-buffer-throws.html
(right):

https://codereview.chromium.org/2838783002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/shared-array-buffer-throws.html:10:
const open = indexedDB.open('db');
Done.

[jbroman, 2017-04-25 17:37:23]

lgtm

https://codereview.chromium.org/2838783002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/storage/indexeddb/shared-array-buffer-throws.html
(right):

https://codereview.chromium.org/2838783002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/shared-array-buffer-throws.html:9:
const sab = new SharedArrayBuffer('256');
nit: It's rather weird to pass a string here, given that we're just going to
convert it to a number. Why not "new SharedArrayBuffer(256)"?

https://codereview.chromium.org/2838783002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/shared-array-buffer-throws.html:26:
done();
nit: I don't think you need to explicitly call done() in this case; it's
implied.

[binji, 2017-04-25 17:37:33]

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

[commit-bot: I haz the power, 2017-04-25 17:37:59]

Dry run: CQ is trying da patch.

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

[haraken, 2017-04-25 17:40:37]

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

[haraken, 2017-04-25 17:40:38]

LGTM

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

https://codereview.chromium.org/2838783002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.h:102:
bool for_storage_ = false;

Add a comment about what the flag means.

[binji, 2017-04-25 17:40:42]

The CQ bit was unchecked by binji@chromium.org

[binji, 2017-04-25 17:51:02]

Description was changed from

==========
SharedArrayBuffer throws serializing to indexeddb

The StructuredDeserialize spec now has a special case for serializing
for storage: it doesn't allow SharedArrayBuffers to be serialized, and
instead throws a DataCloneError.

BUG=chromium:714842
==========

to

==========
SharedArrayBuffer throws serializing to indexeddb

The StructuredDeserialize spec now has a special case for serializing
for storage: it doesn't allow SharedArrayBuffers to be serialized, and
instead throws a DataCloneError.

BUG=chromium:714820
==========

[binji, 2017-04-25 18:06:26]

https://codereview.chromium.org/2838783002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/storage/indexeddb/shared-array-buffer-throws.html
(right):

https://codereview.chromium.org/2838783002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/shared-array-buffer-throws.html:9:
const sab = new SharedArrayBuffer('256');
On 2017/04/25 at 17:37:23, jbroman wrote:
> nit: It's rather weird to pass a string here, given that we're just going to
convert it to a number. Why not "new SharedArrayBuffer(256)"?

Heh, I noticed this too :)
Done.

https://codereview.chromium.org/2838783002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/shared-array-buffer-throws.html:26:
done();
On 2017/04/25 at 17:37:23, jbroman wrote:
> nit: I don't think you need to explicitly call done() in this case; it's
implied.

Hm, the non-virtual testsuite version hangs when I don't do this. I think it
expects at least one test?

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

https://codereview.chromium.org/2838783002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.h:102:
bool for_storage_ = false;
On 2017/04/25 at 17:40:38, haraken wrote:
> Add a comment about what the flag means.

Done.

[binji, 2017-04-25 18:06:43]

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

[commit-bot: I haz the power, 2017-04-25 18:07:18]

Dry run: CQ is trying da patch.

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

[jbroman, 2017-04-25 18:09:42]

https://codereview.chromium.org/2838783002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/storage/indexeddb/shared-array-buffer-throws.html
(right):

https://codereview.chromium.org/2838783002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/shared-array-buffer-throws.html:26:
done();
On 2017/04/25 at 18:06:26, binji wrote:
> On 2017/04/25 at 17:37:23, jbroman wrote:
> > nit: I don't think you need to explicitly call done() in this case; it's
implied.
> 
> Hm, the non-virtual testsuite version hangs when I don't do this. I think it
expects at least one test?

Acknowledged.

[jsbell, 2017-04-25 18:45:02]

lgtm - thanks!

[commit-bot: I haz the power, 2017-04-25 19:57:47]

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

[commit-bot: I haz the power, 2017-04-25 19:57:48]

Dry run: This issue passed the CQ dry run.

[binji, 2017-04-25 20:16:43]

The CQ bit was checked by binji@chromium.org

[binji, 2017-04-25 20:16:44]

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/2838783002/#ps60001
(title: "add for_storage_ comment")

[commit-bot: I haz the power, 2017-04-25 20:17:53]

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-04-25 20:24:59]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493151403745110,
"parent_rev": "3007e36bd4c69727b84ebb459a77cd4cab906f7d", "commit_rev":
"08d35f601c7cfbdc66f9a7f8836e32028d344559"}

[commit-bot: I haz the power, 2017-04-25 20:25:34]

Description was changed from

==========
SharedArrayBuffer throws serializing to indexeddb

The StructuredDeserialize spec now has a special case for serializing
for storage: it doesn't allow SharedArrayBuffers to be serialized, and
instead throws a DataCloneError.

BUG=chromium:714820
==========

to

==========
SharedArrayBuffer throws serializing to indexeddb

The StructuredDeserialize spec now has a special case for serializing
for storage: it doesn't allow SharedArrayBuffers to be serialized, and
instead throws a DataCloneError.

BUG=chromium:714820

Review-Url: https://codereview.chromium.org/2838783002
Cr-Commit-Position: refs/heads/master@{#467093}
Committed:
https://chromium.googlesource.com/chromium/src/+/08d35f601c7cfbdc66f9a7f8836e...
==========

[commit-bot: I haz the power, 2017-04-25 20:25:35]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/08d35f601c7cfbdc66f9a7f8836e...

[gab, 2017-04-26 14:51:41]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2845623002/ by gab@chromium.org.

The reason for reverting is: Suspect for http://crbug.com/715567 (crash when
typing in hangouts chat window)..

[Jakob Kummerow, 2017-04-28 10:54:08]

On 2017/04/26 14:51:41, gab wrote:
> A revert of this CL (patchset #4 id:60001) has been created in
> https://codereview.chromium.org/2845623002/ by mailto:gab@chromium.org.
> 
> The reason for reverting is: Suspect for http://crbug.com/715567 (crash when
> typing in hangouts chat window)..

Turns out 715567 had a different cause, so from that point of view this CL can
be relanded.