Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2454433002:
[Extensions + Blink] Account for user gesture in v8 function calls (Closed)
Description[Extensions + Blink] Account for user gesture in v8 function calls
Determine whether or not a user gesture is being processed when using
the SuspendableScriptExecutor to execute a v8 function.
BUG=629431
Committed: https://crrev.com/57d2116813649148c761075f528db81ec51265db
Cr-Commit-Position: refs/heads/master@{#429805}
Patch Set 1 : rebase #Patch Set 2 : hmm #
Total comments: 3
Patch Set 3 : change + test #Patch Set 4 : whoops #
Total comments: 16
Patch Set 5 : script state changes #
Total comments: 19
Patch Set 6 : more comments #
Total comments: 4
Patch Set 7 : more #
Total comments: 6
Patch Set 8 : nits #
Messages
Total messages: 65 (40 generated)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rdevlin.cronin@chromium.org changed reviewers: + dcheng@chromium.org, haraken@chromium.org
+dcheng, haraken@, please take a look when you have time. https://codereview.chromium.org/2454433002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2454433002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:128: if (m_userGesture && !UserGestureIndicator::processingUserGesture()) { I'm very unsure about this. I originally started by always creating a new gesture, as WebScriptExecutor::execute() does, but that resulted in too many new gestures on the stack. As a result, if we entered this code multiple times (which can happen easily during extension API calls), something that normally would consume the gesture (like window.open()) now only consumed a single gesture, leaving more on the stack. (You can see this test failure in patch set 1.) Is there something better to be doing here? The fact that gestures are counted, rather than a binary yes-no, is something that we don't have much in chrome code, so I'm not sure how best to handle it...
https://codereview.chromium.org/2454433002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2454433002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:128: if (m_userGesture && !UserGestureIndicator::processingUserGesture()) { On 2016/10/26 22:45:22, Devlin wrote: > I'm very unsure about this. > > I originally started by always creating a new gesture, as > WebScriptExecutor::execute() does, but that resulted in too many new gestures on > the stack. As a result, if we entered this code multiple times (which can > happen easily during extension API calls), something that normally would consume > the gesture (like window.open()) now only consumed a single gesture, leaving > more on the stack. (You can see this test failure in patch set 1.) > > Is there something better to be doing here? The fact that gestures are counted, > rather than a binary yes-no, is something that we don't have much in chrome > code, so I'm not sure how best to handle it... I believe we should be saving the original UserGestureToken and using it here. Here's an example where DOMTimer does it: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/DOM...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2454433002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2454433002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:128: if (m_userGesture && !UserGestureIndicator::processingUserGesture()) { On 2016/10/26 23:16:49, dcheng wrote: > On 2016/10/26 22:45:22, Devlin wrote: > > I'm very unsure about this. > > > > I originally started by always creating a new gesture, as > > WebScriptExecutor::execute() does, but that resulted in too many new gestures > on > > the stack. As a result, if we entered this code multiple times (which can > > happen easily during extension API calls), something that normally would > consume > > the gesture (like window.open()) now only consumed a single gesture, leaving > > more on the stack. (You can see this test failure in patch set 1.) > > > > Is there something better to be doing here? The fact that gestures are > counted, > > rather than a binary yes-no, is something that we don't have much in chrome > > code, so I'm not sure how best to handle it... > > I believe we should be saving the original UserGestureToken and using it here. > Here's an example where DOMTimer does it: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/DOM... Tried it out; ptal. https://codereview.chromium.org/2454433002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2454433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:212: ScriptState::Scope scriptScope(scriptState); This change is necessary because when the scriptScope goes out of scope, all the Locals get destroyed. Any better way to do this? (This would be cleaner if we could store ScriptState::Scope in unique_ptr or Optional, but no dice...though we could add a move ctor, perhaps to allow optional?) https://codereview.chromium.org/2454433002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/2454433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:400: EXPECT_EQ(document->body(), I'd like to add this test (and a similar one for suspended user gesture), since there don't seem to be any. But it's failing at this line. Any idea why?
https://codereview.chromium.org/2454433002/diff/100001/extensions/renderer/us... File extensions/renderer/user_gestures_native_handler.cc (right): https://codereview.chromium.org/2454433002/diff/100001/extensions/renderer/us... extensions/renderer/user_gestures_native_handler.cc:42: context()->CallFunctionSafe(v8::Local<v8::Function>::Cast(args[0]), 0, Where is CallFunctionSafe defined? I couldn't find it in code search. https://codereview.chromium.org/2454433002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2454433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:212: ScriptState::Scope scriptScope(scriptState); On 2016/10/27 23:33:28, Devlin wrote: > This change is necessary because when the scriptScope goes out of scope, all the > Locals get destroyed. Any better way to do this? What specifically do you need to keep alive? > > (This would be cleaner if we could store ScriptState::Scope in unique_ptr or > Optional, but no dice...though we could add a move ctor, perhaps to allow > optional?) ScriptState::Scope must not be used outside the machine stack.
https://codereview.chromium.org/2454433002/diff/100001/extensions/renderer/us... File extensions/renderer/user_gestures_native_handler.cc (right): https://codereview.chromium.org/2454433002/diff/100001/extensions/renderer/us... extensions/renderer/user_gestures_native_handler.cc:42: context()->CallFunctionSafe(v8::Local<v8::Function>::Cast(args[0]), 0, On 2016/10/28 08:45:14, haraken wrote: > > Where is CallFunctionSafe defined? I couldn't find it in code search. > D'oh - this is supposed to be SafeCallFunction() (https://cs.chromium.org/chromium/src/extensions/renderer/script_context.h?q=s...) - I only built webkit_unit_tests here. Will fix. The SafeCallFunction() is the version of CallFunction() that uses requestExecuteV8Function rather than callFunctionEvenIfScriptDisabled, which we are trying to get rid of. https://codereview.chromium.org/2454433002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2454433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:212: ScriptState::Scope scriptScope(scriptState); On 2016/10/28 08:45:14, haraken wrote: > On 2016/10/27 23:33:28, Devlin wrote: > > This change is necessary because when the scriptScope goes out of scope, all > the > > Locals get destroyed. Any better way to do this? > > What specifically do you need to keep alive? The results from the script execution. They're populated from the single result returned from V8ScriptRunner::CallFunction and passed to the callback, but if the ScriptState::Scope goes out of scope before the callback is called, the results are invalidated. The way it's written in this patch set works, but is kind of annoying to duplicate the callback/m_frame->client checks. > > > > (This would be cleaner if we could store ScriptState::Scope in unique_ptr or > > Optional, but no dice...though we could add a move ctor, perhaps to allow > > optional?) > > ScriptState::Scope must not be used outside the machine stack. Right. :/ Just makes it a bit more cumbersome here.
https://codereview.chromium.org/2454433002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2454433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:212: ScriptState::Scope scriptScope(scriptState); On 2016/10/28 15:00:05, Devlin wrote: > On 2016/10/28 08:45:14, haraken wrote: > > On 2016/10/27 23:33:28, Devlin wrote: > > > This change is necessary because when the scriptScope goes out of scope, all > > the > > > Locals get destroyed. Any better way to do this? > > > > What specifically do you need to keep alive? > > The results from the script execution. They're populated from the single result > returned from V8ScriptRunner::CallFunction and passed to the callback, but if > the ScriptState::Scope goes out of scope before the callback is called, the > results are invalidated. The way it's written in this patch set works, but is > kind of annoying to duplicate the callback/m_frame->client checks. What ScriptState::Scope are you referring to? Note that ScriptState::Scope is always allocated on the stack. Which means that it's not possible that ScriptState::Scope goes out of the scope before the callback has finished? If you really cannot use ScriptState::Scope for some reason, EscapableHandleScope would be what you need. > > > > > > > (This would be cleaner if we could store ScriptState::Scope in unique_ptr or > > > Optional, but no dice...though we could add a move ctor, perhaps to allow > > > optional?) > > > > ScriptState::Scope must not be used outside the machine stack. > > Right. :/ Just makes it a bit more cumbersome here.
https://codereview.chromium.org/2454433002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2454433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:212: ScriptState::Scope scriptScope(scriptState); On 2016/10/28 15:47:51, haraken wrote: > On 2016/10/28 15:00:05, Devlin wrote: > > On 2016/10/28 08:45:14, haraken wrote: > > > On 2016/10/27 23:33:28, Devlin wrote: > > > > This change is necessary because when the scriptScope goes out of scope, > all > > > the > > > > Locals get destroyed. Any better way to do this? > > > > > > What specifically do you need to keep alive? > > > > The results from the script execution. They're populated from the single > result > > returned from V8ScriptRunner::CallFunction and passed to the callback, but if > > the ScriptState::Scope goes out of scope before the callback is called, the > > results are invalidated. The way it's written in this patch set works, but is > > kind of annoying to duplicate the callback/m_frame->client checks. > > What ScriptState::Scope are you referring to? > > Note that ScriptState::Scope is always allocated on the stack. Which means that > it's not possible that ScriptState::Scope goes out of the scope before the > callback has finished? > > If you really cannot use ScriptState::Scope for some reason, > EscapableHandleScope would be what you need. > > > > > > > > > > > (This would be cleaner if we could store ScriptState::Scope in unique_ptr > or > > > > Optional, but no dice...though we could add a move ctor, perhaps to allow > > > > optional?) > > > > > > ScriptState::Scope must not be used outside the machine stack. > > > > Right. :/ Just makes it a bit more cumbersome here. > In this patch set, as its written, this works, because the ScriptState::Scope is on the stack and the scope includes the call to m_callback. As it was before this CL, it would not work, because ScriptState::Scope was instantiated in the execute() call, and m_callback was called with the results after that - so the execute() call finished, ScriptState::Scope was destroyed, and then the callback ran. Similarly, something like results; if (scriptState) { ScriptState::Scope scriptScope; results = m_executor->execute(m_frame); } else { results = m_executor->execute(m_frame); } if (!m_frame->client()) return; if (m_callback) m_callback->copmleted(results); Also won't work, because the ScriptState::Scope goes out of scope before the callback is called with the results (which were created in the given scope). As such, we're left with this - where we duplicate the m_frame->client() check and callback execution. As you mentioned, the alternative is to use an EscapableHandleScope rather than the ScriptState::Scope, but it wasn't clear to me if that would be preferred.
https://codereview.chromium.org/2454433002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2454433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:212: ScriptState::Scope scriptScope(scriptState); On 2016/10/28 16:00:00, Devlin wrote: > On 2016/10/28 15:47:51, haraken wrote: > > On 2016/10/28 15:00:05, Devlin wrote: > > > On 2016/10/28 08:45:14, haraken wrote: > > > > On 2016/10/27 23:33:28, Devlin wrote: > > > > > This change is necessary because when the scriptScope goes out of scope, > > all > > > > the > > > > > Locals get destroyed. Any better way to do this? > > > > > > > > What specifically do you need to keep alive? > > > > > > The results from the script execution. They're populated from the single > > result > > > returned from V8ScriptRunner::CallFunction and passed to the callback, but > if > > > the ScriptState::Scope goes out of scope before the callback is called, the > > > results are invalidated. The way it's written in this patch set works, but > is > > > kind of annoying to duplicate the callback/m_frame->client checks. > > > > What ScriptState::Scope are you referring to? > > > > Note that ScriptState::Scope is always allocated on the stack. Which means > that > > it's not possible that ScriptState::Scope goes out of the scope before the > > callback has finished? > > > > If you really cannot use ScriptState::Scope for some reason, > > EscapableHandleScope would be what you need. > > > > > > > > > > > > > > > (This would be cleaner if we could store ScriptState::Scope in > unique_ptr > > or > > > > > Optional, but no dice...though we could add a move ctor, perhaps to > allow > > > > > optional?) > > > > > > > > ScriptState::Scope must not be used outside the machine stack. > > > > > > Right. :/ Just makes it a bit more cumbersome here. > > > > In this patch set, as its written, this works, because the ScriptState::Scope is > on the stack and the scope includes the call to m_callback. As it was before > this CL, it would not work, because ScriptState::Scope was instantiated in the > execute() call, and m_callback was called with the results after that - so the > execute() call finished, ScriptState::Scope was destroyed, and then the callback > ran. Similarly, something like > > results; > if (scriptState) { > ScriptState::Scope scriptScope; > results = m_executor->execute(m_frame); > } else { > results = m_executor->execute(m_frame); > } > if (!m_frame->client()) > return; > if (m_callback) > m_callback->copmleted(results); > > Also won't work, because the ScriptState::Scope goes out of scope before the > callback is called with the results (which were created in the given scope). > > As such, we're left with this - where we duplicate the m_frame->client() check > and callback execution. > > As you mentioned, the alternative is to use an EscapableHandleScope rather than > the ScriptState::Scope, but it wasn't clear to me if that would be preferred. Ah, now I understand what you want to do. BTW, can we make WebScriptExecutor hold ScriptState as well? Then the complexity will be gone.
https://codereview.chromium.org/2454433002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2454433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:207: v8::HandleScope scope(v8::Isolate::GetCurrent()); Btw, assuming we combine the two separate paths, this shouldn't be needed with ScriptState::Scope used consistently for both paths. https://codereview.chromium.org/2454433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:212: ScriptState::Scope scriptScope(scriptState); On 2016/10/28 16:18:57, haraken wrote: > On 2016/10/28 16:00:00, Devlin wrote: > > On 2016/10/28 15:47:51, haraken wrote: > > > On 2016/10/28 15:00:05, Devlin wrote: > > > > On 2016/10/28 08:45:14, haraken wrote: > > > > > On 2016/10/27 23:33:28, Devlin wrote: > > > > > > This change is necessary because when the scriptScope goes out of > scope, > > > all > > > > > the > > > > > > Locals get destroyed. Any better way to do this? > > > > > > > > > > What specifically do you need to keep alive? > > > > > > > > The results from the script execution. They're populated from the single > > > result > > > > returned from V8ScriptRunner::CallFunction and passed to the callback, but > > if > > > > the ScriptState::Scope goes out of scope before the callback is called, > the > > > > results are invalidated. The way it's written in this patch set works, > but > > is > > > > kind of annoying to duplicate the callback/m_frame->client checks. > > > > > > What ScriptState::Scope are you referring to? > > > > > > Note that ScriptState::Scope is always allocated on the stack. Which means > > that > > > it's not possible that ScriptState::Scope goes out of the scope before the > > > callback has finished? > > > > > > If you really cannot use ScriptState::Scope for some reason, > > > EscapableHandleScope would be what you need. > > > > > > > > > > > > > > > > > > > (This would be cleaner if we could store ScriptState::Scope in > > unique_ptr > > > or > > > > > > Optional, but no dice...though we could add a move ctor, perhaps to > > allow > > > > > > optional?) > > > > > > > > > > ScriptState::Scope must not be used outside the machine stack. > > > > > > > > Right. :/ Just makes it a bit more cumbersome here. > > > > > > > In this patch set, as its written, this works, because the ScriptState::Scope > is > > on the stack and the scope includes the call to m_callback. As it was before > > this CL, it would not work, because ScriptState::Scope was instantiated in the > > execute() call, and m_callback was called with the results after that - so the > > execute() call finished, ScriptState::Scope was destroyed, and then the > callback > > ran. Similarly, something like > > > > results; > > if (scriptState) { > > ScriptState::Scope scriptScope; > > results = m_executor->execute(m_frame); > > } else { > > results = m_executor->execute(m_frame); > > } > > if (!m_frame->client()) > > return; > > if (m_callback) > > m_callback->copmleted(results); > > > > Also won't work, because the ScriptState::Scope goes out of scope before the > > callback is called with the results (which were created in the given scope). > > > > As such, we're left with this - where we duplicate the m_frame->client() check > > and callback execution. > > > > As you mentioned, the alternative is to use an EscapableHandleScope rather > than > > the ScriptState::Scope, but it wasn't clear to me if that would be preferred. > > Ah, now I understand what you want to do. > > BTW, can we make WebScriptExecutor hold ScriptState as well? Then the complexity > will be gone. Hmm. Why does WebScriptExecutor not need the ScriptState, while V8FunctionExecutor does? I agree with haraken@ that it would be nicer if we could combine the two separate paths. https://codereview.chromium.org/2454433002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/2454433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:400: EXPECT_EQ(document->body(), On 2016/10/27 23:33:28, Devlin wrote: > I'd like to add this test (and a similar one for suspended user gesture), since > there don't seem to be any. But it's failing at this line. Any idea why? I think that's because there's no user gesture active, right? If the test puts a user-gesture indicator on the stack, that should make this pass: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/tests/WebF...
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
If at any point this seems like something the blink team should be working on, lemme know, and I'll step back. :) https://codereview.chromium.org/2454433002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2454433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:207: v8::HandleScope scope(v8::Isolate::GetCurrent()); On 2016/10/28 17:24:17, dcheng wrote: > Btw, assuming we combine the two separate paths, this shouldn't be needed with > ScriptState::Scope used consistently for both paths. Done. https://codereview.chromium.org/2454433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:212: ScriptState::Scope scriptScope(scriptState); On 2016/10/28 17:24:17, dcheng wrote: > On 2016/10/28 16:18:57, haraken wrote: > > On 2016/10/28 16:00:00, Devlin wrote: > > > On 2016/10/28 15:47:51, haraken wrote: > > > > On 2016/10/28 15:00:05, Devlin wrote: > > > > > On 2016/10/28 08:45:14, haraken wrote: > > > > > > On 2016/10/27 23:33:28, Devlin wrote: > > > > > > > This change is necessary because when the scriptScope goes out of > > scope, > > > > all > > > > > > the > > > > > > > Locals get destroyed. Any better way to do this? > > > > > > > > > > > > What specifically do you need to keep alive? > > > > > > > > > > The results from the script execution. They're populated from the > single > > > > result > > > > > returned from V8ScriptRunner::CallFunction and passed to the callback, > but > > > if > > > > > the ScriptState::Scope goes out of scope before the callback is called, > > the > > > > > results are invalidated. The way it's written in this patch set works, > > but > > > is > > > > > kind of annoying to duplicate the callback/m_frame->client checks. > > > > > > > > What ScriptState::Scope are you referring to? > > > > > > > > Note that ScriptState::Scope is always allocated on the stack. Which means > > > that > > > > it's not possible that ScriptState::Scope goes out of the scope before the > > > > callback has finished? > > > > > > > > If you really cannot use ScriptState::Scope for some reason, > > > > EscapableHandleScope would be what you need. > > > > > > > > > > > > > > > > > > > > > > > (This would be cleaner if we could store ScriptState::Scope in > > > unique_ptr > > > > or > > > > > > > Optional, but no dice...though we could add a move ctor, perhaps to > > > allow > > > > > > > optional?) > > > > > > > > > > > > ScriptState::Scope must not be used outside the machine stack. > > > > > > > > > > Right. :/ Just makes it a bit more cumbersome here. > > > > > > > > > > In this patch set, as its written, this works, because the > ScriptState::Scope > > is > > > on the stack and the scope includes the call to m_callback. As it was > before > > > this CL, it would not work, because ScriptState::Scope was instantiated in > the > > > execute() call, and m_callback was called with the results after that - so > the > > > execute() call finished, ScriptState::Scope was destroyed, and then the > > callback > > > ran. Similarly, something like > > > > > > results; > > > if (scriptState) { > > > ScriptState::Scope scriptScope; > > > results = m_executor->execute(m_frame); > > > } else { > > > results = m_executor->execute(m_frame); > > > } > > > if (!m_frame->client()) > > > return; > > > if (m_callback) > > > m_callback->copmleted(results); > > > > > > Also won't work, because the ScriptState::Scope goes out of scope before the > > > callback is called with the results (which were created in the given scope). > > > > > > As such, we're left with this - where we duplicate the m_frame->client() > check > > > and callback execution. > > > > > > As you mentioned, the alternative is to use an EscapableHandleScope rather > > than > > > the ScriptState::Scope, but it wasn't clear to me if that would be > preferred. > > > > Ah, now I understand what you want to do. > > > > BTW, can we make WebScriptExecutor hold ScriptState as well? Then the > complexity > > will be gone. > > Hmm. Why does WebScriptExecutor not need the ScriptState, while > V8FunctionExecutor does? I agree with haraken@ that it would be nicer if we > could combine the two separate paths. WebScriptExecutor uses ScriptController::executeScript*(), which handle the script state, so that's why it didn't already need it. I've made WebScriptExecutor also return the ScriptState to avoid some of the added complexity, though getting at that script state is a little complicated in and of itself. https://codereview.chromium.org/2454433002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/2454433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:400: EXPECT_EQ(document->body(), On 2016/10/28 17:24:17, dcheng wrote: > On 2016/10/27 23:33:28, Devlin wrote: > > I'd like to add this test (and a similar one for suspended user gesture), > since > > there don't seem to be any. But it's failing at this line. Any idea why? > > I think that's because there's no user gesture active, right? If the test puts a > user-gesture indicator on the stack, that should make this pass: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/tests/WebF... the "true" parameter in the requestExecuteScriptAndReturnValue means that it's for a user gesture, which puts a gesture on the stack. To be extra sure, I've added another token here. Neither make a difference - this still doesn't pass. My guess is it's something weird with using body rather than a different element (which is what other tests seem to be doing), but I'm not sure why that is. https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:143: // should still be being processed. Continuing the crazy fun. If we pass m_gestureToken to a UserGestureIndicator, and that indicator goes out of scope, it removes the current gestureToken. This means that something like: ScopedUserGesture gesture; execute() // has gesture re-entrant execute() // has gesture, but then Indicator goes out of scope re-entrant execute() // doesn't have gesture happens, which is a problem given how much extensions code can bounce around. I'm not sure this is the right fix, vs having UserGestureIndicator keep a count of active indicators, but the latter is out of scope of this change.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
Mostly looks good. https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:83: RefPtr<DOMWrapperWorld> world = DOMWrapperWorld::ensureIsolatedWorld( Can you implement DOMWwrapperWorld::fromWorldId(int worldId), which returns a corresponding world? Then we can remove the if(!m_world) check. https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:86: return windowProxy->isContextInitialized() ? windowProxy->getScriptState() You can just use ScriptState::forWorld(world).
https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:33: ScriptState* getScriptState(LocalFrame*) override; It is possible to bind ScriptState when we create the SuspendableScriptExecutor? Then we won't need this virtual. https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:143: // should still be being processed. On 2016/10/28 20:31:37, Devlin wrote: > Continuing the crazy fun. > > If we pass m_gestureToken to a UserGestureIndicator, and that indicator goes out > of scope, it removes the current gestureToken. This means that something like: > > ScopedUserGesture gesture; > execute() // has gesture > re-entrant execute() // has gesture, but then Indicator goes out of scope > re-entrant execute() // doesn't have gesture > > happens, which is a problem given how much extensions code can bounce around. > I'm not sure this is the right fix, vs having UserGestureIndicator keep a count > of active indicators, but the latter is out of scope of this change. I feel like it would be surprising if this were dispatched re-entrantly. Is there a concrete case where this can happen? https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:232: if (scriptState && scriptState->contextIsValid()) { Is it possible to get here if there is not valid ScriptState? It's a SuspendableTimer, which shouldn't execute if the tracked context is destroyed, I think. https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:238: if (!m_frame->client()) Can we just check lifecycleContext() here? https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:242: if (m_callback) Nit: is it possible to remove this if ()? It seems like valid uses should generally pass in a callback. https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.h (right): https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.h:68: Member<LocalFrame> m_frame; Sorry I missed this in the original review, but let's see if we can clean up |m_frame| at some point (either in this CL or a followup). It's redundant with being a SuspendableTimer, since that already has a pointer to the ExecutionContext.
Just replying - will address other comments on Monday. https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:33: ScriptState* getScriptState(LocalFrame*) override; On 2016/10/29 04:36:21, dcheng wrote: > It is possible to bind ScriptState when we create the SuspendableScriptExecutor? > Then we won't need this virtual. Potentially (and I was going to do this originally), but in the WebScriptExecutor case, the isolated world may not exist before we try to execute the script. I wasn't sure it would be a good idea to instantiate it before we are going to actually execute the script - is that a concern? https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:143: // should still be being processed. On 2016/10/29 04:36:21, dcheng wrote: > On 2016/10/28 20:31:37, Devlin wrote: > > Continuing the crazy fun. > > > > If we pass m_gestureToken to a UserGestureIndicator, and that indicator goes > out > > of scope, it removes the current gestureToken. This means that something > like: > > > > ScopedUserGesture gesture; > > execute() // has gesture > > re-entrant execute() // has gesture, but then Indicator goes out of scope > > re-entrant execute() // doesn't have gesture > > > > happens, which is a problem given how much extensions code can bounce around. > > I'm not sure this is the right fix, vs having UserGestureIndicator keep a > count > > of active indicators, but the latter is out of scope of this change. > > I feel like it would be surprising if this were dispatched re-entrantly. Is > there a concrete case where this can happen? Extensions code can do this, given how many times we execute JS in the course of our bindings, particularly since we are trying to make this the only way we execute scripts (rather than executeEvenIfScriptDisabled). ExtensionApiTest.OptionalPermissionsRetainGesture does this by doing chrome.test.runWithUserGesture(function() { chrome.permissions.request(...); }); Each of these steps triggers code that calls into here (multiple times), causing the user gesture issue described above. In the (very) long run, migrating our bindings to C++ should remove this need. But a) I don't think that we want to wait for that, and b) it seems like this would be expected behavior anyway. If I do ScopedUserGesture gesture; DoStuff(); Unless something in DoStuff() *consumes* the user gesture (nothing here does), I would expect the user gesture to persist throughout DoStuff(). https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:242: if (m_callback) On 2016/10/29 04:36:21, dcheng wrote: > Nit: is it possible to remove this if ()? It seems like valid uses should > generally pass in a callback. Extensions code can often do this, since we only care about executing the code and don't necessarily expect a result.
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2454433002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/2454433002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:400: EXPECT_EQ(document->body(), On 2016/10/28 20:31:37, Devlin wrote: > On 2016/10/28 17:24:17, dcheng wrote: > > On 2016/10/27 23:33:28, Devlin wrote: > > > I'd like to add this test (and a similar one for suspended user gesture), > > since > > > there don't seem to be any. But it's failing at this line. Any idea why? > > > > I think that's because there's no user gesture active, right? If the test puts > a > > user-gesture indicator on the stack, that should make this pass: > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/tests/WebF... > > the "true" parameter in the requestExecuteScriptAndReturnValue means that it's > for a user gesture, which puts a gesture on the stack. To be extra sure, I've > added another token here. Neither make a difference - this still doesn't pass. > My guess is it's something weird with using body rather than a different element > (which is what other tests seem to be doing), but I'm not sure why that is. FYI, I've removed this test since making it work was non-trivial and it's orthogonal to this change. https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:83: RefPtr<DOMWrapperWorld> world = DOMWrapperWorld::ensureIsolatedWorld( On 2016/10/29 01:41:06, haraken wrote: > > Can you implement DOMWwrapperWorld::fromWorldId(int worldId), which returns a > corresponding world? Then we can remove the if(!m_world) check. > Done. It's a little awkward because it requires passing in the isolate and extension group, but it works. https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:86: return windowProxy->isContextInitialized() ? windowProxy->getScriptState() On 2016/10/29 01:41:06, haraken wrote: > > You can just use ScriptState::forWorld(world). Ah, handy. Done. https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:232: if (scriptState && scriptState->contextIsValid()) { On 2016/10/29 04:36:21, dcheng wrote: > Is it possible to get here if there is not valid ScriptState? It's a > SuspendableTimer, which shouldn't execute if the tracked context is destroyed, I > think. I'd prefer to not remove any checks just yet since there's a lot of changes going on here, but I've put in a TODO and can change this to an assert in a followup. https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:238: if (!m_frame->client()) On 2016/10/29 04:36:21, dcheng wrote: > Can we just check lifecycleContext() here? I think so. Done. https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.h (right): https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.h:68: Member<LocalFrame> m_frame; On 2016/10/29 04:36:21, dcheng wrote: > Sorry I missed this in the original review, but let's see if we can clean up > |m_frame| at some point (either in this CL or a followup). It's redundant with > being a SuspendableTimer, since that already has a pointer to the > ExecutionContext. TODO added; will fix in a followup.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
As for the UGI stuff, let me try to find someone who knows what the plans in this area are, I've seen things changing lately. I guess the DOMTimer model isn't a great fit, since that shouldn't run tasks re-entrantly, so we might have to figure out something else fo rthis. https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:33: ScriptState* getScriptState(LocalFrame*) override; On 2016/10/29 17:45:50, Devlin wrote: > On 2016/10/29 04:36:21, dcheng wrote: > > It is possible to bind ScriptState when we create the > SuspendableScriptExecutor? > > Then we won't need this virtual. > > Potentially (and I was going to do this originally), but in the > WebScriptExecutor case, the isolated world may not exist before we try to > execute the script. I wasn't sure it would be a good idea to instantiate it > before we are going to actually execute the script - is that a concern? I think that this is OK, and will be simpler overall. Most of the time, we're going to be executing the script shortly afterwards anyway. https://codereview.chromium.org/2454433002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2454433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:230: if (scriptState && scriptState->contextIsValid()) { Are we planning on merging these patches? If not, I'd rather just DCHECK(). Hitting this DCHECK() would mean that something is really wrong.
japhet@chromium.org changed reviewers: + japhet@chromium.org
https://codereview.chromium.org/2454433002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2454433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:134: // It's important that we don't pass the gesture if it's already the Oops, you found a bug in UserGestureIndicator. Rather than doing this, there should probably be an early exit in UserGestureIndicator's constructor for m_token == s_rootToken, which should allow you to beningly initialize a UserGestureIndicator here without worrying about clobbering s_rootToken.
https://codereview.chromium.org/2454433002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2454433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:134: // It's important that we don't pass the gesture if it's already the On 2016/11/01 17:40:18, Nate Chapin wrote: > Oops, you found a bug in UserGestureIndicator. Rather than doing this, there > should probably be an early exit in UserGestureIndicator's constructor for > m_token == s_rootToken, which should allow you to beningly initialize a > UserGestureIndicator here without worrying about clobbering s_rootToken. sgtm. I'll do that in a separate CL and send it your way.
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
haraken + dcheng, mind taking another look? https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2454433002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:33: ScriptState* getScriptState(LocalFrame*) override; On 2016/11/01 00:14:05, dcheng wrote: > On 2016/10/29 17:45:50, Devlin wrote: > > On 2016/10/29 04:36:21, dcheng wrote: > > > It is possible to bind ScriptState when we create the > > SuspendableScriptExecutor? > > > Then we won't need this virtual. > > > > Potentially (and I was going to do this originally), but in the > > WebScriptExecutor case, the isolated world may not exist before we try to > > execute the script. I wasn't sure it would be a good idea to instantiate it > > before we are going to actually execute the script - is that a concern? > > I think that this is OK, and will be simpler overall. Most of the time, we're > going to be executing the script shortly afterwards anyway. Done. https://codereview.chromium.org/2454433002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2454433002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:230: if (scriptState && scriptState->contextIsValid()) { On 2016/11/01 00:14:05, dcheng wrote: > Are we planning on merging these patches? If not, I'd rather just DCHECK(). > Hitting this DCHECK() would mean that something is really wrong. Sounds reasonable, CHECK()ing (given the ambiguity, I'd rather be sure, so opted for that over a DCHECK).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM FWIW, I would still personally prefer a non-null callback (since we have to sprinkle null checks in several places now), but I guess this is OK too. (One other thought is to have a dummy no-op callback internally and have it dispatch to that if no callback was supplied, then we can save having to null check it in different places)
LGTM https://codereview.chromium.org/2454433002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right): https://codereview.chromium.org/2454433002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:143: frame, *DOMWrapperWorld::fromWorldId(v8::Isolate::GetCurrent(), worldID, Use toIsolate(frame), or explicitly pass in v8::Isolate* to this method (which is better). https://codereview.chromium.org/2454433002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:217: if (!lifecycleContext()) This will work but I'd prefer checking m_scriptState->contextIsValid() instead. https://codereview.chromium.org/2454433002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:220: // We need to call the callback before scriptScope goes out of scope. This comment doesn't make much sense now.
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> FWIW, I would still personally prefer a non-null callback (since we have to
> sprinkle null checks in several places now), but I guess this is OK too.
>
> (One other thought is to have a dummy no-op callback internally and have it
> dispatch to that if no callback was supplied, then we can save having to null
> check it in different places)
I'll save this for a followup, since it's very unrelated to this specific CL,
but will respond a bit here.
My biggest concern with not allowing a null callback here is that the "callback"
object is more like a delegate object that has to manage its own lifetime, which
makes the idea of having a dummy version a little more onerous. If we can pass
in a base::Callback (where we could just do base::Bind(&DoNothing)), that's a
lot more appealing than subclassing the delegate to have a OnDone() { delete
this; } impl. Are base::Callback's allowed in general WebKit code yet? If so,
this fix up is pretty trivial.
https://codereview.chromium.org/2454433002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp (right):
https://codereview.chromium.org/2454433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:143: frame,
*DOMWrapperWorld::fromWorldId(v8::Isolate::GetCurrent(), worldID,
On 2016/11/03 14:37:04, haraken wrote:
>
> Use toIsolate(frame), or explicitly pass in v8::Isolate* to this method (which
> is better).
Went with toIsolate(frame) for now, but added a TODO to update. I'm trying to
keep the churn low in this CL (though that ship may have already sailed).
https://codereview.chromium.org/2454433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:217: if
(!lifecycleContext())
On 2016/11/03 14:37:04, haraken wrote:
>
> This will work but I'd prefer checking m_scriptState->contextIsValid()
instead.
Done.
https://codereview.chromium.org/2454433002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp:220: // We need to
call the callback before scriptScope goes out of scope.
On 2016/11/03 14:37:04, haraken wrote:
>
> This comment doesn't make much sense now.
Whoops, done.
The CQ bit was unchecked by rdevlin.cronin@chromium.org
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2454433002/#ps180001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [Extensions + Blink] Account for user gesture in v8 function calls Determine whether or not a user gesture is being processed when using the SuspendableScriptExecutor to execute a v8 function. BUG=629431 ========== to ========== [Extensions + Blink] Account for user gesture in v8 function calls Determine whether or not a user gesture is being processed when using the SuspendableScriptExecutor to execute a v8 function. BUG=629431 Committed: https://crrev.com/57d2116813649148c761075f528db81ec51265db Cr-Commit-Position: refs/heads/master@{#429805} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/57d2116813649148c761075f528db81ec51265db Cr-Commit-Position: refs/heads/master@{#429805} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
