Issue 3594 - remove circular references JsEngine-JsValue-JsEngine

src/Utils.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 <sstream>
 #include <stdexcept>
 
 #ifdef _WIN32
 #include <Windows.h>
 #include <Shlwapi.h>
 #endif
 
 #include "Utils.h"
+#include <AdblockPlus/JsEngine.h>
 
 using namespace AdblockPlus;
 
 std::string Utils::Slurp(std::istream& stream)
 {
   std::stringstream content;
   content << stream.rdbuf();
   return content.str();
 }
 
@@ skipping 60 matching lines @@
     if (FAILED(hr))
     {
       throw std::runtime_error("CanonizeUrl failed\n");
     }
   }
   return canonizedUrl;
 
 }
 #endif
 
+std::shared_ptr<JsEngine> Utils::lockJsEngine(const std::weak_ptr<JsEngine>& jsEngine)
+{
+  auto retValue = jsEngine.lock();
+  if (!retValue)
+    throw AdblockPlus::JsEngine::JsEngineNotAvailableException();
+  return retValue;
+}

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

This function doesn't do what its name indicates that it does. The lifespan of
the shared_ptr `retValue` only lasts the duration of the function body. After
the function returns, the engine argument has no more strong references than it
did at function entry. So there's no actually locking except while the function
is executing.

It's not that checking whether the engine exists behind the pointer isn't
valuable. But such a function shouldn't be named "lock" or anything like it. And
it would be more efficient just to call 'expired()' on the weak_ptr.

See comments where this is used for more.

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

That's just wrong.