Stop returning an empty handle from createWrapper()

Stop returning an empty handle from createWrapper()

Function::NewInstance fails only under OOM or stack-overflow.
Hence, we can just use ToLocalChecked().

I'll remove all IsEmpty checks from toV8() in a follow-up CL.

BUG=625512
Committed: https://crrev.com/120fd684181b5730f243f87189d498ce5f0a1f8d
Cr-Commit-Position: refs/heads/master@{#409760}

[haraken, 2016-08-03 08:55:58]

Description was changed from

==========
Stop returning an empty handle from createWrapper()

Function::NewInstance fails only under OOM or stack-overflow.
Hence, we can just use ToLocalChecked().

I'll remove all IsEmpty checks from toV8() in a follow-up CL.

BUG=
==========

to

==========
Stop returning an empty handle from createWrapper()

Function::NewInstance fails only under OOM or stack-overflow.
Hence, we can just use ToLocalChecked().

I'll remove all IsEmpty checks from toV8() in a follow-up CL.

BUG=625512
==========

[haraken, 2016-08-03 08:56:13]

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

[haraken, 2016-08-03 08:56:13]

PTAL

[Yuki, 2016-08-03 08:59:05]

lgtm

[peria, 2016-08-03 08:59:35]

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

[peria, 2016-08-03 08:59:35]

lgtm

[haraken, 2016-08-03 08:59:42]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-08-03 08:59:57]

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-08-03 09:24:22]

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

[commit-bot: I haz the power, 2016-08-03 09:24:23]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[haraken, 2016-08-03 09:27:10]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-08-03 09:27:57]

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-08-03 10:00:49]

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

[commit-bot: I haz the power, 2016-08-03 10:00:50]

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

[haraken, 2016-08-03 11:06:32]

I removed fast/dom/create-element-after-stack-overflow.html and
http/tests/security/xss-DENIED-cross-origin-stack-overflow.html because they're
testing toV8() when a stack-overflow happens...

I think it would be okay to remove the tests because we're already crashing in a
ton of places when a stack-overflow happens. PTAL.

[haraken, 2016-08-03 11:06:44]

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

[commit-bot: I haz the power, 2016-08-03 11:07:02]

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, 2016-08-03 12:35:26]

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

[commit-bot: I haz the power, 2016-08-03 12:35:27]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-08-04 04:40:29]

yukishiino@: PTAL

https://codereview.chromium.org/2209593002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2209593002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:1227: crbug.com/587136
http/tests/security/xss-DENIED-cross-origin-stack-overflow.html [ Crash ]

As discussed offline, I kept the test and marked it as CRASH.

https://codereview.chromium.org/2209593002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/create-element-after-stack-overflow.html
(left):

https://codereview.chromium.org/2209593002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/create-element-after-stack-overflow.html:11:
document.createElement("marquee");

This test starts crashing after this CL, but I noticed that it's a very specific
issue to <marquee>. Since <marquee> is implemented by Blink-in-JS,
createElement("marquee") crashes when calling a JS function and hitting the
kRecursionMaxDepth check in V8ScriptRunner.cpp.

That might be problematic, but either way this test is not intending to test
Blink-in-JS under stack-overflow. So I changed <marquee> to <div>.

[Yuki, 2016-08-04 05:45:45]

LGTM with comments.

https://codereview.chromium.org/2209593002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2209593002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:1227: crbug.com/587136
http/tests/security/xss-DENIED-cross-origin-stack-overflow.html [ Crash ]
On 2016/08/04 04:40:28, haraken wrote:
> 
> As discussed offline, I kept the test and marked it as CRASH.

Could you add a comment to elaborate the test is intentionally designed to
crash.

Plus, could you close the bug and remove the bug number here?

[haraken, 2016-08-04 08:03:32]

On 2016/08/04 05:45:45, Yuki wrote:
> LGTM with comments.
> 
>
https://codereview.chromium.org/2209593002/diff/60001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/2209593002/diff/60001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/TestExpectations:1227: crbug.com/587136
> http/tests/security/xss-DENIED-cross-origin-stack-overflow.html [ Crash ]
> On 2016/08/04 04:40:28, haraken wrote:
> > 
> > As discussed offline, I kept the test and marked it as CRASH.
> 
> Could you add a comment to elaborate the test is intentionally designed to
> crash.
> 
> Plus, could you close the bug and remove the bug number here?

TestExpectation needs some bug number. I created another bug and added the
number.

[haraken, 2016-08-04 08:03:38]

The CQ bit was checked by haraken@chromium.org

[haraken, 2016-08-04 08:03:38]

The patchset sent to the CQ was uploaded after l-g-t-m from peria@chromium.org,
yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2209593002/#ps80001
(title: "temp")

[commit-bot: I haz the power, 2016-08-04 08:03:52]

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-08-04 09:29:52]

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

[commit-bot: I haz the power, 2016-08-04 09:29:54]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[haraken, 2016-08-04 09:30:31]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-08-04 09:30:49]

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-08-04 11:00:22]

Description was changed from

==========
Stop returning an empty handle from createWrapper()

Function::NewInstance fails only under OOM or stack-overflow.
Hence, we can just use ToLocalChecked().

I'll remove all IsEmpty checks from toV8() in a follow-up CL.

BUG=625512
==========

to

==========
Stop returning an empty handle from createWrapper()

Function::NewInstance fails only under OOM or stack-overflow.
Hence, we can just use ToLocalChecked().

I'll remove all IsEmpty checks from toV8() in a follow-up CL.

BUG=625512
==========

[commit-bot: I haz the power, 2016-08-04 11:00:23]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-04 11:02:25]

Description was changed from

==========
Stop returning an empty handle from createWrapper()

Function::NewInstance fails only under OOM or stack-overflow.
Hence, we can just use ToLocalChecked().

I'll remove all IsEmpty checks from toV8() in a follow-up CL.

BUG=625512
==========

to

==========
Stop returning an empty handle from createWrapper()

Function::NewInstance fails only under OOM or stack-overflow.
Hence, we can just use ToLocalChecked().

I'll remove all IsEmpty checks from toV8() in a follow-up CL.

BUG=625512

Committed: https://crrev.com/120fd684181b5730f243f87189d498ce5f0a1f8d
Cr-Commit-Position: refs/heads/master@{#409760}
==========

[commit-bot: I haz the power, 2016-08-04 11:02:27]

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

[Rick Byers, 2016-11-09 14:43:43]

On 2016/08/04 11:02:27, commit-bot: I haz the power wrote:
> Patchset 5 (id:??) landed as
> https://crrev.com/120fd684181b5730f243f87189d498ce5f0a1f8d
> Cr-Commit-Position: refs/heads/master@{#409760}

It looks like this introduced a crash:
https://bugs.chromium.org/p/chromium/issues/detail?id=644237