Noissue - generalize storing of `JsValue`s in `JsEngine`

src/JsEngine.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 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 66 matching lines @@
   V8Initializer::Init();
   isolate = v8::Isolate::New();
 }
 
 AdblockPlus::ScopedV8Isolate::~ScopedV8Isolate()
 {
   isolate->Dispose();
   isolate = nullptr;
 }
 
-JsEngine::TimerTask::~TimerTask()
+JsEngine::JsWeakValuesList::~JsWeakValuesList()
 {
-  for (auto& arg : arguments)
-    arg->Dispose();
+  for (auto& value : values)
+    value->Dispose();
 }
 
 void JsEngine::ScheduleTimer(const v8::Arguments& arguments)
 {
   auto jsEngine = FromArguments(arguments);
   if (arguments.Length() < 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");
 
-  auto timerTaskIterator = jsEngine->timerTasks.emplace(jsEngine->timerTasks.end());
-
-  for (int i = 0; i < arguments.Length(); i++)
-    timerTaskIterator->arguments.emplace_back(new v8::Persistent<v8::Value>(jsEngine->GetIsolate(), arguments[i]));
+  auto jsValueArguments = jsEngine->ConvertArguments(arguments);
+  auto timerParamsID = jsEngine->StoreJsValues(jsValueArguments);
 
   std::weak_ptr<JsEngine> weakJsEngine = jsEngine;
-  jsEngine->timer->SetTimer(std::chrono::milliseconds(arguments[1]->IntegerValue()), [weakJsEngine, timerTaskIterator]
+  jsEngine->timer->SetTimer(std::chrono::milliseconds(arguments[1]->IntegerValue()), [weakJsEngine, timerParamsID]
   {
     if (auto jsEngine = weakJsEngine.lock())
-      jsEngine->CallTimerTask(timerTaskIterator);
+      jsEngine->CallTimerTask(timerParamsID);
   });
 }
 
-void JsEngine::CallTimerTask(const TimerTasks::const_iterator& timerTaskIterator)
+void JsEngine::CallTimerTask(const JsWeakValuesID& timerParamsID)
 {
-  const JsContext context(*this);
-  JsValue callback(shared_from_this(), v8::Local<v8::Value>::New(GetIsolate(), *timerTaskIterator->arguments[0]));
-  JsValueList callbackArgs;
-  for (int i = 2; i < timerTaskIterator->arguments.size(); i++)
-    callbackArgs.emplace_back(JsValue(shared_from_this(),
-      v8::Local<v8::Value>::New(GetIsolate(), *timerTaskIterator->arguments[i])));
-  callback.Call(callbackArgs);
-  timerTasks.erase(timerTaskIterator);
+  auto timerParams = TakeJsValues(timerParamsID);
+  JsValue callback = std::move(timerParams[0]);
+
+  timerParams.erase(timerParams.begin()); // remove callback placeholder
+  timerParams.erase(timerParams.begin()); // remove timeout param
+  callback.Call(timerParams);
 }
 
 AdblockPlus::JsEngine::JsEngine(const ScopedV8IsolatePtr& isolate, TimerPtr timer)
   : isolate(isolate)
   , fileSystem(new DefaultFileSystem())
   , webRequest(new DefaultWebRequest())
   , logSystem(new DefaultLogSystem())
   , timer(std::move(timer))
 {
 }
@@ skipping 113 matching lines @@
   const v8::Local<const v8::External> external =
       v8::Local<const v8::External>::Cast(arguments.Data());
   std::weak_ptr<JsEngine>* data =
       static_cast<std::weak_ptr<JsEngine>*>(external->Value());
   JsEnginePtr result = data->lock();
   if (!result)
     throw std::runtime_error("Oops, our JsEngine is gone, how did that happen?");
   return result;
 }
 
+JsEngine::JsWeakValuesID JsEngine::StoreJsValues(const JsValueList& values)
+{
+  JsWeakValuesLists::iterator it;
+  {
+    std::lock_guard<std::mutex> lock(jsWeakValuesListsMutex);
+    it = jsWeakValuesLists.emplace(jsWeakValuesLists.end());
+  }
+  {
+    JsContext context(*this);
+    for (const auto& value : values)
+    {
+      it->values.emplace_back(new v8::Persistent<v8::Value>(GetIsolate(), value.UnwrapValue()));
+    }
+  }
+  JsWeakValuesID retValue;
+  retValue.iterator = it;
+  return retValue;
+}
+
+JsValueList JsEngine::TakeJsValues(const JsWeakValuesID& id)
+{
+  JsValueList retValue;
+  {
+    JsContext context(*this);
+    for (const auto& v8Value : id.iterator->values)
+    {
+      retValue.emplace_back(JsValue(shared_from_this(), v8::Local<v8::Value>::New(GetIsolate(), *v8Value)));
+    }
+  }
+  {
+    std::lock_guard<std::mutex> lock(jsWeakValuesListsMutex);
+    jsWeakValuesLists.erase(id.iterator);
+  }
+  return retValue;
+}
+
 AdblockPlus::JsValueList AdblockPlus::JsEngine::ConvertArguments(const v8::Arguments& arguments)
 {
   const JsContext context(*this);
   JsValueList list;
   for (int i = 0; i < arguments.Length(); i++)
     list.push_back(JsValue(shared_from_this(), arguments[i]));
   return list;
 }
 
 AdblockPlus::FileSystemPtr AdblockPlus::JsEngine::GetFileSystem() const
@@ skipping 55 matching lines @@
   logSystem = val;
 }
 
 
 void AdblockPlus::JsEngine::SetGlobalProperty(const std::string& name,
                                               const AdblockPlus::JsValue& value)
 {
   auto global = GetGlobalObject();
   global.SetProperty(name, value);
 }