Issue 3594 - remove circular references JsEngine-JsValue-JsEngine

src/WebRequestJsObject.cpp

Index: src/WebRequestJsObject.cpp
diff --git a/src/WebRequestJsObject.cpp b/src/WebRequestJsObject.cpp
index 48a703709053895845a5bb83e4036158b9b094f9..5eefd27ee99b3eebed1841eae88724a756b01299 100644
--- a/src/WebRequestJsObject.cpp
+++ b/src/WebRequestJsObject.cpp
@@ -30,7 +30,7 @@ namespace
   {
   public:
     WebRequestThread(AdblockPlus::JsEnginePtr jsEngine, AdblockPlus::JsValueList& arguments)
-        : Thread(true), jsEngine(jsEngine), url(arguments[0]->AsString())
+        : Thread(true), m_jsEngine(jsEngine), url(arguments[0]->AsString())
     {
       if (!url.length())
         throw std::runtime_error("Invalid string passed as first argument to GET");
@@ -58,16 +58,16 @@ namespace
 
     void Run()
     {
-      AdblockPlus::ServerResponse result = jsEngine->GetWebRequest()->GET(url, headers);
-
-      AdblockPlus::JsContext context(jsEngine);
-
-      AdblockPlus::JsValuePtr resultObject = jsEngine->NewObject();
+      // Don't keep a strong reference to JsEngine while request is being executed.
+      auto webRequest = AdblockPlus::Utils::lockJsEngine(m_jsEngine)->GetWebRequest();

[Oleksandr, 2016/11/25 10:38:04]

Is there any reason why the line above is not using jsEngine from the context as
well? 

[sergei, 2016/11/25 12:04:46]

Yes, it's in the comment. Let's say we have "locked" JsEngine and call
WebRequest::GET. At the same time we have LazyWebRequest::GET which never
returns. It means that all tests will still prevent JsEngine from destroying.
So, here we lock JsEngine, obtain webRequest and "unlock" JsEngine.
AdblockPlus::Utils::lockJsEngine and AdblockPlus::JsContext throw exception if
they cannot lock JsEngine.

+      auto result = webRequest->GET(url, headers);
+      AdblockPlus::JsContext context(m_jsEngine);
+      AdblockPlus::JsValuePtr resultObject = context.jsEngine().NewObject();
       resultObject->SetProperty("status", result.status);
       resultObject->SetProperty("responseStatus", result.responseStatus);
       resultObject->SetProperty("responseText", result.responseText);
 
-      AdblockPlus::JsValuePtr headersObject = jsEngine->NewObject();
+      AdblockPlus::JsValuePtr headersObject = context.jsEngine().NewObject();
       for (AdblockPlus::HeaderList::iterator it = result.responseHeaders.begin();
         it != result.responseHeaders.end(); ++it)
       {
@@ -81,7 +81,7 @@ namespace
     }
 
   private:
-    AdblockPlus::JsEnginePtr jsEngine;
+    std::weak_ptr<AdblockPlus::JsEngine> m_jsEngine;
     std::string url;
     AdblockPlus::HeaderList headers;
     AdblockPlus::JsValuePtr callback;