ScriptValueSerializer should throw, not crash, when handling unknown types

ScriptValueSerializer should throw, not crash, when handling unknown types

JavaScript Symbols are a new (in ES2015) primitive type. This caused
ScriptValueSerializer to go off the rails when it had exhausted its known
list of primitives and then assumed anything else might be an object.

Now the code checks for Object-ness before assuming it. Also refactored
the if/else-if cascade to group all Object handling together, and prune
dead branches.

BUG=522227
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200874

[adamk, 2015-08-18 22:53:17]

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

[haraken, 2015-08-19 00:04:43]

LGTM but I want to have jsbell confirm.

[jsbell, 2015-08-19 01:24:11]

Ah, the advantage of reviewing *after* the try bots have their say. :)

https://codereview.chromium.org/1297223004/diff/1/LayoutTests/fast/js/structu...
File LayoutTests/fast/js/structured-clone.html (right):

https://codereview.chromium.org/1297223004/diff/1/LayoutTests/fast/js/structu...
LayoutTests/fast/js/structured-clone.html:211: }).catch(function(ex) {
I believe this will 'catch' the exception thrown by assert_unreached; you'll
want to use the two-argument form of then() so a failing assert here will make
it to the end of the promise chain.

(Which means the cases above for Map and Set are wrong too?)

https://codereview.chromium.org/1297223004/diff/1/Source/bindings/core/v8/Scr...
File Source/bindings/core/v8/ScriptValueSerializer.cpp (right):

https://codereview.chromium.org/1297223004/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptValueSerializer.cpp:742:
m_writer.writeTransferredMessagePort(messagePortIndex);
This needs an early exit here

https://codereview.chromium.org/1297223004/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptValueSerializer.cpp:754:
m_writer.writeDate(value.As<v8::Date>()->ValueOf());
this needs an early exit, or the "return startObjectState" down below needs to
be in an `else` clause

https://codereview.chromium.org/1297223004/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptValueSerializer.cpp:756: writeStringObject(value);
ditto

https://codereview.chromium.org/1297223004/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptValueSerializer.cpp:758: writeNumberObject(value);
ditto

https://codereview.chromium.org/1297223004/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptValueSerializer.cpp:760:
writeBooleanObject(value);
ditto

https://codereview.chromium.org/1297223004/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptValueSerializer.cpp:774: writeImageData(value);
ditto

https://codereview.chromium.org/1297223004/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptValueSerializer.cpp:776: writeRegExp(value);
ditto

https://codereview.chromium.org/1297223004/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptValueSerializer.cpp:788: return 0;
nullptr, while you're here?

[adamk, 2015-08-19 20:19:35]

https://codereview.chromium.org/1297223004/diff/1/LayoutTests/fast/js/structu...
File LayoutTests/fast/js/structured-clone.html (right):

https://codereview.chromium.org/1297223004/diff/1/LayoutTests/fast/js/structu...
LayoutTests/fast/js/structured-clone.html:211: }).catch(function(ex) {
On 2015/08/19 01:24:11, jsbell wrote:
> I believe this will 'catch' the exception thrown by assert_unreached; you'll
> want to use the two-argument form of then() so a failing assert here will make
> it to the end of the promise chain.
> 
> (Which means the cases above for Map and Set are wrong too?)

Fixed here and above to use the two-arg then().

https://codereview.chromium.org/1297223004/diff/1/Source/bindings/core/v8/Scr...
File Source/bindings/core/v8/ScriptValueSerializer.cpp (right):

https://codereview.chromium.org/1297223004/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptValueSerializer.cpp:742:
m_writer.writeTransferredMessagePort(messagePortIndex);
On 2015/08/19 01:24:11, jsbell wrote:
> This needs an early exit here

Switched the logic around here to return if tryGet fails.

It's unclear to me why this tryGet failure is treated as an error while the
tryGets for ArrayBuffer and SharedArrayBuffer are not.

https://codereview.chromium.org/1297223004/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptValueSerializer.cpp:754:
m_writer.writeDate(value.As<v8::Date>()->ValueOf());
On 2015/08/19 01:24:11, jsbell wrote:
> this needs an early exit, or the "return startObjectState" down below needs to
> be in an `else` clause

Oops, didn't see that still needed to be in an 'else'. Changed back to that.

https://codereview.chromium.org/1297223004/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptValueSerializer.cpp:788: return 0;
On 2015/08/19 01:24:11, jsbell wrote:
> nullptr, while you're here?

Done.

[jsbell, 2015-08-19 20:53:15]

lgtm

https://codereview.chromium.org/1297223004/diff/1/Source/bindings/core/v8/Scr...
File Source/bindings/core/v8/ScriptValueSerializer.cpp (right):

https://codereview.chromium.org/1297223004/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptValueSerializer.cpp:742:
m_writer.writeTransferredMessagePort(messagePortIndex);
On 2015/08/19 20:19:34, adamk wrote:
> It's unclear to me why this tryGet failure is treated as an error while the
> tryGets for ArrayBuffer and SharedArrayBuffer are not.

A MessagePort can't be just cloned, it must be transferred (i.e. the original is
modified by the operation, and this must be specified by including the
MessagePort in the transferrables list). An ArrayBuffer can be just cloned or
transferred. (Ditto for SharedBuffer apparently?)

[adamk, 2015-08-19 20:54:58]

The CQ bit was checked by adamk@chromium.org

[adamk, 2015-08-19 20:54:58]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1297223004/#ps60001
(title: "Use nullptr")

[commit-bot: I haz the power, 2015-08-19 20:55:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1297223004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1297223004/60001

[commit-bot: I haz the power, 2015-08-20 00:01:39]

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

[commit-bot: I haz the power, 2015-08-20 00:01:40]

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

[adamk, 2015-08-20 01:44:43]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2015-08-20 01:44:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1297223004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1297223004/60001

[commit-bot: I haz the power, 2015-08-20 03:08:24]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200874