Issue #4692 - Rewrite web request task to avoid engine self-reference

src/WebRequestJsObject.cpp

Index: src/WebRequestJsObject.cpp
===================================================================
--- a/src/WebRequestJsObject.cpp
+++ b/src/WebRequestJsObject.cpp
@@ -19,9 +19,12 @@
 #include <AdblockPlus/JsValue.h>
 #include <AdblockPlus/WebRequest.h>
 
-#include "JsContext.h"
+#include "JsEngineInternal.h"
+#include "JsEngineTransition.h"
 #include "Utils.h"
 #include "Scheduler.h"
+#include "V8Upgrade.h"
+#include "Value.h"
 #include "WebRequestJsObject.h"
 
 namespace
@@ -29,90 +32,138 @@
   class WebRequestTask
   {
   public:
-    WebRequestTask(AdblockPlus::JsEnginePtr jsEngine, AdblockPlus::JsValueList& arguments)
-        : jsEngine(jsEngine), url(arguments[0]->AsString())
-    {
-      if (!url.length())
-        throw std::runtime_error("Invalid string passed as first argument to GET");
-
-      {
-        AdblockPlus::JsValuePtr headersObj = arguments[1];
-        if (!headersObj->IsObject())
-          throw std::runtime_error("Second argument to GET must be an object");
-
-        std::vector<std::string> properties = headersObj->GetOwnPropertyNames();
-        for (std::vector<std::string>::iterator it = properties.begin();
-            it != properties.end(); ++it)
-        {
-          std::string header = *it;
-          std::string headerValue = headersObj->GetProperty(header)->AsString();
-          if (header.length() && headerValue.length())
-            headers.push_back(std::pair<std::string, std::string>(header, headerValue));
-        }
-      }
-
-      callback = arguments[2];
-      if (!callback->IsFunction())
-        throw std::runtime_error("Third argument to GET must be a function");
-    }
+    WebRequestTask(
+      JsEngineInternal *engine,
+      std::string url,
+      AdblockPlus::HeaderList headers,
+      V8PersistentNG<v8::Function> callbackFunction
+    )
+      : jsEngine(engine->shared_from_this()),
+      url(url), headers(headers), callbackFunction(callbackFunction)
+    {}
 
     void operator()()
     {
-      AdblockPlus::ServerResponse result = jsEngine->GetWebRequest()->GET(url, headers);
-
-      AdblockPlus::JsContext context(jsEngine);
-
-      AdblockPlus::JsValuePtr resultObject = jsEngine->NewObject();
-      resultObject->SetProperty("status", result.status);
-      resultObject->SetProperty("responseStatus", result.responseStatus);
-      resultObject->SetProperty("responseText", result.responseText);
+      auto engine = ToInternal(jsEngine); // temporary statement while task keeps its own engine alive
+      /*
+       * Synchronous HTTP GET request is arbitrary-duration and not interruptible
+       */
+      AdblockPlus::ServerResponse result = engine->GetWebRequest()->GET(url, headers);
+      /*
+       * Instantiate our scope after the long-lived operation above.
+       */
+      V8ExecutionScope sentry(engine);
+      auto isolate = engine->GetIsolate();
 
-      AdblockPlus::JsValuePtr headersObject = jsEngine->NewObject();
-      for (AdblockPlus::HeaderList::iterator it = result.responseHeaders.begin();
-        it != result.responseHeaders.end(); ++it)
+      // Create the response object to pass to the callback function
+      auto response = v8::Object::New();
+      SetPropertyOnV8Object(isolate, response, "status", result.status);
+      SetPropertyOnV8Object(isolate, response, "responseStatus", result.responseStatus);
+      SetPropertyOnV8Object(isolate, response, "responseText", result.responseText);
+      auto responseHeaders = v8::Object::New();
+      for (auto it = result.responseHeaders.begin(); it != result.responseHeaders.end(); ++it)
       {
-        headersObject->SetProperty(it->first, it->second);
+        SetPropertyOnV8Object(isolate, responseHeaders, it->first, it->second);
       }
-      resultObject->SetProperty("responseHeaders", headersObject);
+      SetPropertyOnV8Object(isolate, response, "responseHeaders", responseHeaders);
 
-      AdblockPlus::JsValueList params;
-      params.push_back(resultObject);
-      callback->Call(params);
+      // Call the callback
+      auto args = AllocatedArray<v8::Local<v8::Value>>(1);
+      args[0] = response;
+      engine->ApplyFunction(callbackFunction.Get(isolate), std::move(args));
     }
 
   private:
+    /**
+     * Engine pointer keeps engine in existence across thread boundary
+     */
     AdblockPlus::JsEnginePtr jsEngine;
     std::string url;
     AdblockPlus::HeaderList headers;
-    AdblockPlus::JsValuePtr callback;
+    V8PersistentNG<v8::Function> callbackFunction;
   };
