[webkit-reviews] review requested: [Bug 63481] Bring V8's SerializedScriptValue implementation up to date : [Attachment 100881] Promote array buffer passing test and address comments.

[bugzilla-daemon at webkit.org]

Luke Zarko <lukezarko+bugzilla at gmail.com> has asked  for review:
Bug 63481: Bring V8's SerializedScriptValue implementation up to date
https://bugs.webkit.org/show_bug.cgi?id=63481

Attachment 100881: Promote array buffer passing test and address comments.
https://bugs.webkit.org/attachment.cgi?id=100881&action=review

------- Additional Comments from Luke Zarko <lukezarko+bugzilla at gmail.com>
> (array-message-passing)

I've moved this into the common tests directory and configured expectations
appropriately.

> (Super low priority but) Want to patch one of them to make the error text the
same? (Different results are painful.)

I will look into this; I hope it won't require too many changes to
expectations.

> Do you think the ArrayBufferView::is* should change to an enum value (in a
future patch)?

Yes, to permit exactly this kind of typecasing.

> What happens if the length given here doesn't agree with the length given
when doing "newArray"?
> This algorithm in general seems very paranoid (about possible corrupted
data), but in this case depth could go negative if given a bad length but it
appears ok with that. Should it check?

length <= stackDepth() (otherwise we would have returned). Hence:
  length >= 0 (length is uint32_t), stackDepth() >= 0 (invariant)
  const int depth = stackDepth() - length;  // depth >= 0; depth + length =
stackDepth()
  for (unsigned int i = 0; i < length; ++i) // i = [0..length - 1]
      array->Set(i, element(depth + i));    // depth + i = [depth .. depth +
length - 1]
					    // depth + i = [depth ..
stackDepth() - 1]

so we'll never try to grab a bad element().

length may <> the length given to newArray if the object is corrupted or if the
array was mutated during serialization (and we didn't decide how many elements
we were going to write before we started). Our behavior then depends on
v8::Object::Set(uint32_t, v8::Handle<Value>), and we're worried about the index
>= length
case (because otherwise we get the default initialized value). If we assume
that the array is still a dense array internally, we'll eventually get down to 

JSObject::SetElementWithoutInterceptor and JSObject::SetFastElement
(v8/src/objects.cc:8263), the effect of which appears to be to rearrange the
array's storage to
fit the new element (and not to throw an exception or do something even less
pleasant). (Thanks to vegorov@ for confirming that this API sticks to JS
semantics.)

For this patch the above is kind of moot, though, because it does not address
the FIXME in newArrayState() from the old code-- so arrays are serialized as
sparse arrays and that code doesn't get hit. Another future patch in the
making!