Issue 6477 - separate done and error callbacks in IFileSystem::Read

src/FileSystemJsObject.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 14 matching lines @@
 #include "JsContext.h"
 #include "Utils.h"
 #include "JsError.h"
 #include <AdblockPlus/Platform.h>
 
 using namespace AdblockPlus;
 using AdblockPlus::Utils::ThrowExceptionInJS;
 
 namespace
 {
-  void ReadCallback(const v8::FunctionCallbackInfo<v8::Value>& arguments)
+  namespace ReadCallback

[sergei, 2018/03/23 11:27:19]

the changes here are:
- create the namespace `ReadCallback`
- move the previously called `ReadCallback` function into the namespace and call
it V8Callback.
- add WeakData which is removing the stored JS values from JsEngine. It's a
first attempt to address #6517, however here it's not generic and it's almost
the same what was here before. As soon as there are several (3-5) such WeakData
structures they will be generalized and committed as #6517.
- now the JS callback function requires 3 arguments, previously it required 2.
- the call of fileSystem.Read is adjusted in accordance with the changes of the
interface.

   {
-    AdblockPlus::JsEnginePtr jsEngine = AdblockPlus::JsEngine::FromArguments(arguments);
-    AdblockPlus::JsValueList converted = jsEngine->ConvertArguments(arguments);
+    struct WeakData
+    {
+    public:
+      WeakData(const JsEnginePtr& jsEngine,
+        JsEngine::JsWeakValuesID weakResolveCallback,
+        JsEngine::JsWeakValuesID weakRejectCallback)
+        : weakJsEngine(jsEngine)
+        , weakResolveCallback(weakResolveCallback)
+        , weakRejectCallback(weakRejectCallback)
+      {
+      }
+      virtual ~WeakData()
+      {
+        auto jsEngine = weakJsEngine.lock();
+        if (!jsEngine)
+          return;
+        jsEngine->TakeJsValues(weakResolveCallback);
+        jsEngine->TakeJsValues(weakRejectCallback);
+      }
+      std::weak_ptr<JsEngine> weakJsEngine;
+      JsEngine::JsWeakValuesID weakResolveCallback;
+      JsEngine::JsWeakValuesID weakRejectCallback;
+    };
 
-    v8::Isolate* isolate = arguments.GetIsolate();
-    if (converted.size() != 2)
-      return ThrowExceptionInJS(isolate, "_fileSystem.read requires 2 parameters");
-    if (!converted[1].IsFunction())
-      return ThrowExceptionInJS(isolate, "Second argument to _fileSystem.read must be a function");
+    static void V8Callback(const v8::FunctionCallbackInfo<v8::Value>& arguments)
+    {
+      AdblockPlus::JsEnginePtr jsEngine = AdblockPlus::JsEngine::FromArguments(arguments);
+      AdblockPlus::JsValueList converted = jsEngine->ConvertArguments(arguments);
 
-    JsValueList values;
-    values.push_back(converted[1]);
-    auto weakCallback = jsEngine->StoreJsValues(values);
-    std::weak_ptr<JsEngine> weakJsEngine = jsEngine;
-    auto fileName = converted[0].AsString();
-    jsEngine->GetPlatform().WithFileSystem(
-      [weakJsEngine, weakCallback, fileName](IFileSystem& fileSystem)
+      v8::Isolate* isolate = arguments.GetIsolate();
+      if (converted.size() != 3)
+        return ThrowExceptionInJS(isolate, "_fileSystem.read requires 3 parameters");
+      if (!converted[1].IsFunction())
+        return ThrowExceptionInJS(isolate, "Second argument to _fileSystem.read must be a function");
+      if (!converted[2].IsFunction())
+        return ThrowExceptionInJS(isolate, "Third argument to _fileSystem.read must be a function");
+
+      auto weakResolveCallback = jsEngine->StoreJsValues({converted[1]});
+      auto weakRejectCallback = jsEngine->StoreJsValues({converted[2]});
+      auto weakData = std::make_shared<WeakData>(jsEngine, weakResolveCallback, weakRejectCallback);
+      auto fileName = converted[0].AsString();
+      jsEngine->GetPlatform().WithFileSystem(
+        [weakData, fileName](IFileSystem& fileSystem)
+        {
+          fileSystem.Read(fileName, [weakData](IFileSystem::IOBuffer&& content)
+            {
+              auto jsEngine = weakData->weakJsEngine.lock();
+              if (!jsEngine)
+                return;
+              const JsContext context(*jsEngine);
+              auto result = jsEngine->NewObject();
+              result.SetStringBufferProperty("content", content);
+              jsEngine->GetJsValues(weakData->weakResolveCallback)[0].Call(result);
+            },
+            [weakData](const std::string& error)
+            {
+              if (error.empty())
+                return;
+              auto jsEngine = weakData->weakJsEngine.lock();
+              if (!jsEngine)
+                return;
+              const JsContext context(*jsEngine);
+              jsEngine->GetJsValues(weakData->weakRejectCallback)[0].Call(jsEngine->NewValue(error));
+            });
+        });
+    } // V8Callback
+  } // namespace ReadCallback
+
+  namespace ReadFromFileCallback

[sergei, 2018/03/23 11:27:19]

the changes here are:
- create the namespace `ReadFromFileCallback`
- move the previously called `ReadFromFileCallback` function into the namespace
and call it V8Callback.
- move other related helper functions to the namespace too.
- add WeakData which is removing the stored JS values from JsEngine. See above
regarding #6517.
- now the JS callback function requires 4 arguments, previously it required 3.
- the call of fileSystem.Read is adjusted in accordance with the changes of the
interface.

+  {
+    inline bool IsEndOfLine(char c)
+    {
+      return c == 10 || c == 13;
+    }
+
+    inline StringBuffer::const_iterator SkipEndOfLine(StringBuffer::const_iterator ii, StringBuffer::const_iterator end)
+    {
+      while (ii != end && IsEndOfLine(*ii))
+        ++ii;
+      return ii;
+    }
+
+    inline StringBuffer::const_iterator AdvanceToEndOfLine(StringBuffer::const_iterator ii, StringBuffer::const_iterator end)
+    {
+      while (ii != end && !IsEndOfLine(*ii))
+        ++ii;
+      return ii;
+    }
+
+    struct WeakData : ReadCallback::WeakData
+    {
+    public:
+      WeakData(const JsEnginePtr& jsEngine,
+        JsEngine::JsWeakValuesID weakResolveCallback,
+        JsEngine::JsWeakValuesID weakRejectCallback,
+        JsEngine::JsWeakValuesID weakProcessFunc)
+        : ReadCallback::WeakData(jsEngine, weakResolveCallback, weakRejectCallback)
+        , weakProcessFunc(weakProcessFunc)
       {
-        fileSystem.Read(fileName,
-          [weakJsEngine, weakCallback]
-          (IFileSystem::IOBuffer&& content, const std::string& error)
-          {
-            auto jsEngine = weakJsEngine.lock();
-            if (!jsEngine)
-              return;
+      }
+      ~WeakData()
+      {
+        auto jsEngine = weakJsEngine.lock();
+        if (!jsEngine)
+          return;
+        jsEngine->TakeJsValues(weakProcessFunc);
+      }
+      JsEngine::JsWeakValuesID weakProcessFunc;
+    };
 
-            const JsContext context(*jsEngine);
-            auto result = jsEngine->NewObject();
-            result.SetStringBufferProperty("content", std::move(content));
-            if (!error.empty())
-              result.SetProperty("error", error);
-            jsEngine->TakeJsValues(weakCallback)[0].Call(result);
-          });
-      });
-  }
+    void V8Callback(const v8::FunctionCallbackInfo<v8::Value>& arguments)
+    {
+      AdblockPlus::JsEnginePtr jsEngine = AdblockPlus::JsEngine::FromArguments(arguments);
+      AdblockPlus::JsValueList converted = jsEngine->ConvertArguments(arguments);
 
-  inline bool IsEndOfLine(char c)
-  {
-    return c == 10 || c == 13;
-  }
+      v8::Isolate* isolate = arguments.GetIsolate();
+      if (converted.size() != 4)
+        return ThrowExceptionInJS(isolate, "_fileSystem.readFromFile requires 4 parameters");
+      if (!converted[1].IsFunction())
+        return ThrowExceptionInJS(isolate, "Second argument to _fileSystem.readFromFile must be a function (listener callback)");
+      if (!converted[2].IsFunction())
+        return ThrowExceptionInJS(isolate, "Third argument to _fileSystem.readFromFile must be a function (done callback)");
+      if (!converted[3].IsFunction())
+        return ThrowExceptionInJS(isolate, "Third argument to _fileSystem.readFromFile must be a function (error callback)");
 
-  inline StringBuffer::const_iterator SkipEndOfLine(StringBuffer::const_iterator ii, StringBuffer::const_iterator end)
-  {
-    while (ii != end && IsEndOfLine(*ii))
-      ++ii;
-    return ii;
-  }
+      auto weakProcessFunc = jsEngine->StoreJsValues({converted[1]});
+      auto weakResolveCallback = jsEngine->StoreJsValues({converted[2]});
+      auto weakRejectCallback = jsEngine->StoreJsValues({converted[3]});
+      auto weakData = std::make_shared<WeakData>(jsEngine, weakResolveCallback, weakRejectCallback, weakProcessFunc);
+      auto fileName = converted[0].AsString();
+      jsEngine->GetPlatform().WithFileSystem([weakData, fileName](IFileSystem& fileSystem)
+        {
+          fileSystem.Read(fileName, [weakData](IFileSystem::IOBuffer&& content)
+            {
+              auto jsEngine = weakData->weakJsEngine.lock();
+              if (!jsEngine)
+                return;
 
-  inline StringBuffer::const_iterator AdvanceToEndOfLine(StringBuffer::const_iterator ii, StringBuffer::const_iterator end)
-  {
-    while (ii != end && !IsEndOfLine(*ii))
-      ++ii;
-    return ii;
-  }
+              const JsContext context(*jsEngine);
+              auto jsValues = jsEngine->GetJsValues(weakData->weakProcessFunc);
+              auto processFunc = jsValues[0].UnwrapValue().As<v8::Function>();
+              auto globalContext = context.GetV8Context()->Global();
+              if (!globalContext->IsObject())
+                throw std::runtime_error("`this` pointer has to be an object");
 
-  void ReadFromFileCallback(const v8::FunctionCallbackInfo<v8::Value>& arguments)
-  {
-    AdblockPlus::JsEnginePtr jsEngine = AdblockPlus::JsEngine::FromArguments(arguments);
-    AdblockPlus::JsValueList converted = jsEngine->ConvertArguments(arguments);
-
-    v8::Isolate* isolate = arguments.GetIsolate();
-    if (converted.size() != 3)
-      return ThrowExceptionInJS(isolate, "_fileSystem.readFromFile requires 3 parameters");
-    if (!converted[1].IsFunction())
-      return ThrowExceptionInJS(isolate, "Second argument to _fileSystem.readFromFile must be a function (listener callback)");
-    if (!converted[2].IsFunction())
-      return ThrowExceptionInJS(isolate, "Third argument to _fileSystem.readFromFile must be a function (done callback)");
-
-    JsValueList values;
-    values.push_back(converted[1]);
-    values.push_back(converted[2]);
-    auto weakCallback = jsEngine->StoreJsValues(values);
-    std::weak_ptr<JsEngine> weakJsEngine = jsEngine;
-    auto fileName = converted[0].AsString();
-    jsEngine->GetPlatform().WithFileSystem(
-      [weakJsEngine, weakCallback, fileName](IFileSystem& fileSystem)
-      {
-        fileSystem.Read(fileName,
-          [weakJsEngine, weakCallback]
-          (IFileSystem::IOBuffer&& content, const std::string& error)
-          {
-            auto jsEngine = weakJsEngine.lock();
-            if (!jsEngine)
-              return;
-
-            const JsContext context(*jsEngine);
-
-            auto jsValues = jsEngine->TakeJsValues(weakCallback);
-            if (!error.empty())
+              const v8::TryCatch tryCatch;
+              const auto contentEnd = content.cend();
+              auto stringBegin = SkipEndOfLine(content.begin(), contentEnd);
+              do
+              {
+                auto stringEnd = AdvanceToEndOfLine(stringBegin, contentEnd);
+                auto jsLine = Utils::StringBufferToV8String(jsEngine->GetIsolate(), StringBuffer(stringBegin, stringEnd)).As<v8::Value>();
+                processFunc->Call(globalContext, 1, &jsLine);
+                if (tryCatch.HasCaught())
+                  throw JsError(tryCatch.Exception(), tryCatch.Message());

[sergei, 2018/03/23 11:27:19]

Please pay attention to this change. Previously it was

if (tryCatch.HasCaught())	
{	
 
jsValues[1].Call(jsEngine->NewValue(JsError::ExceptionToString(tryCatch.Exception(),
tryCatch.Message())));	
  return;
}

now it's

if (tryCatch.HasCaught())
  throw JsError(tryCatch.Exception(), tryCatch.Message());

I find it deserving a comment here because it looks like the changing of the
approach, namely it seems that we switched to the exception-based execution
flow, however it's not completely correct. One should be ready that an exception
can be thrown by any method of either JsValue or JsEngine, in particular, when
one calls `jsValues[1].Call();` (see in the old code) it throws the same
exception, here we throw the exception manually because `v8::Function` is not
wrapped by JsValue, so IMO it's not a switching to the exception-based flow,
it's rather a unifying of the approach.

+                stringBegin = SkipEndOfLine(stringEnd, contentEnd);
+              } while (stringBegin != contentEnd);
+              jsEngine->GetJsValues(weakData->weakResolveCallback)[0].Call();
+            }, [weakData](const std::string& error)
             {
-              jsValues[1].Call(jsEngine->NewValue(error));
-              return;
-            }
-
-            auto processFunc = jsValues[0].UnwrapValue().As<v8::Function>();
-
-            auto globalContext = context.GetV8Context()->Global();
-            if (!globalContext->IsObject())
-              throw std::runtime_error("`this` pointer has to be an object");
-
-            const v8::TryCatch tryCatch;
-
-            const auto contentEnd = content.cend();
-            auto stringBegin = SkipEndOfLine(content.begin(), contentEnd);
-            do
-            {
-              auto stringEnd = AdvanceToEndOfLine(stringBegin, contentEnd);
-              auto jsLine = Utils::StringBufferToV8String(jsEngine->GetIsolate(), StringBuffer(stringBegin, stringEnd)).As<v8::Value>();
-              processFunc->Call(globalContext, 1, &jsLine);
-              if (tryCatch.HasCaught())
-              {
-                jsValues[1].Call(jsEngine->NewValue(JsError::ExceptionToString(tryCatch.Exception(), tryCatch.Message())));
+              if (error.empty())
                 return;
-              }
-              stringBegin = SkipEndOfLine(stringEnd, contentEnd);
-            } while (stringBegin != contentEnd);
-            jsValues[1].Call();
-          });
-      });
-  }
+              auto jsEngine = weakData->weakJsEngine.lock();
+              if (!jsEngine)
+                return;
+              jsEngine->GetJsValues(weakData->weakRejectCallback)[0].Call(jsEngine->NewValue(error));
+            });
+        });
+    } // V8Callback
+  } // namespace ReadFromFileCallback
 
   void WriteCallback(const v8::FunctionCallbackInfo<v8::Value>& arguments)
   {
     AdblockPlus::JsEnginePtr jsEngine = AdblockPlus::JsEngine::FromArguments(arguments);
     AdblockPlus::JsValueList converted = jsEngine->ConvertArguments(arguments);
 
     v8::Isolate* isolate = arguments.GetIsolate();
     if (converted.size() != 3)
       return ThrowExceptionInJS(isolate, "_fileSystem.write requires 3 parameters");
     if (!converted[2].IsFunction())
@@ skipping 134 matching lines @@
              params.push_back(result);
              jsEngine->TakeJsValues(weakCallback)[0].Call(params);
            });
       });
   }
 }
 
 
 JsValue& FileSystemJsObject::Setup(JsEngine& jsEngine, JsValue& obj)
 {
-  obj.SetProperty("read", jsEngine.NewCallback(::ReadCallback));
-  obj.SetProperty("readFromFile", jsEngine.NewCallback(::ReadFromFileCallback));
+  obj.SetProperty("read", jsEngine.NewCallback(::ReadCallback::V8Callback));
+  obj.SetProperty("readFromFile", jsEngine.NewCallback(::ReadFromFileCallback::V8Callback));
   obj.SetProperty("write", jsEngine.NewCallback(::WriteCallback));
   obj.SetProperty("move", jsEngine.NewCallback(::MoveCallback));
   obj.SetProperty("remove", jsEngine.NewCallback(::RemoveCallback));
   obj.SetProperty("stat", jsEngine.NewCallback(::StatCallback));
   return obj;
 }