Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1885833002:
Make messageHandlerInMainThread support main-thread worklets. (Closed)
|
Created:
4 years, 8 months ago by ikilpatrick Modified:
4 years, 8 months ago Reviewers:
haraken CC:
chromium-reviews, blink-reviews, blink-reviews-bindings_chromium.org, yhirano, jochen (gone - plz use gerrit), domenic Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake messageHandlerInMainThread support main-thread worklets.
This allows messageHandlerInMainThread to work with both a
window/document and worklet execution contexts.
This is needed for properly handling JS Errors in worklets.
BUG=602445
Committed: https://crrev.com/8e9a7753c76d9878338c6b916bdba9fa80796574
Cr-Commit-Position: refs/heads/master@{#387384}
Patch Set 1 #Patch Set 2 : simpler? #Patch Set 3 : . #
Total comments: 3
Patch Set 4 : fix all the things? #Patch Set 5 : . #Patch Set 6 : . #Patch Set 7 : . #Patch Set 8 : #Patch Set 9 : pass tests, but tests are probably wrong. #
Total comments: 10
Patch Set 10 : . #Patch Set 11 : Move check. #Patch Set 12 : rebase #
Messages
Total messages: 15 (4 generated)
ikilpatrick@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/1885833002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right): https://codereview.chromium.org/1885833002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:159: if (context->isDocument() && !toDocument(context)->domWindow()->isCurrentlyDisplayedInFrame()) Is toDocument(context)->domWindow() safe? (non-null? and equivalent to before?)
https://codereview.chromium.org/1885833002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right): https://codereview.chromium.org/1885833002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:159: if (context->isDocument() && !toDocument(context)->domWindow()->isCurrentlyDisplayedInFrame()) On 2016/04/13 04:21:09, ikilpatrick wrote: > Is toDocument(context)->domWindow() safe? (non-null? and equivalent to before?) This can change existing behavior because the entered window can be different from the ScriptState::current()->getExecutionContext()->domWindow()... Hmm. It looks like that the current code is wrong anyway. It is mixing ScriptState::current() and enteredWindow, which are different things... OK. My proposal is: - Stop using the entered window. Always use ScriptState::current(). (Actually I don't know why we've been using the entered window.) - Add the following check: ScriptState* scriptState = ScriptState::current(isolate); if (!scriptState->contextIsValid()) // This check is essentially the same as the isCurrentlyDisplayedInFrame check. return scriptState; ...; // Use scriptState as you like.
https://codereview.chromium.org/1885833002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right): https://codereview.chromium.org/1885833002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:159: if (context->isDocument() && !toDocument(context)->domWindow()->isCurrentlyDisplayedInFrame()) On 2016/04/13 04:38:48, haraken wrote: > On 2016/04/13 04:21:09, ikilpatrick wrote: > > Is toDocument(context)->domWindow() safe? (non-null? and equivalent to > before?) > > This can change existing behavior because the entered window can be different > from the ScriptState::current()->getExecutionContext()->domWindow()... > > Hmm. It looks like that the current code is wrong anyway. It is mixing > ScriptState::current() and enteredWindow, which are different things... > > OK. My proposal is: > > - Stop using the entered window. Always use ScriptState::current(). (Actually I > don't know why we've been using the entered window.) > > - Add the following check: > > ScriptState* scriptState = ScriptState::current(isolate); > if (!scriptState->contextIsValid()) // This check is essentially the same as > the isCurrentlyDisplayedInFrame check. > return scriptState; > > ...; // Use scriptState as you like. Done. Tried to clean up the other ScriptState checks as well. Now sufficiently scared about this code :). https://codereview.chromium.org/1885833002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right): https://codereview.chromium.org/1885833002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:276: LocalDOMWindow* window = currentDOMWindow(isolate); There is a series of extension tests that require this check, see patch set 8 test results: https://luci-logdog.appspot.com/v/?s=bb%2Ftryserver.chromium.linux%2Flinux_ch... Basically the tests fail on creating a newLocal of the context: #3 0x000004d6b13e blink::ScopedPersistent<>::newLocal() #4 0x000004d68543 blink::ScriptState::context() #5 0x000004d6b571 blink::ScriptState::from() #6 0x000004ef0ba6 blink::promiseRejectHandlerInMainThread() https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... How can a newLocal can fail? https://codereview.chromium.org/1885833002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:286: promiseRejectHandler(data, rejectedPromisesOnMainThread(), executionContext ? executionContext->url() : String()); The nullptr check for executionContext->url() is still needed as ScriptPromiseTest fails without it. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... It is using a TestingScriptState that doesn't have an associated ExecutionContext. In real code can that ever happen? I.e. a ScriptState not having an associated ExecutionContext? (if it can we'll need to add back in a few nullptr checks on executioncontext).
LGTM with comments. https://codereview.chromium.org/1885833002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right): https://codereview.chromium.org/1885833002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:276: LocalDOMWindow* window = currentDOMWindow(isolate); On 2016/04/13 23:26:53, ikilpatrick wrote: > There is a series of extension tests that require this check, see patch set 8 > test results: > https://luci-logdog.appspot.com/v/?s=bb%2Ftryserver.chromium.linux%2Flinux_ch... > > Basically the tests fail on creating a newLocal of the context: > #3 0x000004d6b13e blink::ScopedPersistent<>::newLocal() > #4 0x000004d68543 blink::ScriptState::context() > #5 0x000004d6b571 blink::ScriptState::from() > #6 0x000004ef0ba6 blink::promiseRejectHandlerInMainThread() > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > How can a newLocal can fail? This would be caused because promiseRejectHandlerInMainThread() is called when there is no valid ScriptState on the top of the stack. This needs to be fixed but would be a separate issue. Can we add: // TODO: Remove the check. if (!ScriptState::hasCurrentScriptState(isolate)) return; ? https://codereview.chromium.org/1885833002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:286: promiseRejectHandler(data, rejectedPromisesOnMainThread(), executionContext ? executionContext->url() : String()); On 2016/04/13 23:26:53, ikilpatrick wrote: > The nullptr check for executionContext->url() is still needed as > ScriptPromiseTest fails without it. > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > It is using a TestingScriptState that doesn't have an associated > ExecutionContext. In real code can that ever happen? I.e. a ScriptState not > having an associated ExecutionContext? > > (if it can we'll need to add back in a few nullptr checks on executioncontext). I don't know why the ScriptPromiseTest fails here. ScriptStateForTesting guarantees that scriptState->getExecutionContext() returns a valid ExecutionContext, doesn't it? https://codereview.chromium.org/1885833002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:437: if (scriptState->contextIsValid()) { I'd move the line 436 and 437 to just after line 427. We can early-return if scriptState->contextIsValid() returns false.
https://codereview.chromium.org/1885833002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right): https://codereview.chromium.org/1885833002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:276: LocalDOMWindow* window = currentDOMWindow(isolate); On 2016/04/14 02:02:00, haraken wrote: > On 2016/04/13 23:26:53, ikilpatrick wrote: > > There is a series of extension tests that require this check, see patch set 8 > > test results: > > > https://luci-logdog.appspot.com/v/?s=bb%2Ftryserver.chromium.linux%2Flinux_ch... > > > > Basically the tests fail on creating a newLocal of the context: > > #3 0x000004d6b13e blink::ScopedPersistent<>::newLocal() > > #4 0x000004d68543 blink::ScriptState::context() > > #5 0x000004d6b571 blink::ScriptState::from() > > #6 0x000004ef0ba6 blink::promiseRejectHandlerInMainThread() > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > How can a newLocal can fail? > > This would be caused because promiseRejectHandlerInMainThread() is called when > there is no valid ScriptState on the top of the stack. This needs to be fixed > but would be a separate issue. > > Can we add: > > // TODO: Remove the check. > if (!ScriptState::hasCurrentScriptState(isolate)) > return; > > ? > I tried this and doesn't appear to fix it. Thoughts? https://codereview.chromium.org/1885833002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:286: promiseRejectHandler(data, rejectedPromisesOnMainThread(), executionContext ? executionContext->url() : String()); On 2016/04/14 02:02:00, haraken wrote: > On 2016/04/13 23:26:53, ikilpatrick wrote: > > The nullptr check for executionContext->url() is still needed as > > ScriptPromiseTest fails without it. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > It is using a TestingScriptState that doesn't have an associated > > ExecutionContext. In real code can that ever happen? I.e. a ScriptState not > > having an associated ExecutionContext? > > > > (if it can we'll need to add back in a few nullptr checks on > executioncontext). > > I don't know why the ScriptPromiseTest fails here. ScriptStateForTesting > guarantees that scriptState->getExecutionContext() returns a valid > ExecutionContext, doesn't it? Dug into this a bit, ScriptStateForTesting explicitly needs an ExecutionContext set. Mosts tests do this, this one doesn't, I've modified the test. https://codereview.chromium.org/1885833002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:437: if (scriptState->contextIsValid()) { On 2016/04/14 02:02:00, haraken wrote: > > I'd move the line 436 and 437 to just after line 427. We can early-return if > scriptState->contextIsValid() returns false. > Done.
https://codereview.chromium.org/1885833002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right): https://codereview.chromium.org/1885833002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:276: LocalDOMWindow* window = currentDOMWindow(isolate); On 2016/04/14 04:09:53, ikilpatrick wrote: > On 2016/04/14 02:02:00, haraken wrote: > > On 2016/04/13 23:26:53, ikilpatrick wrote: > > > There is a series of extension tests that require this check, see patch set > 8 > > > test results: > > > > > > https://luci-logdog.appspot.com/v/?s=bb%2Ftryserver.chromium.linux%2Flinux_ch... > > > > > > Basically the tests fail on creating a newLocal of the context: > > > #3 0x000004d6b13e blink::ScopedPersistent<>::newLocal() > > > #4 0x000004d68543 blink::ScriptState::context() > > > #5 0x000004d6b571 blink::ScriptState::from() > > > #6 0x000004ef0ba6 blink::promiseRejectHandlerInMainThread() > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > How can a newLocal can fail? > > > > This would be caused because promiseRejectHandlerInMainThread() is called when > > there is no valid ScriptState on the top of the stack. This needs to be fixed > > but would be a separate issue. > > > > Can we add: > > > > // TODO: Remove the check. > > if (!ScriptState::hasCurrentScriptState(isolate)) > > return; > > > > ? > > > > I tried this and doesn't appear to fix it. Thoughts? OK, then let's keep the current code as is with a TODO. I think a right fix would be to make the to-be-rejected promise hold a ScriptState, or something. +yhirano
https://codereview.chromium.org/1885833002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right): https://codereview.chromium.org/1885833002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:276: LocalDOMWindow* window = currentDOMWindow(isolate); On 2016/04/14 04:13:17, haraken wrote: > On 2016/04/14 04:09:53, ikilpatrick wrote: > > On 2016/04/14 02:02:00, haraken wrote: > > > On 2016/04/13 23:26:53, ikilpatrick wrote: > > > > There is a series of extension tests that require this check, see patch > set > > 8 > > > > test results: > > > > > > > > > > https://luci-logdog.appspot.com/v/?s=bb%2Ftryserver.chromium.linux%2Flinux_ch... > > > > > > > > Basically the tests fail on creating a newLocal of the context: > > > > #3 0x000004d6b13e blink::ScopedPersistent<>::newLocal() > > > > #4 0x000004d68543 blink::ScriptState::context() > > > > #5 0x000004d6b571 blink::ScriptState::from() > > > > #6 0x000004ef0ba6 blink::promiseRejectHandlerInMainThread() > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > > > How can a newLocal can fail? > > > > > > This would be caused because promiseRejectHandlerInMainThread() is called > when > > > there is no valid ScriptState on the top of the stack. This needs to be > fixed > > > but would be a separate issue. > > > > > > Can we add: > > > > > > // TODO: Remove the check. > > > if (!ScriptState::hasCurrentScriptState(isolate)) > > > return; > > > > > > ? > > > > > > > I tried this and doesn't appear to fix it. Thoughts? > > OK, then let's keep the current code as is with a TODO. > > I think a right fix would be to make the to-be-rejected promise hold a > ScriptState, or something. > > +yhirano Sounds good. I updated the comment to: // TODO(ikilpatrick): Remove this check, extensions tests that use // extensions::ModuleSystemTest incorrectly don't have a valid script state.
cc: jochen@, domenic@ fyi
The CQ bit was checked by ikilpatrick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1885833002/#ps220001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1885833002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1885833002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Make messageHandlerInMainThread support main-thread worklets. This allows messageHandlerInMainThread to work with both a window/document and worklet execution contexts. This is needed for properly handling JS Errors in worklets. BUG=602445 ========== to ========== Make messageHandlerInMainThread support main-thread worklets. This allows messageHandlerInMainThread to work with both a window/document and worklet execution contexts. This is needed for properly handling JS Errors in worklets. BUG=602445 Committed: https://crrev.com/8e9a7753c76d9878338c6b916bdba9fa80796574 Cr-Commit-Position: refs/heads/master@{#387384} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/8e9a7753c76d9878338c6b916bdba9fa80796574 Cr-Commit-Position: refs/heads/master@{#387384} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
