Issue #4692 - Rewrite `SetTimeout` facility to avoid `JsValuePtr` and `JsValueList`

src/GlobalJsObject.cpp

Index: src/GlobalJsObject.cpp
===================================================================
--- a/src/GlobalJsObject.cpp
+++ b/src/GlobalJsObject.cpp
@@ -25,6 +25,8 @@
 #include "ConsoleJsObject.h"
 #include "FileSystemJsObject.h"
 #include "GlobalJsObject.h"
+#include "JsEngineInternal.h"
+#include "JsEngineTransition.h"
 #include "Scheduler.h"
 #include "Utils.h"
 #include "WebRequestJsObject.h"
@@ -35,58 +37,95 @@
 {
   class TimeoutTask
   {
+    typedef int64_t delayType; // return type of v8::Value::GetInteger()
   public:
-    TimeoutTask(JsValueList& arguments)
-    {
-      if (arguments.size() < 2)
-        throw std::runtime_error("setTimeout requires at least 2 parameters");
-
-      if (!arguments[0]->IsFunction())
-        throw std::runtime_error(
-          "First argument to setTimeout must be a function");
-
-      function = arguments[0];
-      delay = arguments[1]->AsInt();
-      for (size_t i = 2; i < arguments.size(); i++)
-        functionArguments.push_back(arguments[i]);
-    }
+    TimeoutTask(
+      JsEngineInternal *engine,
+      V8PersistentNG<v8::Function> function,
+      delayType delay,
+      PersistentValueArray functionArguments
+    ) :
+      jsEnginePtr(engine->shared_from_this()),
+      function(function),
+      delay(delay),
+      functionArguments(std::move(functionArguments))
+    {}
 
     void operator()()
-    { 
+    {
       std::this_thread::sleep_for(std::chrono::milliseconds(delay));
-      function->Call(functionArguments);
+
+      auto engine = ToInternal(jsEnginePtr);
+      V8ExecutionScope sentry(engine);
+      auto isolate = jsEnginePtr->GetIsolate();
+      engine->ApplyFunction(function.Get(isolate), functionArguments.GetAsLocal(isolate));
     }
 
   private:
-    JsValuePtr function;
-    int delay;
-    JsValueList functionArguments;
+    /**
+     * shared_ptr ensures the engine remains in existence
+     *   while we're waiting for the task to complete.
+     * The scheduler ought to undertake this responsibility,
+     *   but until then we must do it ourselves.
+     */
+    AdblockPlus::JsEnginePtr jsEnginePtr;

[Eric, 2016/12/19 22:39:52]

This is the only `JsEnginePtr` in the entirety of the new "SetTimeout"
implementation in the present change set.

+    V8PersistentNG<v8::Function> function;
+    delayType delay;
+    PersistentValueArray functionArguments;
   };
+}
 
-  v8::Handle<v8::Value> SetTimeoutCallback(const v8::Arguments& arguments)
+/**
+ * Callback function registered to v8 to implement `SetTimeout`
+ *
+ * Note that this implementation does not support the "evaluate code" version
+ *   of `SetTimeout`, only the "apply function" version.
+ *
+ * \par Reference
+ * https://developer.mozilla.org/en-US/docs/Web/API/WindowTimers/setTimeout
+ */
+v8::Handle<v8::Value> CallbackForSetTimeout(const v8::Arguments& arguments)
+{
+  std::shared_ptr<TimeoutTask> timeoutTask; 
+  auto engine = JsEngineInternal::ExtractEngine(arguments);
+  try
   {
-    std::shared_ptr<TimeoutTask> timeoutTask; 
-    JsEnginePtr engine;
-    try
+    if (arguments.Length() < 2)
     {
-      engine = AdblockPlus::JsEngine::FromArguments(arguments);
-      AdblockPlus::JsValueList converted =
-          engine->ConvertArguments(arguments);
-      timeoutTask = std::make_shared<TimeoutTask>(converted);
+      throw std::runtime_error("setTimeout: must have at least 2 arguments");
     }
-    catch (const std::exception& e)
+    if (!arguments[0]->IsFunction())
     {
-      v8::Isolate* isolate = arguments.GetIsolate();
-      return v8::ThrowException(Utils::ToV8String(isolate, e.what()));
+      throw std::runtime_error("setTimeout: argument 1 must be a function");
     }
-    StartImmediatelyInSingleUseDetachedThread(MakeHeapFunction(timeoutTask));
+    auto isolate = engine->GetIsolate();
+    size_t n = arguments.Length() - 2;
+    PersistentValueArray functionArguments(n);
+    for (size_t i = 0; i < n; i++)
+    {
+      functionArguments[i] = V8PersistentNG<v8::Value>(isolate, arguments[i + 2]);
+    }
+    timeoutTask = std::make_shared<TimeoutTask>(
+      engine,
+      V8PersistentNG<v8::Function>(isolate, v8::Local<v8::Function>::Cast(arguments[0])), 
+      arguments[1]->IntegerValue(), 
+      std::move(functionArguments));
+  }
+  catch (const std::exception& e)
+  {
+    v8::Isolate* isolate = arguments.GetIsolate();
+    return v8::ThrowException(Utils::ToV8String(isolate, e.what()));
+  }
+  StartImmediatelyInSingleUseDetachedThread(MakeHeapFunction(timeoutTask));
 
-    // We should actually return the timer ID here, which could be
-    // used via clearTimeout(). But since we don't seem to need
-    // clearTimeout(), we can save that for later.
-    return v8::Undefined();
-  }
+  // We should actually return the timer ID here, which could be
+  // used via clearTimeout(). But since we don't seem to need
+  // clearTimeout(), we can save that for later.
+  return v8::Undefined();
+}
 
+namespace
+{
   v8::Handle<v8::Value> TriggerEventCallback(const v8::Arguments& arguments)
   {
     AdblockPlus::JsEnginePtr jsEngine = AdblockPlus::JsEngine::FromArguments(arguments);
@@ -104,10 +143,9 @@
   }
 }
 
-JsValuePtr GlobalJsObject::Setup(JsEnginePtr jsEngine, const AppInfo& appInfo,
+JsValuePtr AdblockPlus::GlobalJsObject::Setup(JsEnginePtr jsEngine, const AppInfo& appInfo,
     JsValuePtr obj)
 {
-  obj->SetProperty("setTimeout", jsEngine->NewCallback(::SetTimeoutCallback));

[Eric, 2016/12/19 22:39:52]

Old initialization of global object that used `JsEnginePtr`

   obj->SetProperty("_triggerEvent", jsEngine->NewCallback(::TriggerEventCallback));
   obj->SetProperty("_fileSystem",
       FileSystemJsObject::Setup(jsEngine, jsEngine->NewObject()));