Issue 5118 - Lock the platform interfaces before use

src/Platform.cpp

Index: src/Platform.cpp
===================================================================
--- a/src/Platform.cpp
+++ b/src/Platform.cpp
@@ -47,16 +47,21 @@
   ASSIGN_PLATFORM_PARAM(logSystem);
   ASSIGN_PLATFORM_PARAM(timer);
   ASSIGN_PLATFORM_PARAM(fileSystem);
   ASSIGN_PLATFORM_PARAM(webRequest);
 }
 
 Platform::~Platform()
 {
+  std::lock_guard<std::recursive_mutex> lock(interfacesMutex);
+  logSystem.reset();
+  timer.reset();

[sergei, 2017/09/13 19:20:53]

I'm afraid here is a dead lock:
Let's say `interfacesMutex` has been acquired here and the current thread is on
the line 56, meanwhile the timer callback is being fired and gets locked on the
same mutex, now our thread reaches the 57th line and destroys the timer which
will wait for the finishing of its thread what will never happen because the
timer's thread is waiting for the mutex.
I think it should be rather
{
  LogSystemPtr tmpLogSystem;
  ...
  {
    std::lock_guard<std::recursive_mutex> lock(interfacesMutex);
    tmpLogSystem = move(logSystem);
    ...
  }
  // here we can safely destroy the interfaces because the mutex is unlocked and
they can wait for their threads but any new scheduled tasks (functions passed
into WithSomeInterface) will be ignored.
}

[hub, 2017/09/13 19:50:49]

Good catch.

+  fileSystem.reset();
+  webRequest.reset();
 }
 
 void Platform::SetUpJsEngine(const AppInfo& appInfo, std::unique_ptr<IV8IsolateProvider> isolate)
 {
   std::lock_guard<std::mutex> lock(modulesMutex);
   if (jsEngine)
     return;
   jsEngine = JsEngine::New(appInfo, *this, std::move(isolate));
@@ -90,34 +95,42 @@
 }
 
 FilterEngine& Platform::GetFilterEngine()
 {
   CreateFilterEngineAsync();
   return *std::shared_future<FilterEnginePtr>(filterEngine).get();
 }
 
-ITimer& Platform::GetTimer()
+void Platform::WithTimer(const WithTimerCallback& callback)
 {
-  return *timer;
+  std::lock_guard<std::recursive_mutex> lock(interfacesMutex);
+  if (timer && callback)
+    callback(*timer);
 }
 
-IFileSystem& Platform::GetFileSystem()
+void Platform::WithFileSystem(const WithFileSystemCallback& callback)
 {
-  return *fileSystem;
+  std::lock_guard<std::recursive_mutex> lock(interfacesMutex);
+  if (fileSystem && callback)
+    callback(*fileSystem);
 }
 
-IWebRequest& Platform::GetWebRequest()
+void Platform::WithWebRequest(const WithWebRequestCallback& callback)
 {
-  return *webRequest;
+  std::lock_guard<std::recursive_mutex> lock(interfacesMutex);
+  if (webRequest && callback)
+    callback(*webRequest);
 }
 
-LogSystem& Platform::GetLogSystem()
+void Platform::WithLogSystem(const WithLogSystemCallback& callback)
 {
-  return *logSystem;
+  std::lock_guard<std::recursive_mutex> lock(interfacesMutex);
+  if (logSystem && callback)
+    callback(*logSystem);
 }
 
 namespace
 {
   class DefaultPlatform : public Platform
   {
   public:
     typedef std::shared_ptr<Scheduler> AsyncExecutorPtr;
@@ -182,9 +195,9 @@
   if (!fileSystem)
     CreateDefaultFileSystem();
   if (!webRequest)
     CreateDefaultWebRequest();
 
   std::unique_ptr<Platform> platform(new DefaultPlatform(asyncExecutor, std::move(*this)));
   asyncExecutor.reset();
   return platform;
-}
+}