[Blink] Modify SuspendableScriptExecutor to take a v8::Function

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 9a3dabcbff7b81727e33907ca7147941bd8686e8..f1b4e7ffb38f2ced6b3ced563511eb556a44d99d 100644
--- a/third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp
+++ b/third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp
@@ -16,28 +16,117 @@
 
 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;
+
+    void trace(Visitor*) override;

[haraken, 2016/09/28 00:41:22]

Use DEFINE_INLINE_VIRTUAL_TRACE.

[Devlin, 2016/09/30 21:22:22]

Done.

+
+private:
+    HeapVector<ScriptSourceCode> m_sources;

[dcheng, 2016/09/27 08:30:57]

I'm a little confused by the ownership model of ScriptSourceCode.  It doesn't
inherit from GarbageCollected or GarbageCollectedFinalized, but it has Member
variables. How does this work? +haraken

[haraken, 2016/09/27 08:39:45]

Yes, because ScriptSourceCode is used only as a part-of object in a
GarbageCollected object.

The owner GarbageCollected object needs to trace ScriptSourceCode. Then
ScriptSourceCode's members are traced.

[dcheng, 2016/09/27 09:02:59]

I see. How come Oilpan doesn't warn that the holder class here isn't GCed
though? It's allocated and stored in a unique_ptr, which should be an error,
right?

[haraken, 2016/09/27 13:25:41]

ScriptSourceCode is in HeapVector (i.e., a GCed object). So this is fine.

[dcheng, 2016/09/27 22:24:25]

So just to confirm, we have the following ownership:

SuspendableScriptExecutor (GarbageCollectedFinalized)
  std::unique_ptr<SuspendableScriptExecutor::Executor> (Executor is not GCed)
    HeapVector<ScriptSourceCode> (ScriptSourceCode is not GCed)
      Member<ScriptResource>, Member<ScriptStreamer>, etc...
(GarbageCollectedFinalized)

Though both SuspendableScriptExecutor::Executor and ScriptSourceCode are both
not GCed, they define trace() methods, so this is OK, because Oilpan can still
reach the garbage collected members stored inside them. Is this correct?

+    int m_worldID;
+    int m_extensionGroup;
+    bool m_userGesture;
+};
+
+WebScriptExecutor::WebScriptExecutor(const HeapVector<ScriptSourceCode>& sources, int worldID, int extensionGroup, bool userGesture)
+    : m_sources(sources)
+    , m_worldID(worldID)
+    , m_extensionGroup(extensionGroup)
+    , m_userGesture(userGesture)
+{
+}
+
+Vector<v8::Local<v8::Value>> WebScriptExecutor::execute(LocalFrame* frame)
+{
+    // after calling the destructor of object - object will be unsubscribed from
+    // resumed and contextDestroyed LifecycleObserver methods

[dcheng, 2016/09/27 08:30:57]

Hmmm. I'm not sure /where/ this comment belongs, but probably not here (this
thing isn't a ContextLifecycleObserver)

Maybe we can remove this comment, since it doesn't really make sense...

[Devlin, 2016/09/27 22:14:20]

I'm happy to remove it.  Done.

+    std::unique_ptr<UserGestureIndicator> indicator;
+    if (m_userGesture)
+        indicator = wrapUnique(new UserGestureIndicator(DefinitelyProcessingNewUserGesture));
+
+    Vector<v8::Local<v8::Value>> results;
+    if (m_worldID) {
+        frame->script().executeScriptInIsolatedWorld(m_worldID, m_sources, m_extensionGroup, &results);
+    } else {
+        v8::Local<v8::Value> scriptValue = frame->script().executeScriptInMainWorldAndReturnValue(m_sources.first());
+        results.append(scriptValue);
+    }
+
+    return results;
+}
+
+void WebScriptExecutor::trace(Visitor* visitor)
+{
+    visitor->trace(m_sources);
+    SuspendableScriptExecutor::Executor::trace(visitor);
+}
+
+class V8FunctionExecutor : public SuspendableScriptExecutor::Executor {
+public:
+    V8FunctionExecutor(v8::Isolate*, v8::Local<v8::Function>, v8::Local<v8::Value> receiver, int argc, v8::Local<v8::Value> argv[]);
+
+    Vector<v8::Local<v8::Value>> execute(LocalFrame*) override;
+
+private:
+    ScopedPersistent<v8::Function> m_function;
+    ScopedPersistent<v8::Value> m_receiver;
+    Vector<std::unique_ptr<ScopedPersistent<v8::Value>>> m_args;

[dcheng, 2016/09/27 08:30:57]

Maybe V8PersistentValueVector from
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...
will be useful here?

[Devlin, 2016/09/27 22:14:20]

Handy!  Done.

+};
+
+V8FunctionExecutor::V8FunctionExecutor(v8::Isolate* isolate, 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)
+{
+    for (int i = 0; i < argc; ++i)
+        m_args.append(std::unique_ptr<ScopedPersistent<v8::Value>>(new ScopedPersistent<v8::Value>(isolate, argv[i])));

[dcheng, 2016/09/27 08:30:57]

wrapUnique from wtf/PtrUtil.h will make this a little easier to write.

[Devlin, 2016/09/27 22:14:20]

Unnecessary with V8PersistentValueVector.

+}
+
+Vector<v8::Local<v8::Value>> V8FunctionExecutor::execute(LocalFrame* frame)
+{
+    v8::Isolate* isolate = v8::Isolate::GetCurrent();
+    Vector<v8::Local<v8::Value>> results;
+    v8::Local<v8::Value> singleResult;
+    v8::Local<v8::Value> args[m_args.size()];
+    for (size_t i = 0; i < m_args.size(); ++i)
+        args[i] = m_args[i]->newLocal(isolate);
+    if (V8ScriptRunner::callFunction(m_function.newLocal(isolate), frame->document(), m_receiver.newLocal(isolate), m_args.size(), static_cast<v8::Local<v8::Value>*>(args), toIsolate(frame)).ToLocal(&singleResult))
+        results.append(singleResult);
+    return results;
+}
+
+} // namespace
+
 void SuspendableScriptExecutor::createAndRun(LocalFrame* frame, int worldID, const HeapVector<ScriptSourceCode>& sources, int extensionGroup, bool userGesture, WebScriptExecutionCallback* callback)
 {
-    SuspendableScriptExecutor* executor = new SuspendableScriptExecutor(frame, worldID, sources, extensionGroup, userGesture, callback);
+    SuspendableScriptExecutor* executor = new SuspendableScriptExecutor(frame, callback, std::unique_ptr<WebScriptExecutor>(new WebScriptExecutor(sources, worldID, extensionGroup, userGesture)));
+    executor->run();
+}
+
+void SuspendableScriptExecutor::createAndRun(LocalFrame* frame, v8::Isolate* isolate, v8::Local<v8::Function> function, v8::Local<v8::Value> receiver, int argc, v8::Local<v8::Value> argv[], WebScriptExecutionCallback* callback)
+{
+    SuspendableScriptExecutor* executor = new SuspendableScriptExecutor(frame, callback, std::unique_ptr<V8FunctionExecutor>(new V8FunctionExecutor(isolate, function, receiver, argc, argv)));
     executor->run();
 }
 
 void SuspendableScriptExecutor::contextDestroyed()
 {
     SuspendableTimer::contextDestroyed();
-    m_callback->completed(Vector<v8::Local<v8::Value>>());
+    if (m_callback)
+        m_callback->completed(Vector<v8::Local<v8::Value>>());
     dispose();
 }
 
