Issue 3594 - remove circular references JsEngine-JsValue-JsEngine

src/JsValue.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 #include <vector>
 #include <AdblockPlus.h>
 
 #include "JsContext.h"
 #include "JsError.h"
 #include "Utils.h"
 
 AdblockPlus::JsValue::JsValue(const std::weak_ptr<AdblockPlus::JsEngine>& jsEngine,
   v8::Handle<v8::Value> value)
   : jsEngine(jsEngine)
   , value(new v8::Persistent<v8::Value>(Utils::lockJsEngine(jsEngine)->GetIsolate(), value))

[Eric, 2016/11/28 14:21:13]

This use of 'lockJsEngine' has a race condition. The lock keeping the engine
object in existence expires when 'lockJsEngine' returns and is no longer in
place for the dereference operator ->. This is a tiny window for failure, but
it's not thread-safe and may occasionally generate null pointer violations.

[sergei, 2016/11/29 10:45:18]

lockJsEngine returns an object not a reference or pointer, according to the
standard the temporary here will be alive until the finishing of execution of
the whole expression 'value(new
v8::Persistent<v8::Value>(Utils::lockJsEngine(jsEngine)->GetIsolate(), value))'.
So, there is no race condition.

 {
 }
 
 AdblockPlus::JsValue::JsValue(AdblockPlus::JsValue&& src)
-    : jsEngine(move(src.jsEngine)),
+    : jsEngine(std::move(src.jsEngine)),
       value(std::move(src.value))

[Eric, 2016/11/28 14:21:13]

For consistency, this should either be (1) `std::move` in both places or (2) a
declaration `using std::move;` and `move` in both places. Personally, `move` is
so fundamental to rvalue semantics that constantly putting `std::` in front of
it is distracting.

This compiles, though, so somehow the compiler is allowing it. A future
compiler, however, might not.

[sergei, 2016/11/29 10:45:19]

I have added std:: rather for aesthetic view than for consistency.
Here it works without std:: because of ADL, it's pretty stable and widely used
feature of C++.

 {
 }
 
 AdblockPlus::JsValue::~JsValue()
 {
   try
   {
     JsContext context(jsEngine);

[sergei, 2016/12/01 22:24:37]

Although I have not seen a crash on practice there is still a race condition:
if there is already no JsEngine then v8::Persistent::Dispose still can be called
"simultaneously" from different threads. I will look into the code of a newer
version of v8 and check how it works.

     if (value)
     {
       value->Dispose();
       value.reset();
     }
   }
   catch (const JsEngine::JsEngineNotAvailableException&)
   {
   }
 }

[Eric, 2016/11/28 14:21:14]

It's excessive to instantiate a `JsContext` object to ensure the existence of
`jsEngine` for the duration of the destructor. Calling `weak_ptr::lock()` is
enough.

Incidentally, there's also no need to call `value.reset()`, since the destructor
for `value` will take care of that, but that was already there.

AdblockPlus::JsValue::~JsValue()
{
  if (value)
  {
    auto engineGuard = jsEngine.lock();
    if (engineGuard)
    {
      value->Dispose();
    }
  }
}

[sergei, 2016/11/29 10:45:18]

JsContext is not only to ensure that JsEngine is still alive, it also saves from
data race (thanks to JsContext::locker), which happens when `value->Dispose();`
is called from different threads for different values.
Actually there should not be a call of Dispose. That call will be deleted here
after updating of v8. The destructor of v8::UniquePersistent should make that
call, however in the currently used version of v8 it's commented.

 
 bool AdblockPlus::JsValue::IsUndefined() const
 {
   JsContext context(jsEngine);
   return UnwrapValue(context.GetJsEngine())->IsUndefined();
 }
 
 bool AdblockPlus::JsValue::IsNull() const
 {
   JsContext context(jsEngine);
@@ skipping 181 matching lines @@
   return JsValuePtr(new JsValue(jsEngine, result));
 }
 
 AdblockPlus::JsValuePtr AdblockPlus::JsValue::Call(const JsValue& arg) const
 {
   JsContext context(jsEngine);
   JsValueList params;
   params.push_back(JsValuePtr(new JsValue(arg.jsEngine, arg.UnwrapValue(context.GetJsEngine()))));
   return Call(params);
 }