[Extensions + Blink] Account for user gesture in v8 function calls

third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp

Index: third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp
diff --git a/third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp b/third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp
index 20aba329814e771e03d166318aecd919a4079194..b12a1b2b31cd92ef9107383f68f1842b33b59014 100644
--- a/third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp
+++ b/third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp
@@ -29,6 +29,7 @@ class WebScriptExecutor : public SuspendableScriptExecutor::Executor {
                     bool userGesture);
 
   Vector<v8::Local<v8::Value>> execute(LocalFrame*) override;
+  ScriptState* getScriptState() override { return nullptr; }
 
   DEFINE_INLINE_VIRTUAL_TRACE() {
     visitor->trace(m_sources);
@@ -84,12 +85,14 @@ class V8FunctionExecutor : public SuspendableScriptExecutor::Executor {
                      v8::Local<v8::Value> argv[]);
 
   Vector<v8::Local<v8::Value>> execute(LocalFrame*) override;
+  ScriptState* getScriptState() override { return m_scriptState.get(); }
 
  private:
   ScopedPersistent<v8::Function> m_function;
   ScopedPersistent<v8::Value> m_receiver;
   V8PersistentValueVector<v8::Value> m_args;
   RefPtr<ScriptState> m_scriptState;
+  RefPtr<UserGestureToken> m_gestureToken;
 };
 
 V8FunctionExecutor::V8FunctionExecutor(v8::Isolate* isolate,
@@ -101,7 +104,8 @@ V8FunctionExecutor::V8FunctionExecutor(v8::Isolate* isolate,
     : m_function(isolate, function),
       m_receiver(isolate, receiver),
       m_args(isolate),
-      m_scriptState(scriptState) {
+      m_scriptState(scriptState),
+      m_gestureToken(UserGestureIndicator::currentToken()) {
   m_args.ReserveCapacity(argc);
   for (int i = 0; i < argc; ++i)
     m_args.Append(argv[i]);
@@ -110,20 +114,21 @@ V8FunctionExecutor::V8FunctionExecutor(v8::Isolate* isolate,
 Vector<v8::Local<v8::Value>> V8FunctionExecutor::execute(LocalFrame* frame) {
   v8::Isolate* isolate = v8::Isolate::GetCurrent();
   Vector<v8::Local<v8::Value>> results;
-  if (!m_scriptState->contextIsValid())
-    return results;
-  ScriptState::Scope scope(m_scriptState.get());
+  DCHECK(m_scriptState->contextIsValid());
   v8::Local<v8::Value> singleResult;
   Vector<v8::Local<v8::Value>> args;
   args.reserveCapacity(m_args.Size());
   for (size_t i = 0; i < m_args.Size(); ++i)
     args.append(m_args.Get(i));
-  if (V8ScriptRunner::callFunction(m_function.newLocal(isolate),
-                                   frame->document(),
-                                   m_receiver.newLocal(isolate), args.size(),
-                                   args.data(), toIsolate(frame))
-          .ToLocal(&singleResult))
-    results.append(singleResult);
+  {
+    UserGestureIndicator gestureIndicator(m_gestureToken.release());
+    if (V8ScriptRunner::callFunction(m_function.newLocal(isolate),
+                                     frame->document(),
+                                     m_receiver.newLocal(isolate), args.size(),
+                                     args.data(), toIsolate(frame))
+            .ToLocal(&singleResult))
+      results.append(singleResult);
+  }
   return results;
 }
 
@@ -200,15 +205,28 @@ void SuspendableScriptExecutor::run() {
 
 void SuspendableScriptExecutor::executeAndDestroySelf() {
   v8::HandleScope scope(v8::Isolate::GetCurrent());

[dcheng, 2016/10/28 17:24:17]

Btw, assuming we combine the two separate paths, this shouldn't be needed with
ScriptState::Scope used consistently for both paths.

[Devlin, 2016/10/28 20:31:37]

Done.

-  Vector<v8::Local<v8::Value>> results = m_executor->execute(m_frame);
-
-  // The script may have removed the frame, in which case contextDestroyed()
-  // will have handled the disposal/callback.
-  if (!m_frame->client())
-    return;
 
-  if (m_callback)
-    m_callback->completed(results);
+  Vector<v8::Local<v8::Value>> results;
+  ScriptState* scriptState = m_executor->getScriptState();
+  if (scriptState && scriptState->contextIsValid()) {
+    ScriptState::Scope scriptScope(scriptState);

[Devlin, 2016/10/27 23:33:28]

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?)

[haraken, 2016/10/28 08:45:14]

What specifically do you need to keep alive?
ScriptState::Scope must not be used outside the machine stack.

[Devlin, 2016/10/28 15:00:05]

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.
Right. :/  Just makes it a bit more cumbersome here.

[haraken, 2016/10/28 15:47:51]

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.

[Devlin, 2016/10/28 16:00:00]

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.

[haraken, 2016/10/28 16:18:57]

Ah, now I understand what you want to do.

BTW, can we make WebScriptExecutor hold ScriptState as well? Then the complexity
will be gone.

[dcheng, 2016/10/28 17:24:17]

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.

[Devlin, 2016/10/28 20:31:37]

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.

+    results = m_executor->execute(m_frame);
+
+    // The script may have removed the frame, in which case contextDestroyed()
+    // will have handled the disposal/callback.
+    if (!m_frame->client())
+      return;
+    if (m_callback)
+      m_callback->completed(results);
+  } else {
+    results = m_executor->execute(m_frame);
+    // The script may have removed the frame, in which case contextDestroyed()
+    // will have handled the disposal/callback.
+    if (!m_frame->client())
+      return;
+    if (m_callback)
+      m_callback->completed(results);
+  }
   dispose();
 }