[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;

[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);
     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) {
+  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*,
                      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
+    // 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;
 }
 
 }  // 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);
+  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;
 
-  if (m_callback)
-    m_callback->completed(results);
+    // 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>>());
+  }
+
   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