[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..8d707f73467a175613579581284ee9ac6cdfbc2d 100644
--- a/third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp
+++ b/third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp
@@ -7,6 +7,7 @@
 #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"
@@ -29,6 +30,7 @@ class WebScriptExecutor : public SuspendableScriptExecutor::Executor {
                     bool userGesture);
 
   Vector<v8::Local<v8::Value>> execute(LocalFrame*) override;
+  ScriptState* getScriptState(LocalFrame*) override;
 
   DEFINE_INLINE_VIRTUAL_TRACE() {
     visitor->trace(m_sources);
@@ -74,6 +76,12 @@ Vector<v8::Local<v8::Value>> WebScriptExecutor::execute(LocalFrame* frame) {
   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*,
@@ -84,12 +92,16 @@ class V8FunctionExecutor : public SuspendableScriptExecutor::Executor {
                      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,
@@ -101,7 +113,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 +123,31 @@ 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);
+  {
+    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;
 }
 
@@ -199,16 +223,27 @@ void SuspendableScriptExecutor::run() {
 }
 
 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;
+
+    // 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>>());
+  }
 
-  if (m_callback)
-    m_callback->completed(results);
   dispose();
 }