[d8] Use ValueSerializer for postMessage (instead of ad-hoc serializer)

[d8] Use ValueSerializer for postMessage (instead of ad-hoc serializer)

Review-Url: https://codereview.chromium.org/2643723010
Cr-Commit-Position: refs/heads/master@{#42749}
Committed: https://chromium.googlesource.com/v8/v8/+/966355585bb3e6e21c063c2b670045f5a75e5aa5

[binji, 2017-01-24 03:44:37]

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

[commit-bot: I haz the power, 2017-01-24 03:44:47]

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

[binji, 2017-01-24 03:49:48]

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

[commit-bot: I haz the power, 2017-01-24 03:50:37]

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

[commit-bot: I haz the power, 2017-01-24 03:50:38]

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

[jbroman, 2017-01-24 18:53:43]

Just a few initial comments; I can have a closer look at the use of
ValueSerializer/ValueDeserializer but there are also trybot failures you might
want to look into first.

https://codereview.chromium.org/2643723010/diff/20001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2643723010/diff/20001/src/d8.cc#newcode65
src/d8.cc:65: struct hash<v8::SharedArrayBuffer::Contents> {
(I'm not a V8 owner, but...)

1. The Chromium/Google style guide recommends against specializing std::hash in
most cases.

2. It's particularly surprising to me to see std::hash and operator== declared
and defined here, as it risks ODR violations which are hard to track down. For
instance, another translation unit might define them differently (and it's
undefined how the difference is resolved). Additionally, a template might be
instantiated differently depending on whether these exist or not.

https://codereview.chromium.org/2643723010/diff/20001/src/d8.cc#newcode1176
src/d8.cc:1176: args.Length() >= 2 ? args[1] :
Local<Value>::Cast(Undefined(isolate));
nit: is the Local<Value> cast necessary; I think it's implied.

https://codereview.chromium.org/2643723010/diff/20001/src/d8.cc#newcode1195
src/d8.cc:1195: if (Shell::DeserializeValue(isolate, data).ToLocal(&value)) {
nit: Just checking, if this fails (i.e. an exception is thrown from
DeserializeValue), you're okay with an exception being thrown from here?

https://codereview.chromium.org/2643723010/diff/20001/src/d8.cc#newcode2291
src/d8.cc:2291: if (Shell::DeserializeValue(isolate, data).ToLocal(&value)) {
If an exception is thrown from DeserializeValue, don't you want to catch it?
(Blink always catches and reports null instead, but up to you.) This is probably
less of an issue for in-process messaging than cross-process messaging, but it's
still possible (e.g. if array buffer allocation fails). I suspect you want to
catch it and pass it to Shell::ReportException, but up to you.

https://codereview.chromium.org/2643723010/diff/20001/src/d8.cc#newcode2324
src/d8.cc:2324: SerializationData* data;
Here and elsewhere: managing this memory by hand makes me uneasy. Have you
considered moving to unique_ptr so that you can be sure this isn't leaked?

https://codereview.chromium.org/2643723010/diff/20001/src/d8.cc#newcode2597
src/d8.cc:2597: if (transfer_array->Get(context, i).ToLocal(&element)) {
Not new, but this isn't very resilient against code doing strange things. If Get
throws, then there will be an exception on the isolate, but serialization will
still try to proceed. I don't know the use case of d8 to know how resilient you
need to be to bad script.

e.g. this array:
var transferList = [undefined];
Object.defineProperty(transferList, '0', function() { throw 'unexpected!'; });
worker.postMessage([], transferList);

https://codereview.chromium.org/2643723010/diff/20001/src/d8.h
File src/d8.h (right):

https://codereview.chromium.org/2643723010/diff/20001/src/d8.h#newcode166
src/d8.h:166: uint8_t* data_;
This looks like it's leaked. You could use unique_ptr, vector, or at a bare
minimum freeing this in the SerializationData destructor.

[binji, 2017-01-25 00:31:36]

https://codereview.chromium.org/2643723010/diff/20001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2643723010/diff/20001/src/d8.cc#newcode65
src/d8.cc:65: struct hash<v8::SharedArrayBuffer::Contents> {
On 2017/01/24 18:53:43, jbroman wrote:
> (I'm not a V8 owner, but...)
> 
> 1. The Chromium/Google style guide recommends against specializing std::hash
in
> most cases.
> 
> 2. It's particularly surprising to me to see std::hash and operator== declared
> and defined here, as it risks ODR violations which are hard to track down. For
> instance, another translation unit might define them differently (and it's
> undefined how the difference is resolved). Additionally, a template might be
> instantiated differently depending on whether these exist or not.

Done.

https://codereview.chromium.org/2643723010/diff/20001/src/d8.cc#newcode1176
src/d8.cc:1176: args.Length() >= 2 ? args[1] :
Local<Value>::Cast(Undefined(isolate));
On 2017/01/24 18:53:43, jbroman wrote:
> nit: is the Local<Value> cast necessary; I think it's implied.

The two arms of the conditional have different types (Undefined returns
Value<Primitive>).

https://codereview.chromium.org/2643723010/diff/20001/src/d8.cc#newcode1195
src/d8.cc:1195: if (Shell::DeserializeValue(isolate, data).ToLocal(&value)) {
On 2017/01/24 18:53:43, jbroman wrote:
> nit: Just checking, if this fails (i.e. an exception is thrown from
> DeserializeValue), you're okay with an exception being thrown from here?

Yeah, it definitely will break the tests (or perhaps cause timeout), which is
all that really matters.

https://codereview.chromium.org/2643723010/diff/20001/src/d8.cc#newcode2291
src/d8.cc:2291: if (Shell::DeserializeValue(isolate, data).ToLocal(&value)) {
On 2017/01/24 18:53:43, jbroman wrote:
> If an exception is thrown from DeserializeValue, don't you want to catch it?
> (Blink always catches and reports null instead, but up to you.) This is
probably
> less of an issue for in-process messaging than cross-process messaging, but
it's
> still possible (e.g. if array buffer allocation fails). I suspect you want to
> catch it and pass it to Shell::ReportException, but up to you.

Done.

https://codereview.chromium.org/2643723010/diff/20001/src/d8.cc#newcode2324
src/d8.cc:2324: SerializationData* data;
On 2017/01/24 18:53:43, jbroman wrote:
> Here and elsewhere: managing this memory by hand makes me uneasy. Have you
> considered moving to unique_ptr so that you can be sure this isn't leaked?

At the time when I originally wrote some of this, it seemed as though d8 wasn't
using std stuff, but now it seems more OK. Agreed it's nicer :)

https://codereview.chromium.org/2643723010/diff/20001/src/d8.cc#newcode2597
src/d8.cc:2597: if (transfer_array->Get(context, i).ToLocal(&element)) {
On 2017/01/24 18:53:43, jbroman wrote:
> Not new, but this isn't very resilient against code doing strange things. If
Get
> throws, then there will be an exception on the isolate, but serialization will
> still try to proceed. I don't know the use case of d8 to know how resilient
you
> need to be to bad script.
> 
> e.g. this array:
> var transferList = [undefined];
> Object.defineProperty(transferList, '0', function() { throw 'unexpected!'; });
> worker.postMessage([], transferList);

Good point, added a TryCatch.

https://codereview.chromium.org/2643723010/diff/20001/src/d8.h
File src/d8.h (right):

https://codereview.chromium.org/2643723010/diff/20001/src/d8.h#newcode166
src/d8.h:166: uint8_t* data_;
On 2017/01/24 18:53:43, jbroman wrote:
> This looks like it's leaked. You could use unique_ptr, vector, or at a bare
> minimum freeing this in the SerializationData destructor.

Done.

https://codereview.chromium.org/2643723010/diff/60001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2643723010/diff/60001/src/d8.cc#newcode2563
src/d8.cc:2563: Shell::Exit(1);
Not sure what to do here, actually. If I pass back a null pointer,
ValueSerializer blows up (it doesn't check). But we already have a test that
fails here (mjsunit/regress/regress-crbug-514081.js).

It seems as though the only choice is to fix ValueSerializer to check this
result, but I wanted to check w/ you that I'm not missing something before I
went down this route, as it will probably propagate through a number of
functions (basically all Write functions can now fail).

[binji, 2017-01-25 01:15:31]

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

[commit-bot: I haz the power, 2017-01-25 01:15:38]

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-01-25 01:38:12]

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

[commit-bot: I haz the power, 2017-01-25 01:38:12]

Dry run: Try jobs failed on following builders:
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/19811)
  v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)

[jbroman, 2017-01-25 16:34:54]

https://codereview.chromium.org/2643723010/diff/20001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2643723010/diff/20001/src/d8.cc#newcode1176
src/d8.cc:1176: args.Length() >= 2 ? args[1] :
Local<Value>::Cast(Undefined(isolate));
On 2017/01/25 at 00:31:36, binji wrote:
> On 2017/01/24 18:53:43, jbroman wrote:
> > nit: is the Local<Value> cast necessary; I think it's implied.
> 
> The two arms of the conditional have different types (Undefined returns
Value<Primitive>).

OK, sorry.

Ordinarily the compiler would still add the conversion automatically, but
conversions in both directions are defined even though only one is legal
(neither SFINAEs away), so it gets confused. (A one-line change to the
definition of Local would fix this, but that discussion is well outside the
scope of this change.)

https://codereview.chromium.org/2643723010/diff/60001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2643723010/diff/60001/src/d8.cc#newcode2563
src/d8.cc:2563: Shell::Exit(1);
On 2017/01/25 at 00:31:36, binji wrote:
> Not sure what to do here, actually. If I pass back a null pointer,
ValueSerializer blows up (it doesn't check). But we already have a test that
fails here (mjsunit/regress/regress-crbug-514081.js).
> 
> It seems as though the only choice is to fix ValueSerializer to check this
result, but I wanted to check w/ you that I'm not missing something before I
went down this route, as it will probably propagate through a number of
functions (basically all Write functions can now fail).

Chromium usually deals with out-of-memory conditions by crashing. We could set
an arbitrary limit on the size of an individual array buffer, like your fix for
514081 did, but it seems arbitrary to me.

It seems a little unfortunate to add escape hatches to ValueSerializer for every
write to deal with this case.

[Camillo Bruni, 2017-01-25 19:06:12]

https://codereview.chromium.org/2643723010/diff/60001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2643723010/diff/60001/src/d8.cc#newcode2578
src/d8.cc:2578: if (transfer_array->Get(context, i).ToLocal(&element)) {
I think you can avoid the TryCatch here by simply returning Nothing<bool>() on
the else branch given that you rethrow.
Might miss something here as I hardly ever use the V8 API :)

https://codereview.chromium.org/2643723010/diff/60001/src/d8.cc#newcode2710
src/d8.cc:2710: return MaybeLocal<Value>();
Simplify: return deserializer.ReadValue(context);

[binji, 2017-01-26 00:46:55]

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

[commit-bot: I haz the power, 2017-01-26 00:46:57]

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

[binji, 2017-01-26 00:48:39]

https://codereview.chromium.org/2643723010/diff/60001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2643723010/diff/60001/src/d8.cc#newcode2563
src/d8.cc:2563: Shell::Exit(1);
On 2017/01/25 16:34:54, jbroman wrote:
> On 2017/01/25 at 00:31:36, binji wrote:
> > Not sure what to do here, actually. If I pass back a null pointer,
> ValueSerializer blows up (it doesn't check). But we already have a test that
> fails here (mjsunit/regress/regress-crbug-514081.js).
> > 
> > It seems as though the only choice is to fix ValueSerializer to check this
> result, but I wanted to check w/ you that I'm not missing something before I
> went down this route, as it will probably propagate through a number of
> functions (basically all Write functions can now fail).
> 
> Chromium usually deals with out-of-memory conditions by crashing. We could set
> an arbitrary limit on the size of an individual array buffer, like your fix
for
> 514081 did, but it seems arbitrary to me.
> 
> It seems a little unfortunate to add escape hatches to ValueSerializer for
every
> write to deal with this case.

Not sure how it performs, but I added a write_ok_ bool that gets set if the
reallocation fails. Didn't seem to impact the code too much. Wdyt?

https://codereview.chromium.org/2643723010/diff/60001/src/d8.cc#newcode2578
src/d8.cc:2578: if (transfer_array->Get(context, i).ToLocal(&element)) {
On 2017/01/25 19:06:12, Camillo Bruni wrote:
> I think you can avoid the TryCatch here by simply returning Nothing<bool>() on
> the else branch given that you rethrow.
> Might miss something here as I hardly ever use the V8 API :)

Done.

https://codereview.chromium.org/2643723010/diff/60001/src/d8.cc#newcode2710
src/d8.cc:2710: return MaybeLocal<Value>();
On 2017/01/25 19:06:12, Camillo Bruni wrote:
> Simplify: return deserializer.ReadValue(context);

Done.

[commit-bot: I haz the power, 2017-01-26 01:10:37]

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

[commit-bot: I haz the power, 2017-01-26 01:10:38]

Dry run: Try jobs failed on following builders:
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/19907)
  v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)

[binji, 2017-01-26 01:43:51]

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

[commit-bot: I haz the power, 2017-01-26 01:43:53]

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-01-26 02:01:41]

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

[commit-bot: I haz the power, 2017-01-26 02:01:42]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

[binji, 2017-01-26 07:54:49]

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

[commit-bot: I haz the power, 2017-01-26 07:54:54]

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-01-26 08:27:40]

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

[commit-bot: I haz the power, 2017-01-26 08:27:41]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2017-01-26 20:40:14]

I'm not that familiar with all C++ 11, jbroman is definitely more confident than
I am...
LGTM though from my side.

https://codereview.chromium.org/2643723010/diff/120001/src/d8.h
File src/d8.h (right):

https://codereview.chromium.org/2643723010/diff/120001/src/d8.h#newcode429
src/d8.h:429: std::hash<size_t>()(contents.ByteLength());
nit: use base::hash_combine from base/functional.h

[jbroman, 2017-01-26 21:03:18]

https://codereview.chromium.org/2643723010/diff/60001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2643723010/diff/60001/src/d8.cc#newcode2563
src/d8.cc:2563: Shell::Exit(1);
On 2017/01/26 at 00:48:39, binji wrote:
> On 2017/01/25 16:34:54, jbroman wrote:
> > On 2017/01/25 at 00:31:36, binji wrote:
> > > Not sure what to do here, actually. If I pass back a null pointer,
> > ValueSerializer blows up (it doesn't check). But we already have a test that
> > fails here (mjsunit/regress/regress-crbug-514081.js).
> > > 
> > > It seems as though the only choice is to fix ValueSerializer to check this
> > result, but I wanted to check w/ you that I'm not missing something before I
> > went down this route, as it will probably propagate through a number of
> > functions (basically all Write functions can now fail).
> > 
> > Chromium usually deals with out-of-memory conditions by crashing. We could
set
> > an arbitrary limit on the size of an individual array buffer, like your fix
for
> > 514081 did, but it seems arbitrary to me.
> > 
> > It seems a little unfortunate to add escape hatches to ValueSerializer for
every
> > write to deal with this case.
> 
> Not sure how it performs, but I added a write_ok_ bool that gets set if the
reallocation fails. Didn't seem to impact the code too much. Wdyt?

We should check to see if it regresses performance. The code complexity hit
isn't too bad, but it's still not trivial.

But I guess my question is, why is crashing in this case not OK? Chrome has
historically reacted to this (and other cases) by crashing, and it's surprising
to me that d8 would have a higher bar. Is fixing regress-crbug-514081.js in this
way actually protecting Chromium against bug 514081?

[binji, 2017-01-26 21:12:58]

On 2017/01/26 21:03:18, jbroman wrote:
> https://codereview.chromium.org/2643723010/diff/60001/src/d8.cc
> File src/d8.cc (right):
> 
> https://codereview.chromium.org/2643723010/diff/60001/src/d8.cc#newcode2563
> src/d8.cc:2563: Shell::Exit(1);
> On 2017/01/26 at 00:48:39, binji wrote:
> > On 2017/01/25 16:34:54, jbroman wrote:
> > > On 2017/01/25 at 00:31:36, binji wrote:
> > > > Not sure what to do here, actually. If I pass back a null pointer,
> > > ValueSerializer blows up (it doesn't check). But we already have a test
that
> > > fails here (mjsunit/regress/regress-crbug-514081.js).
> > > > 
> > > > It seems as though the only choice is to fix ValueSerializer to check
this
> > > result, but I wanted to check w/ you that I'm not missing something before
I
> > > went down this route, as it will probably propagate through a number of
> > > functions (basically all Write functions can now fail).
> > > 
> > > Chromium usually deals with out-of-memory conditions by crashing. We could
> set
> > > an arbitrary limit on the size of an individual array buffer, like your
fix
> for
> > > 514081 did, but it seems arbitrary to me.
> > > 
> > > It seems a little unfortunate to add escape hatches to ValueSerializer for
> every
> > > write to deal with this case.
> > 
> > Not sure how it performs, but I added a write_ok_ bool that gets set if the
> reallocation fails. Didn't seem to impact the code too much. Wdyt?
> 
> We should check to see if it regresses performance. 

Happy to take a look, which ones should I check?

> The code complexity hit isn't too bad, but it's still not trivial.
> 
> But I guess my question is, why is crashing in this case not OK? Chrome has
> historically reacted to this (and other cases) by crashing, and it's
surprising
> to me that d8 would have a higher bar.

Perhaps, though v8 is used outside of Chromium where maybe that isn't
appropriate behavior.

> Is fixing regress-crbug-514081.js in this way actually protecting Chromium
against bug 514081?

Well, it looks like 514081 is just a cluster-fuzz issue on the v8 side, so I
doubt it would affect Chromium.

But AIUI, crashing as a result of running a fuzzed input is never allowed.
Perhaps we could have a special error code when crashing due to OOM, but it
doesn't appear that v8 does that anywhere else.

[jbroman, 2017-01-26 21:53:03]

ok lgtm

https://codereview.chromium.org/2643723010/diff/120001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2643723010/diff/120001/src/d8.cc#newcode2692
src/d8.cc:2692: return std::unique_ptr<SerializationData>();
nit: Dunno about v8, but in Chromium/Blink I'd prefer "return nullptr" for
conciseness.

https://codereview.chromium.org/2643723010/diff/120001/src/value-serializer.cc
File src/value-serializer.cc (right):

https://codereview.chromium.org/2643723010/diff/120001/src/value-serializer.c...
src/value-serializer.cc:252: if (new_buffer && provided_capacity >=
requested_capacity) {
It seems odd to need to check both of these. Can we just pick one to signify OOM
(say, a null return value), and DCHECK the other? Could you also please update
the comment on ReallocateBuffer to specify what the expected behaviour to
signify OOM is? (It sounds like you're matching the realloc behaviour of "return
null but leave the caller responsible for freeing the buffer".)

[binji, 2017-01-27 19:33:22]

https://codereview.chromium.org/2643723010/diff/120001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2643723010/diff/120001/src/d8.cc#newcode2692
src/d8.cc:2692: return std::unique_ptr<SerializationData>();
On 2017/01/26 21:53:03, jbroman wrote:
> nit: Dunno about v8, but in Chromium/Blink I'd prefer "return nullptr" for
> conciseness.

Done.

https://codereview.chromium.org/2643723010/diff/120001/src/d8.h
File src/d8.h (right):

https://codereview.chromium.org/2643723010/diff/120001/src/d8.h#newcode429
src/d8.h:429: std::hash<size_t>()(contents.ByteLength());
On 2017/01/26 20:40:14, Camillo Bruni wrote:
> nit: use base::hash_combine from base/functional.h

Done.

https://codereview.chromium.org/2643723010/diff/120001/src/value-serializer.cc
File src/value-serializer.cc (right):

https://codereview.chromium.org/2643723010/diff/120001/src/value-serializer.c...
src/value-serializer.cc:252: if (new_buffer && provided_capacity >=
requested_capacity) {
On 2017/01/26 21:53:03, jbroman wrote:
> It seems odd to need to check both of these. Can we just pick one to signify
OOM
> (say, a null return value), and DCHECK the other? Could you also please update
> the comment on ReallocateBuffer to specify what the expected behaviour to
> signify OOM is? (It sounds like you're matching the realloc behaviour of
"return
> null but leave the caller responsible for freeing the buffer".)

Done.

[binji, 2017-01-27 19:34:00]

The CQ bit was checked by binji@chromium.org

[binji, 2017-01-27 19:34:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbroman@chromium.org, cbruni@chromium.org
Link to the patchset: https://codereview.chromium.org/2643723010/#ps160001
(title: "forgot hash_combine")

[commit-bot: I haz the power, 2017-01-27 19:34:12]

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-01-27 20:15:38]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1485545640774640,
"parent_rev": "9515f7edf0702015bf026211790cfc0e48378023", "commit_rev":
"966355585bb3e6e21c063c2b670045f5a75e5aa5"}

[commit-bot: I haz the power, 2017-01-27 20:15:43]

Description was changed from

==========
[d8] Use ValueSerializer for postMessage (instead of ad-hoc serializer)
==========

to

==========
[d8] Use ValueSerializer for postMessage (instead of ad-hoc serializer)

Review-Url: https://codereview.chromium.org/2643723010
Cr-Commit-Position: refs/heads/master@{#42749}
Committed:
https://chromium.googlesource.com/v8/v8/+/966355585bb3e6e21c063c2b670045f5a75...
==========

[commit-bot: I haz the power, 2017-01-27 20:15:44]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/v8/v8/+/966355585bb3e6e21c063c2b670045f5a75...

[Michael Achenbach, 2017-01-28 11:27:12]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/2657403002/ by machenbach@chromium.org.

The reason for reverting is: This made a regression test consistently flaky:
https://build.chromium.org/p/client.v8/builders/V8%20Linux64/builds/15618

The test allocates a lot of memory, which might explain why it passes on rerun.
Maybe make the test use less memory?.

[Michael Achenbach, 2017-01-28 11:46:55]

On 2017/01/28 11:27:12, Michael Achenbach wrote:
> A revert of this CL (patchset #9 id:160001) has been created in
> https://codereview.chromium.org/2657403002/ by mailto:machenbach@chromium.org.
> 
> The reason for reverting is: This made a regression test consistently flaky:
> https://build.chromium.org/p/client.v8/builders/V8%20Linux64/builds/15618
> 
> The test allocates a lot of memory, which might explain why it passes on
rerun.
> Maybe make the test use less memory?.

Revert doesn't want to go in. Please disable this test for now and consider
improving it somehow...

[Michael Achenbach, 2017-02-07 10:17:27]

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

[Michael Achenbach, 2017-02-07 10:17:28]

https://codereview.chromium.org/2643723010/diff/160001/test/mjsunit/regress/r...
File test/mjsunit/regress/regress-crbug-514081.js (right):

https://codereview.chromium.org/2643723010/diff/160001/test/mjsunit/regress/r...
test/mjsunit/regress/regress-crbug-514081.js:9: var ab = new
ArrayBuffer(2147483648);
Still seeing flakes of this on windows, e.g.:
https://build.chromium.org/p/client.v8/builders/V8%20Win64/builds/15656/steps...

Either passes fast or times out. Could you make this more deterministic?
Creating a too large array buffer might interfere with other test cases using
too much RAM in the same time. Also, the exceptions seem to be deliberately
non-deterministic here. You can't end up getting both, right?

Could we mock out the memory limits here with a d8 flag passed only in this
test?

[binji, 2017-02-07 18:08:25]

On 2017/02/07 10:17:28, Michael Achenbach wrote:
>
https://codereview.chromium.org/2643723010/diff/160001/test/mjsunit/regress/r...
> File test/mjsunit/regress/regress-crbug-514081.js (right):
> 
>
https://codereview.chromium.org/2643723010/diff/160001/test/mjsunit/regress/r...
> test/mjsunit/regress/regress-crbug-514081.js:9: var ab = new
> ArrayBuffer(2147483648);
> Still seeing flakes of this on windows, e.g.:
>
https://build.chromium.org/p/client.v8/builders/V8%20Win64/builds/15656/steps...
> 
> Either passes fast or times out. Could you make this more deterministic?
> Creating a too large array buffer might interfere with other test cases using
> too much RAM in the same time. Also, the exceptions seem to be deliberately
> non-deterministic here. You can't end up getting both, right?
> 
> Could we mock out the memory limits here with a d8 flag passed only in this
> test?

I'll take a look.