V8: Add API to report OOM to embedder.

V8: Add API to report OOM to embedder.

This is a dependent CL for the blink/chromium side change in https://codereview.chromium.org/2130293003/

BUG=614440
Committed: https://crrev.com/bc44b1c627e92000cd37d46092f8bed8986f52f5
Cr-Commit-Position: refs/heads/master@{#37781}

[jochen (gone - plz use gerrit), 2016-07-12 15:11:29]

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

[jochen (gone - plz use gerrit), 2016-07-12 15:11:31]

https://codereview.chromium.org/2139873002/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2139873002/diff/1/include/v8.h#newcode6470
include/v8.h:6470: void SetOOMErrorHandler(OOMErrorCallback that));
don't add that

https://codereview.chromium.org/2139873002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2139873002/diff/1/src/api.cc#newcode332
src/api.cc:332: if (callback == NULL) {
nullptr

[Will Harris, 2016-07-12 15:42:53]

All done. thanks for taking a look already. PTAL.

https://codereview.chromium.org/2139873002/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2139873002/diff/1/include/v8.h#newcode6470
include/v8.h:6470: void SetOOMErrorHandler(OOMErrorCallback that));
On 2016/07/12 15:11:30, jochen wrote:
> don't add that

Done.

https://codereview.chromium.org/2139873002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2139873002/diff/1/src/api.cc#newcode332
src/api.cc:332: if (callback == NULL) {
On 2016/07/12 15:11:30, jochen wrote:
> nullptr

Done.

[Will Harris, 2016-07-12 15:46:10]

https://codereview.chromium.org/2139873002/diff/20001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2139873002/diff/20001/src/api.cc#newcode331
src/api.cc:331: if (callback == nullptr) {
This will change the signature of OOM errors in v8 until blink can be updated to
call the SetOOMErrorHandler function (see
https://codereview.chromium.org/2130293003/) - but the blink side can't be
committed until this lands. It's probably okay to have this inconsistency for a
short period.

[jochen (gone - plz use gerrit), 2016-07-13 08:00:49]

test

[jochen (gone - plz use gerrit), 2016-07-13 08:01:27]

jochen@chromium.org changed reviewers:
+ hpayer@chromium.org

[jochen (gone - plz use gerrit), 2016-07-13 08:01:28]

+hpayer to chime in on the OOM signature

[Hannes Payer (out of office), 2016-07-13 09:21:11]

https://codereview.chromium.org/2139873002/diff/20001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2139873002/diff/20001/src/api.cc#newcode331
src/api.cc:331: if (callback == nullptr) {
On 2016/07/12 15:46:10, Will Harris wrote:
> This will change the signature of OOM errors in v8 until blink can be updated
to
> call the SetOOMErrorHandler function (see
> https://codereview.chromium.org/2130293003/) - but the blink side can't be
> committed until this lands. It's probably okay to have this inconsistency for
a
> short period.

After landing 2130293003 the signature will change again. Why don't we keep the
current signature until then. We shouldn't change it too often because
Chromecrash, Clusterfuzz, etc. will think that a new crasher showed up.

[Will Harris, 2016-07-13 18:44:41]

On 2016/07/13 09:21:11, Hannes Payer wrote:
> https://codereview.chromium.org/2139873002/diff/20001/src/api.cc
> File src/api.cc (right):
> 
> https://codereview.chromium.org/2139873002/diff/20001/src/api.cc#newcode331
> src/api.cc:331: if (callback == nullptr) {
> On 2016/07/12 15:46:10, Will Harris wrote:
> > This will change the signature of OOM errors in v8 until blink can be
updated
> to
> > call the SetOOMErrorHandler function (see
> > https://codereview.chromium.org/2130293003/) - but the blink side can't be
> > committed until this lands. It's probably okay to have this inconsistency
for
> a
> > short period.
> 
> After landing 2130293003 the signature will change again. Why don't we keep
the
> current signature until then. We shouldn't change it too often because
> Chromecrash, Clusterfuzz, etc. will think that a new crasher showed up.

if I get lgtm on both CLs I can commit the v8 one and then wait until v8 rolls
then commit the second one before daily canary branch, and the signature
shouldn't change.

[Will Harris, 2016-07-13 18:45:46]

Description was changed from

==========
V8: Add API to report OOM to embedder.

BUG=614440
==========

to

==========
V8: Add API to report OOM to embedder.

This is a dependent CL for the blink/chromium side change in
https://codereview.chromium.org/2130293003/

BUG=614440
==========

[jochen (gone - plz use gerrit), 2016-07-13 18:54:34]

On 2016/07/13 at 18:44:41, wfh wrote:
> On 2016/07/13 09:21:11, Hannes Payer wrote:
> > https://codereview.chromium.org/2139873002/diff/20001/src/api.cc
> > File src/api.cc (right):
> > 
> > https://codereview.chromium.org/2139873002/diff/20001/src/api.cc#newcode331
> > src/api.cc:331: if (callback == nullptr) {
> > On 2016/07/12 15:46:10, Will Harris wrote:
> > > This will change the signature of OOM errors in v8 until blink can be
updated
> > to
> > > call the SetOOMErrorHandler function (see
> > > https://codereview.chromium.org/2130293003/) - but the blink side can't be
> > > committed until this lands. It's probably okay to have this inconsistency
for
> > a
> > > short period.
> > 
> > After landing 2130293003 the signature will change again. Why don't we keep
the
> > current signature until then. We shouldn't change it too often because
> > Chromecrash, Clusterfuzz, etc. will think that a new crasher showed up.
> 
> if I get lgtm on both CLs I can commit the v8 one and then wait until v8 rolls
then commit the second one before daily canary branch, and the signature
shouldn't change.

we actually have a bot that screams at you if you try to use an API that isn't
in the previous canary, because it might always happen that we have to revert a
V8 roll due to unforeseen issues, and that's only possible if the last canary
uses the same API as the current one...

[Will Harris, 2016-07-13 18:57:49]

On 2016/07/13 18:54:34, jochen wrote:
> On 2016/07/13 at 18:44:41, wfh wrote:
> > On 2016/07/13 09:21:11, Hannes Payer wrote:
> > > https://codereview.chromium.org/2139873002/diff/20001/src/api.cc
> > > File src/api.cc (right):
> > > 
> > >
https://codereview.chromium.org/2139873002/diff/20001/src/api.cc#newcode331
> > > src/api.cc:331: if (callback == nullptr) {
> > > On 2016/07/12 15:46:10, Will Harris wrote:
> > > > This will change the signature of OOM errors in v8 until blink can be
> updated
> > > to
> > > > call the SetOOMErrorHandler function (see
> > > > https://codereview.chromium.org/2130293003/) - but the blink side can't
be
> > > > committed until this lands. It's probably okay to have this
inconsistency
> for
> > > a
> > > > short period.
> > > 
> > > After landing 2130293003 the signature will change again. Why don't we
keep
> the
> > > current signature until then. We shouldn't change it too often because
> > > Chromecrash, Clusterfuzz, etc. will think that a new crasher showed up.
> > 
> > if I get lgtm on both CLs I can commit the v8 one and then wait until v8
rolls
> then commit the second one before daily canary branch, and the signature
> shouldn't change.
> 
> we actually have a bot that screams at you if you try to use an API that isn't
> in the previous canary, because it might always happen that we have to revert
a
> V8 roll due to unforeseen issues, and that's only possible if the last canary
> uses the same API as the current one...

I could make this CL first check oom_behavior() then if that's nullptr, check
exception_behavior() and if not-null, call that - then the behavior should be
the same until the blink-side lands. Once the blink-side lands I can pull out
the fallback code from v8 (i.e. it will be as it is now). This should keep the
signatures the same during the transition, and means there is no hurry to land
them at the same time.

[jochen (gone - plz use gerrit), 2016-07-13 19:38:20]

On 2016/07/13 at 18:57:49, wfh wrote:
> On 2016/07/13 18:54:34, jochen wrote:
> > On 2016/07/13 at 18:44:41, wfh wrote:
> > > On 2016/07/13 09:21:11, Hannes Payer wrote:
> > > > https://codereview.chromium.org/2139873002/diff/20001/src/api.cc
> > > > File src/api.cc (right):
> > > > 
> > > >
https://codereview.chromium.org/2139873002/diff/20001/src/api.cc#newcode331
> > > > src/api.cc:331: if (callback == nullptr) {
> > > > On 2016/07/12 15:46:10, Will Harris wrote:
> > > > > This will change the signature of OOM errors in v8 until blink can be
> > updated
> > > > to
> > > > > call the SetOOMErrorHandler function (see
> > > > > https://codereview.chromium.org/2130293003/) - but the blink side
can't be
> > > > > committed until this lands. It's probably okay to have this
inconsistency
> > for
> > > > a
> > > > > short period.
> > > > 
> > > > After landing 2130293003 the signature will change again. Why don't we
keep
> > the
> > > > current signature until then. We shouldn't change it too often because
> > > > Chromecrash, Clusterfuzz, etc. will think that a new crasher showed up.
> > > 
> > > if I get lgtm on both CLs I can commit the v8 one and then wait until v8
rolls
> > then commit the second one before daily canary branch, and the signature
> > shouldn't change.
> > 
> > we actually have a bot that screams at you if you try to use an API that
isn't
> > in the previous canary, because it might always happen that we have to
revert a
> > V8 roll due to unforeseen issues, and that's only possible if the last
canary
> > uses the same API as the current one...
> 
> I could make this CL first check oom_behavior() then if that's nullptr, check
exception_behavior() and if not-null, call that - then the behavior should be
the same until the blink-side lands. Once the blink-side lands I can pull out
the fallback code from v8 (i.e. it will be as it is now). This should keep the
signatures the same during the transition, and means there is no hurry to land
them at the same time.

sgtm

[Will Harris, 2016-07-13 20:09:45]

PTAL

https://codereview.chromium.org/2139873002/diff/20001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2139873002/diff/20001/src/api.cc#newcode331
src/api.cc:331: if (callback == nullptr) {
On 2016/07/13 09:21:11, Hannes Payer wrote:
> On 2016/07/12 15:46:10, Will Harris wrote:
> > This will change the signature of OOM errors in v8 until blink can be
updated
> to
> > call the SetOOMErrorHandler function (see
> > https://codereview.chromium.org/2130293003/) - but the blink side can't be
> > committed until this lands. It's probably okay to have this inconsistency
for
> a
> > short period.
> 
> After landing 2130293003 the signature will change again. Why don't we keep
the
> current signature until then. We shouldn't change it too often because
> Chromecrash, Clusterfuzz, etc. will think that a new crasher showed up.

now it will fall back to previous behavior if the oom handler is not set.

[Hannes Payer (out of office), 2016-07-14 08:19:54]

thanks, lgtm

[jochen (gone - plz use gerrit), 2016-07-14 15:03:30]

lgtm

[Will Harris, 2016-07-14 18:43:34]

The CQ bit was checked by wfh@chromium.org

[commit-bot: I haz the power, 2016-07-14 18:43:42]

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, 2016-07-14 18:46:31]

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

[commit-bot: I haz the power, 2016-07-14 18:46:32]

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...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/5066)

[Will Harris, 2016-07-14 19:00:38]

The CQ bit was checked by wfh@chromium.org

[Will Harris, 2016-07-14 19:00:38]

The patchset sent to the CQ was uploaded after l-g-t-m from hpayer@chromium.org,
jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2139873002/#ps80001
(title: "compile fixes")

[commit-bot: I haz the power, 2016-07-14 19:00:42]

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, 2016-07-14 19:41:09]

Description was changed from

==========
V8: Add API to report OOM to embedder.

This is a dependent CL for the blink/chromium side change in
https://codereview.chromium.org/2130293003/

BUG=614440
==========

to

==========
V8: Add API to report OOM to embedder.

This is a dependent CL for the blink/chromium side change in
https://codereview.chromium.org/2130293003/

BUG=614440
==========

[commit-bot: I haz the power, 2016-07-14 19:41:12]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-14 19:42:46]

Description was changed from

==========
V8: Add API to report OOM to embedder.

This is a dependent CL for the blink/chromium side change in
https://codereview.chromium.org/2130293003/

BUG=614440
==========

to

==========
V8: Add API to report OOM to embedder.

This is a dependent CL for the blink/chromium side change in
https://codereview.chromium.org/2130293003/

BUG=614440

Committed: https://crrev.com/bc44b1c627e92000cd37d46092f8bed8986f52f5
Cr-Commit-Position: refs/heads/master@{#37781}
==========

[commit-bot: I haz the power, 2016-07-14 19:42:47]

Patchset 5 (id:??) landed as
https://crrev.com/bc44b1c627e92000cd37d46092f8bed8986f52f5
Cr-Commit-Position: refs/heads/master@{#37781}