Convert references to FileSystem & Co. into shared pointers (avoid use after free)

src/JsEngine.cpp

Index: src/JsEngine.cpp
===================================================================
--- a/src/JsEngine.cpp
+++ b/src/JsEngine.cpp
@@ -41,22 +41,18 @@ namespace
 }
 
 AdblockPlus::JsError::JsError(const v8::Handle<v8::Value> exception,
     const v8::Handle<v8::Message> message)
   : std::runtime_error(ExceptionToString(exception, message))
 {
 }
 
-AdblockPlus::JsEngine::JsEngine(const AppInfo& appInfo,
-                                FileSystem* const fileSystem,
-                                WebRequest* const webRequest,
-                                ErrorCallback* const errorCallback)
-  : fileSystem(*fileSystem), webRequest(*webRequest),
-    errorCallback(*errorCallback), isolate(v8::Isolate::GetCurrent())
+AdblockPlus::JsEngine::JsEngine(const AppInfo& appInfo)
+  : isolate(v8::Isolate::GetCurrent())
 {
   const v8::Locker locker(isolate);
   const v8::HandleScope handleScope;
 
   context = v8::Context::New();
   AdblockPlus::GlobalJsObject::Setup(*this, appInfo,
       JsValuePtr(new JsValue(*this, context->Global())));
 }
@@ -70,17 +66,17 @@ AdblockPlus::JsValuePtr AdblockPlus::JsE
   CheckTryCatch(tryCatch);
   v8::Local<v8::Value> result = script->Run();
   CheckTryCatch(tryCatch);
   return JsValuePtr(new JsValue(*this, result));
 }
 
 void AdblockPlus::JsEngine::Load(const std::string& scriptPath)
 {
-  const std::tr1::shared_ptr<std::istream> file = fileSystem.Read(scriptPath);
+  const std::tr1::shared_ptr<std::istream> file = GetFileSystem()->Read(scriptPath);
   if (!file || !*file)
     throw std::runtime_error("Unable to load script " + scriptPath);
   Evaluate(Utils::Slurp(*file));
 }
 
 void AdblockPlus::JsEngine::Gc()
 {
   while (!v8::V8::IdleNotification());
@@ -131,13 +127,49 @@ AdblockPlus::JsValueList AdblockPlus::Js
 {
   const Context context(*this);
   JsValueList list;
   for (int i = 0; i < arguments.Length(); i++)
     list.push_back(JsValuePtr(new JsValue(*this, arguments[i])));
   return list;
 }
 
+AdblockPlus::FileSystemPtr AdblockPlus::JsEngine::GetFileSystem()
+{
+  if (!fileSystem)
+    fileSystem.reset(new DefaultFileSystem());

[Felix Dahlke, 2013/04/18 13:30:12]

I'd rather have this in the constructor and make the getter const. But we should
throw an exception here if it's null.

Likewise for WebRequest and ErrorCallback below.

[Wladimir Palant, 2013/04/18 13:59:55]

The reason for not doing it in the constructor was to avoid creating unnecessary
objects that will be replaced immediately. Are you sure you want this?
I would rather throw an exception in SetFileSystem() - how else can we get an
empty pointer?

[Felix Dahlke, 2013/04/18 14:04:12]

Oh right, was still thinking we pass them in to the constructor. Throwing an
exception in the setters sounds good.

+  return fileSystem;
+}
+
+void AdblockPlus::JsEngine::SetFileSystem(AdblockPlus::FileSystemPtr val)
+{
+  fileSystem = val;
+}
+
+AdblockPlus::WebRequestPtr AdblockPlus::JsEngine::GetWebRequest()
+{
+  if (!webRequest)
+    webRequest.reset(new DefaultWebRequest());
+  return webRequest;
+}
+
+void AdblockPlus::JsEngine::SetWebRequest(AdblockPlus::WebRequestPtr val)
+{
+  webRequest = val;
+}
+
+AdblockPlus::ErrorCallbackPtr AdblockPlus::JsEngine::GetErrorCallback()
+{
+  if (!errorCallback)
+    errorCallback.reset(new DefaultErrorCallback());
+  return errorCallback;
+}
+
+void AdblockPlus::JsEngine::SetErrorCallback(AdblockPlus::ErrorCallbackPtr val)
+{
+  errorCallback = val;
+}
+
 AdblockPlus::JsEngine::Context::Context(const JsEngine& jsEngine)
     : locker(jsEngine.isolate), handleScope(),
       contextScope(jsEngine.context)
 {
 }