New issue
Advanced search Search tips

Issue 625512 link

Starred by 2 users
Status: Assigned
Owner:
haraken@chromium.org
Cc:
yukishiino@chromium.org
bashi@chromium.org
lfg@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Make V8 APIs immediately crash when they return empty MaybeLocal handles

Project Member Reported by haraken@chromium.org, Jul 4 2016 
Thanks to nhiroki's work, isolate->TerminateExecution was removed from common paths of worker shutdown. This means that it's super rare that V8 APIs return empty MaybeLocal handles. We should just make the V8 APIs immediately crash if they return empty MaybeLocal handles and stop passing around the MaybeLocal handles. This will significantly clean up the code base.

Comment 1 by haraken@chromium.org, Jul 4 2016 

Uploaded a CL to measure how rare V8 APIs return empty MaybeLocal handles.

https://codereview.chromium.org/2121663002

Comment 2 by haraken@chromium.org, Aug 3 2016 

The conclusion is as follows. See this thread (https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/vGLGCaMqhCU/discussion) for more details.

a) If you want to crash:

Local<T> result = maybeLocalAPI().ToLocalChecked();
T result = maybeAPI().ToChecked();

b) If you want to handle an exception:

Local<T> result;
if (!maybeLocalAPI().ToLocal(&result)) { /* handle the exception */ }
T result;
if (!maybeAPI().To(&result)) { /* handle the exception */ }
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 4 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/120fd684181b5730f243f87189d498ce5f0a1f8d

commit 120fd684181b5730f243f87189d498ce5f0a1f8d
Author: haraken <haraken@chromium.org>
Date: Thu Aug 04 11:00:07 2016

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

Review-Url: https://codereview.chromium.org/2209593002
Cr-Commit-Position: refs/heads/master@{#409760}

[modify] https://crrev.com/120fd684181b5730f243f87189d498ce5f0a1f8d/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/120fd684181b5730f243f87189d498ce5f0a1f8d/third_party/WebKit/LayoutTests/fast/dom/create-element-after-stack-overflow-expected.txt
[modify] https://crrev.com/120fd684181b5730f243f87189d498ce5f0a1f8d/third_party/WebKit/LayoutTests/fast/dom/create-element-after-stack-overflow.html
[modify] https://crrev.com/120fd684181b5730f243f87189d498ce5f0a1f8d/third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.cpp
[modify] https://crrev.com/120fd684181b5730f243f87189d498ce5f0a1f8d/third_party/WebKit/Source/bindings/core/v8/V8DOMWrapper.cpp
[modify] https://crrev.com/120fd684181b5730f243f87189d498ce5f0a1f8d/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/120fd684181b5730f243f87189d498ce5f0a1f8d/third_party/WebKit/Source/core/dom/Node.cpp

Comment 4 by bashi@chromium.org, Aug 16 2016

Status: Assigned (was: Untriaged)
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 17 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dffcde2825d2d8bbf7d0e19e978ba95a169c147a

commit dffcde2825d2d8bbf7d0e19e978ba95a169c147a
Author: haraken <haraken@chromium.org>
Date: Wed Aug 17 10:06:18 2016

Remove ActiveDOMCallback::isScriptControllerTerminating

We stopped returning an empty handle from toV8(). Now we can remove the IsEmpty() check
for the handle returned by toV8().

Also this CL removes ActiveDOMCallback::isScriptControllerTerminating.
The check shouldn't have been necessary because if the ScriptController is terminating,
ScriptState::contextIsValid() returns false and we won't reach the check in the first place.

BUG=625512

Review-Url: https://codereview.chromium.org/2200263005
Cr-Commit-Position: refs/heads/master@{#412486}

[modify] https://crrev.com/dffcde2825d2d8bbf7d0e19e978ba95a169c147a/third_party/WebKit/Source/bindings/core/v8/ActiveDOMCallback.cpp
[modify] https://crrev.com/dffcde2825d2d8bbf7d0e19e978ba95a169c147a/third_party/WebKit/Source/bindings/core/v8/ActiveDOMCallback.h
[modify] https://crrev.com/dffcde2825d2d8bbf7d0e19e978ba95a169c147a/third_party/WebKit/Source/bindings/core/v8/V8IntersectionObserverCallback.cpp
[modify] https://crrev.com/dffcde2825d2d8bbf7d0e19e978ba95a169c147a/third_party/WebKit/Source/bindings/core/v8/V8MutationCallback.cpp
[modify] https://crrev.com/dffcde2825d2d8bbf7d0e19e978ba95a169c147a/third_party/WebKit/Source/bindings/core/v8/V8PerformanceObserverCallback.cpp
[modify] https://crrev.com/dffcde2825d2d8bbf7d0e19e978ba95a169c147a/third_party/WebKit/Source/bindings/modules/v8/V8IDBObserverCallback.cpp
[modify] https://crrev.com/dffcde2825d2d8bbf7d0e19e978ba95a169c147a/third_party/WebKit/Source/bindings/modules/v8/custom/V8CustomSQLStatementErrorCallback.cpp
[modify] https://crrev.com/dffcde2825d2d8bbf7d0e19e978ba95a169c147a/third_party/WebKit/Source/bindings/templates/callback_interface.cpp
[modify] https://crrev.com/dffcde2825d2d8bbf7d0e19e978ba95a169c147a/third_party/WebKit/Source/bindings/tests/results/core/V8TestCallbackInterface.cpp
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 24 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/829be45c2826d816879153c0134307964d402614

commit 829be45c2826d816879153c0134307964d402614
Author: haraken <haraken@chromium.org>
Date: Wed Aug 24 08:37:41 2016

Add DCHECKs to check that toV8 never returns an empty handle

BUG=625512

Review-Url: https://codereview.chromium.org/2267423003
Cr-Commit-Position: refs/heads/master@{#414023}

[modify] https://crrev.com/829be45c2826d816879153c0134307964d402614/third_party/WebKit/Source/bindings/core/v8/ToV8.h
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 26 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5a94e63aa33df1b6a6e57d48c0abebe62486a30f

commit 5a94e63aa33df1b6a6e57d48c0abebe62486a30f
Author: haraken <haraken@chromium.org>
Date: Fri Aug 26 11:55:08 2016

Make toV8(ScriptValue) never return an empty handle

I'm making toV8() never return an empty handle.
This CL replaces the return value of toV8(ScriptValue) with v8::Undefined,
but it won't change any web-exposed behavior because V8 interprets
a returned empty handle as v8::Undefined().

BUG=625512

Review-Url: https://codereview.chromium.org/2280893002
Cr-Commit-Position: refs/heads/master@{#414688}

[modify] https://crrev.com/5a94e63aa33df1b6a6e57d48c0abebe62486a30f/third_party/WebKit/Source/bindings/core/v8/ToV8.h
►

Sign in to add a comment