[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..79f0972c9cbedee48f96def7d05cd04c622dab56 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;

[dcheng, 2016/10/29 04:36:21]

It is possible to bind ScriptState when we create the SuspendableScriptExecutor?
Then we won't need this virtual.

[Devlin, 2016/10/29 17:45:50]

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?

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

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.

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

Done.

 
   DEFINE_INLINE_VIRTUAL_TRACE() {
     visitor->trace(m_sources);
@@ -74,6 +76,17 @@ Vector<v8::Local<v8::Value>> WebScriptExecutor::execute(LocalFrame* frame) {
   return results;
 }
 
+ScriptState* WebScriptExecutor::getScriptState(LocalFrame* frame) {
+  if (!m_worldID)
+    return ScriptState::forMainWorld(frame);
+
+  RefPtr<DOMWrapperWorld> world = DOMWrapperWorld::ensureIsolatedWorld(

[haraken, 2016/10/29 01:41:06]

Can you implement DOMWwrapperWorld::fromWorldId(int worldId), which returns a
corresponding world? Then we can remove the if(!m_world) check.

[Devlin, 2016/10/31 17:00:46]

Done.  It's a little awkward because it requires passing in the isolate and
extension group, but it works.

+      v8::Isolate::GetCurrent(), m_worldID, m_extensionGroup);
+  WindowProxy* windowProxy = frame->script().windowProxy(*world);
+  return windowProxy->isContextInitialized() ? windowProxy->getScriptState()

[haraken, 2016/10/29 01:41:06]

You can just use ScriptState::forWorld(world).

[Devlin, 2016/10/31 17:00:47]

Ah, handy.  Done.

+                                             : nullptr;
+}
+
 class V8FunctionExecutor : public SuspendableScriptExecutor::Executor {
  public:
   V8FunctionExecutor(v8::Isolate*,
@@ -84,12 +97,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 +118,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 +128,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
+    // 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.

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

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.

[dcheng, 2016/10/29 04:36:21]

I feel like it would be surprising if this were dispatched re-entrantly. Is
there a concrete case where this can happen?

[Devlin, 2016/10/29 17:45:50]

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().

+    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 +228,24 @@ 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);
+  if (scriptState && scriptState->contextIsValid()) {

[dcheng, 2016/10/29 04:36:21]

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.

[Devlin, 2016/10/31 17:00:47]

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.

+    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 (!m_frame->client())

[dcheng, 2016/10/29 04:36:21]

Can we just check lifecycleContext() here?

[Devlin, 2016/10/31 17:00:46]

I think so.  Done.

+      return;
+
+    // We need to call the callback before scriptScope goes out of scope.
+    if (m_callback)

[dcheng, 2016/10/29 04:36:21]

Nit: is it possible to remove this if ()? It seems like valid uses should
generally pass in a callback.

[Devlin, 2016/10/29 17:45:49]

Extensions code can often do this, since we only care about executing the code
and don't necessarily expect a result.

+      m_callback->completed(results);
+  } else {
+    if (m_callback)
+      m_callback->completed(Vector<v8::Local<v8::Value>>());
+  }
 
-  if (m_callback)
-    m_callback->completed(results);
   dispose();
 }