-
-  v8::Handle<v8::Value> GETCallback(const v8::Arguments& arguments)
-  {
-    std::shared_ptr<WebRequestTask> thread;
-    AdblockPlus::JsEnginePtr jsEngine;
-    try
-    {
-      jsEngine = AdblockPlus::JsEngine::FromArguments(arguments);
-      AdblockPlus::JsValueList converted = jsEngine->ConvertArguments(arguments);
-      if (converted.size() != 3u)
-        throw std::runtime_error("GET requires exactly 3 arguments");
-      thread = std::make_shared<WebRequestTask>(jsEngine, converted);
-    }
-    catch (const std::exception& e)
-    {
-      using AdblockPlus::Utils::ToV8String;
-      v8::Isolate* isolate = arguments.GetIsolate();
-      return v8::ThrowException(ToV8String(isolate, e.what()));
-    }
-    jsEngine->Schedule(AdblockPlus::MakeHeapFunction(thread), AdblockPlus::ImmediateSingleUseThread);
-    return v8::Undefined();
-  }
 }
 
-AdblockPlus::JsValuePtr AdblockPlus::WebRequestJsObject::Setup(
-    AdblockPlus::JsEnginePtr jsEngine, AdblockPlus::JsValuePtr obj)
+/**
+ * Implementing function for JS "<global>._webrequest.GET()".
+ * 
+ * Used in "compat.js" to implement "XMLHttpRequest.send()" for GET requests.
+ *
+ * \par JavaScript arguments
+ * 1. url.
+ *   The URL of the resource to which the request will be sent.
+ * 2. requestHeaders. 
+ *   An object whose properties represent HTTP GET request headers.
+ *   These properties are added to the GET request.
+ * 3. readyCallback.
+ *   A function to be executed when the GET response is ready.
+ *   The argument of this call is an object encoding the GET response.
+ */
+v8::Handle<v8::Value> GETCallback(const v8::Arguments& arguments)
 {
-  obj->SetProperty("GET", jsEngine->NewCallback(::GETCallback));
-  return obj;
+  auto engine = JsEngineInternal::ExtractEngine(arguments);
+  V8ExecutionScope sentry(engine);
+
+  /*
+   * Factory block for WebRequest tasks.
+   */
+  std::shared_ptr<WebRequestTask> task;
+  try
+  {
+    std::string url;
+    if (arguments.Length() != 3u)
+    {
+      throw std::runtime_error("GET requires exactly 3 arguments");
+    }
+    bool b;
+    std::tie(b, url) = ConvertString(arguments[0]);
+    if (!b)
+    {
+      throw std::runtime_error("First argument to GET must be a string");
+    }
+    if (!url.length())
+    {
+      throw std::runtime_error("Invalid string passed as first argument to GET");
+    }
+
+    AdblockPlus::HeaderList headers;
+    auto headersArg = arguments[1];
+    if (!headersArg->IsObject())
+    {
+      throw std::runtime_error("Second argument to GET must be an object");
+    }
+    auto headersObj = v8::Local<v8::Object>::Cast(headersArg);
+    auto headersProperties = headersObj->GetOwnPropertyNames();
+    uint32_t length = headersProperties->Length();
+    for (uint32_t i = 0; i < length; i++)
+    {
+      auto v8PropertyName(headersProperties->Get(i));
+      auto v8PropertyValue(headersObj->Get(v8PropertyName));
+      auto propertyName(AdblockPlus::Utils::FromV8String(v8PropertyName));
+      auto propertyValue(AdblockPlus::Utils::FromV8String(v8PropertyValue));
+      if (propertyName.length() > 0 && propertyValue.length() > 0)
+      {
+        headers.push_back(std::make_pair(propertyName, propertyValue));
+      }
+    }
+
+    auto callbackArg = arguments[2];
+    if (!callbackArg->IsFunction())
+    {
+      throw std::runtime_error("Third argument to GET must be a function");
+    }
+    task = std::make_shared<WebRequestTask>(engine, url, headers, 
+      V8PersistentNG<v8::Function>(engine->GetIsolate(), v8::Local<v8::Function>::Cast(arguments[2])));
+  }
+  catch (const std::exception& e)
+  {
+    using AdblockPlus::Utils::ToV8String;
+    v8::Isolate* isolate = arguments.GetIsolate();
+    return v8::ThrowException(ToV8String(isolate, e.what()));
+  }
+  /*
+   * Run the task
+   */
+  engine->Schedule(AdblockPlus::MakeHeapFunction(task), AdblockPlus::ImmediateSingleUseThread);
+  return v8::Undefined();
 }