-SuspendableScriptExecutor::SuspendableScriptExecutor(LocalFrame* frame, int worldID, const HeapVector<ScriptSourceCode>& sources, int extensionGroup, bool userGesture, WebScriptExecutionCallback* callback)
+SuspendableScriptExecutor::SuspendableScriptExecutor(LocalFrame* frame, WebScriptExecutionCallback* callback, std::unique_ptr<Executor> executor)
     : SuspendableTimer(frame->document())
     , m_frame(frame)
-    , m_sources(sources)
     , m_callback(callback)
     , m_keepAlive(this)
-    , m_worldID(worldID)
-    , m_extensionGroup(extensionGroup)
-    , m_userGesture(userGesture)
+    , m_executor(std::move(executor))
 {
 }
 
@@ -65,27 +154,16 @@ void SuspendableScriptExecutor::run()
 
 void SuspendableScriptExecutor::executeAndDestroySelf()
 {
-    // after calling the destructor of object - object will be unsubscribed from
-    // resumed and contextDestroyed LifecycleObserver methods
-    std::unique_ptr<UserGestureIndicator> indicator;
-    if (m_userGesture)
-        indicator = wrapUnique(new UserGestureIndicator(DefinitelyProcessingNewUserGesture));
-
     v8::HandleScope scope(v8::Isolate::GetCurrent());
-    Vector<v8::Local<v8::Value>> results;
-    if (m_worldID) {
-        m_frame->script().executeScriptInIsolatedWorld(m_worldID, m_sources, m_extensionGroup, &results);
-    } else {
-        v8::Local<v8::Value> scriptValue = m_frame->script().executeScriptInMainWorldAndReturnValue(m_sources.first());
-        results.append(scriptValue);
-    }
+    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;
 
-    m_callback->completed(results);
+    if (m_callback)
+        m_callback->completed(results);
     dispose();
 }
 
@@ -100,7 +178,6 @@ void SuspendableScriptExecutor::dispose()
 DEFINE_TRACE(SuspendableScriptExecutor)
 {
     visitor->trace(m_frame);
-    visitor->trace(m_sources);
     SuspendableTimer::trace(visitor);
 }