[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 "bindings/core/v8/WindowProxy.h"
 #include "core/dom/Document.h"
 #include "core/dom/DocumentUserGestureToken.h"
 #include "core/frame/LocalFrame.h"
 #include "platform/UserGestureIndicator.h"
 #include "public/platform/WebVector.h"
 #include "public/web/WebScriptExecutionCallback.h"
 #include "wtf/PtrUtil.h"
 #include "wtf/Vector.h"
 #include <memory>
 
 namespace blink {
 
 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(LocalFrame*) override;
 
   DEFINE_INLINE_VIRTUAL_TRACE() {
     visitor->trace(m_sources);
     SuspendableScriptExecutor::Executor::trace(visitor);
   }
 
  private:
   HeapVector<ScriptSourceCode> m_sources;
   int m_worldID;
   int m_extensionGroup;
@@ skipping 25 matching lines @@
   } else {
     v8::Local<v8::Value> scriptValue =
         frame->script().executeScriptInMainWorldAndReturnValue(
             m_sources.first());
     results.append(scriptValue);
   }
 
   return results;
 }
 
+ScriptState* WebScriptExecutor::getScriptState(LocalFrame* frame) {
+  return ScriptState::forWorld(
+      frame, DOMWrapperWorld::fromWorldId(v8::Isolate::GetCurrent(), m_worldID,
+                                          m_extensionGroup));
+}
+
 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(LocalFrame*) 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);
+  {
+    std::unique_ptr<UserGestureIndicator> gestureIndicator;
+    // It's important that we don't pass the gesture if it's already the

[Nate Chapin, 2016/11/01 17:40:18]

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.

[Devlin, 2016/11/01 17:56:10]

sgtm.  I'll do that in a separate CL and send it your way.

+    // current token, because otherwise once this goes out of scope, the gesture
+    // is removed and is no longer considered to be processing. Thus, if
+    // execute() were called re-entrantly, we would lose the gesture while it
+    // should still be being processed.
+    if (m_gestureToken &&
+        UserGestureIndicator::currentToken() != m_gestureToken) {
+      gestureIndicator =
+          wrapUnique(new UserGestureIndicator(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());
-  Vector<v8::Local<v8::Value>> results = m_executor->execute(m_frame);
+  ScriptState* scriptState = m_executor->getScriptState(m_frame);
+  // TODO(devlin): Is it possible to get here without a valid ScriptState? It's
+  // a SuspendableTimer, which shouldn't execute if the tracked context is
+  // destroyed.
+  if (scriptState && scriptState->contextIsValid()) {

[dcheng, 2016/11/01 00:14:05]

Are we planning on merging these patches? If not, I'd rather just DCHECK().
Hitting this DCHECK() would mean that something is really wrong.

[Devlin, 2016/11/03 01:17:08]

Sounds reasonable, CHECK()ing (given the ambiguity, I'd rather be sure, so opted
for that over a DCHECK).

+    ScriptState::Scope scriptScope(scriptState);
+    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;
+    // The script may have removed the frame, in which case contextDestroyed()
+    // will have handled the disposal/callback.
+    if (!lifecycleContext())
+      return;
 
-  if (m_callback)
-    m_callback->completed(results);
+    // We need to call the callback before scriptScope goes out of scope.
+    if (m_callback)
+      m_callback->completed(results);
+  } else {
+    if (m_callback)
+      m_callback->completed(Vector<v8::Local<v8::Value>>());
+  }
+
   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