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

third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "web/SuspendableScriptExecutor.h"
 
 #include "bindings/core/v8/ScriptController.h"
 #include "bindings/core/v8/ScriptSourceCode.h"
 #include "bindings/core/v8/V8PersistentValueVector.h"
 #include "core/dom/Document.h"
@@ skipping 11 matching lines @@
 namespace {
 
 class WebScriptExecutor : public SuspendableScriptExecutor::Executor {
  public:
   WebScriptExecutor(const HeapVector<ScriptSourceCode>& sources,
                     int worldID,
                     int extensionGroup,
                     bool userGesture);
 
   Vector<v8::Local<v8::Value>> execute(LocalFrame*) override;
+  ScriptState* getScriptState() override { return nullptr; }
 
   DEFINE_INLINE_VIRTUAL_TRACE() {
     visitor->trace(m_sources);
     SuspendableScriptExecutor::Executor::trace(visitor);
   }
 
  private:
   HeapVector<ScriptSourceCode> m_sources;
   int m_worldID;
   int m_extensionGroup;
@@ skipping 35 matching lines @@
 class V8FunctionExecutor : public SuspendableScriptExecutor::Executor {
  public:
   V8FunctionExecutor(v8::Isolate*,
                      ScriptState*,
                      v8::Local<v8::Function>,
                      v8::Local<v8::Value> receiver,
                      int argc,
                      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,
                                        ScriptState* scriptState,
                                        v8::Local<v8::Function> function,
                                        v8::Local<v8::Value> receiver,
                                        int argc,
                                        v8::Local<v8::Value> argv[])
     : 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]);
 }
 
 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;
 }
 
 }  // namespace
 
 void SuspendableScriptExecutor::createAndRun(
     LocalFrame* frame,
     int worldID,
     const HeapVector<ScriptSourceCode>& sources,
     int extensionGroup,
@@ skipping 55 matching lines @@
   if (!context->activeDOMObjectsAreSuspended()) {
     suspendIfNeeded();
     executeAndDestroySelf();
     return;
   }
   startOneShot(0, BLINK_FROM_HERE);
   suspendIfNeeded();
 }
 
 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;
+  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);
 
-  if (m_callback)
-    m_callback->completed(results);
+    // 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();
 }
 
 void SuspendableScriptExecutor::dispose() {
   // Remove object as a ContextLifecycleObserver.
   ActiveDOMObject::clearContext();
   m_keepAlive.clear();
   stop();
 }
 
 DEFINE_TRACE(SuspendableScriptExecutor) {
   visitor->trace(m_frame);
   visitor->trace(m_executor);
   SuspendableTimer::trace(visitor);
 }
 
 }  // namespace blink