Issue 5118 - Lock the platform interfaces before use

src/Platform.cpp

 /*
 * This file is part of Adblock Plus <https://adblockplus.org/>,
 * Copyright (C) 2006-present 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 77 matching lines @@
       onCreated(*filterEngine);
   }, parameters);
 }
 
 FilterEngine& Platform::GetFilterEngine()
 {
   CreateFilterEngineAsync();
   return *std::shared_future<FilterEnginePtr>(filterEngine).get();
 }
 
-ITimer& Platform::GetTimer()
+void Platform::WithTimer(const WithTimerCallback& callback)
 {
-  return *timer;
+  if (timer && callback)
+    callback(*timer);
 }
 
-IFileSystem& Platform::GetFileSystem()
+void Platform::WithFileSystem(const WithFileSystemCallback& callback)
 {
-  return *fileSystem;
+  if (fileSystem && callback)
+    callback(*fileSystem);
 }
 
-IWebRequest& Platform::GetWebRequest()
+void Platform::WithWebRequest(const WithWebRequestCallback& callback)
 {
-  return *webRequest;
+  if (webRequest && callback)
+    callback(*webRequest);
 }
 
-LogSystem& Platform::GetLogSystem()
+void Platform::WithLogSystem(const WithLogSystemCallback& callback)
 {
-  return *logSystem;
+  if (logSystem && callback)
+    callback(*logSystem);
 }
 
 namespace
 {
   class DefaultPlatform : public Platform
   {
   public:
     typedef std::shared_ptr<Scheduler> AsyncExecutorPtr;
     explicit DefaultPlatform(const AsyncExecutorPtr& asyncExecutor, CreationParameters&& creationParams)
       : Platform(std::move(creationParams)), asyncExecutor(asyncExecutor)
     {
     }
-    ~DefaultPlatform()
-    {
-      asyncExecutor.reset();
-    }
+    ~DefaultPlatform();
+
+    virtual void WithTimer(const WithTimerCallback&) override;

[sergei, 2017/09/13 20:06:37]

Could you please remove the `virtual` keyword here for overridden methods
because it's already clear that they are virtual from `override`.

[hub, 2017/09/13 20:17:51]

Done.

+    virtual void WithFileSystem(const WithFileSystemCallback&) override;
+    virtual void WithWebRequest(const WithWebRequestCallback&) override;
+    virtual void WithLogSystem(const WithLogSystemCallback&) override;
+
   private:
     AsyncExecutorPtr asyncExecutor;
+    std::recursive_mutex interfacesMutex;
   };
+
+  DefaultPlatform::~DefaultPlatform()
+  {
+    asyncExecutor.reset();
+    LogSystemPtr tmpLogSystem;
+    TimerPtr tmpTimer;
+    FileSystemPtr tmpFileSystem;
+    WebRequestPtr tmpWebRequest;
+    {
+      std::lock_guard<std::recursive_mutex> lock(interfacesMutex);
+      tmpLogSystem = std::move(logSystem);
+      tmpTimer = std::move(timer);
+      tmpFileSystem = std::move(fileSystem);
+      tmpWebRequest = std::move(webRequest);
+    }
+    tmpLogSystem.reset();
+    tmpTimer.reset();
+    tmpFileSystem.reset();
+    tmpWebRequest.reset();

[sergei, 2017/09/13 20:06:37]

I think this calls of reset are not required.

[hub, 2017/09/13 20:17:51]

Done.

+  }
+
+  void DefaultPlatform::WithTimer(const WithTimerCallback& callback)
+  {
+    std::lock_guard<std::recursive_mutex> lock(interfacesMutex);
+    Platform::WithTimer(callback);
+  }
+
+  void DefaultPlatform::WithFileSystem(const WithFileSystemCallback& callback)
+  {
+    std::lock_guard<std::recursive_mutex> lock(interfacesMutex);
+    Platform::WithFileSystem(callback);
+  }
+
+  void DefaultPlatform::WithWebRequest(const WithWebRequestCallback& callback)
+  {
+    std::lock_guard<std::recursive_mutex> lock(interfacesMutex);
+    Platform::WithWebRequest(callback);
+  }
+
+  void DefaultPlatform::WithLogSystem(const WithLogSystemCallback& callback)
+  {
+    std::lock_guard<std::recursive_mutex> lock(interfacesMutex);
+    Platform::WithLogSystem(callback);
+  }
 }
 
 Scheduler DefaultPlatformBuilder::GetDefaultAsyncExecutor()
 {
   if (!defaultScheduler)
   {
     asyncExecutor = std::make_shared<Scheduler>(::DummyScheduler);
     std::weak_ptr<Scheduler> weakExecutor = asyncExecutor;
     defaultScheduler = [weakExecutor](const SchedulerTask& task)
     {
@@ skipping 35 matching lines @@
   if (!timer)
     CreateDefaultTimer();
   if (!fileSystem)
     CreateDefaultFileSystem();
   if (!webRequest)
     CreateDefaultWebRequest();
 
   std::unique_ptr<Platform> platform(new DefaultPlatform(asyncExecutor, std::move(*this)));
   asyncExecutor.reset();
   return platform;
-}
+}