discardable_memory: fix a use-after-free issue in asan build during shutdown

And this CL also fixes an issue to make sure ppapi plugin will request the
discardable memory interface from ui service instead of browser when the
browser is launched with --mus flag.

Bug: 791119
Change-Id: I1ce2b256c2280fe02c364aff8d199ab4c43618f0
Reviewed-on: https://chromium-review.googlesource.com/806482Reviewed-by: David Reveman <reveman@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: Peng Huang <penghuang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521991}

components/discardable_memory/service/discardable_shared_memory_manager.cc

@@ -5,6 +5,7 @@
 #include "components/discardable_memory/service/discardable_shared_memory_manager.h"
 
 #include <algorithm>
+#include <memory>
 #include <utility>
 
 #include "base/atomic_sequence_num.h"
@@ -15,12 +16,12 @@
 #include "base/macros.h"
 #include "base/memory/discardable_memory.h"
 #include "base/memory/memory_coordinator_client_registry.h"
-#include "base/memory/ptr_util.h"
 #include "base/memory/shared_memory_tracker.h"
 #include "base/numerics/safe_math.h"
 #include "base/process/memory.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/sys_info.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/trace_event/memory_allocator_dump.h"
@@ -52,12 +53,14 @@ class MojoDiscardableSharedMemoryManagerImpl
  public:
   MojoDiscardableSharedMemoryManagerImpl(
       int32_t client_id,
-      ::discardable_memory::DiscardableSharedMemoryManager* manager)
+      base::WeakPtr<::discardable_memory::DiscardableSharedMemoryManager>
+          manager)
       : client_id_(client_id), manager_(manager) {}
 
   ~MojoDiscardableSharedMemoryManagerImpl() override {
     // Remove this client from the |manager_|, so all allocated discardable
     // memory belong to this client will be released.
+    if (manager_)
       manager_->ClientRemoved(client_id_);
   }
 
@@ -67,20 +70,24 @@ class MojoDiscardableSharedMemoryManagerImpl
       int32_t id,
       AllocateLockedDiscardableSharedMemoryCallback callback) override {
     base::SharedMemoryHandle handle;
+    mojo::ScopedSharedBufferHandle memory;
+    if (manager_) {
       manager_->AllocateLockedDiscardableSharedMemoryForClient(client_id_, size,
                                                                id, &handle);
-    mojo::ScopedSharedBufferHandle memory =
+      memory =
           mojo::WrapSharedMemoryHandle(handle, size, false /* read_only */);
+    }
     std::move(callback).Run(std::move(memory));
   }
 
   void DeletedDiscardableSharedMemory(int32_t id) override {
+    if (manager_)
       manager_->ClientDeletedDiscardableSharedMemory(id, client_id_);
   }
 
  private:
   const int32_t client_id_;
-  ::discardable_memory::DiscardableSharedMemoryManager* const manager_;
+  base::WeakPtr<::discardable_memory::DiscardableSharedMemoryManager> manager_;
 
   DISALLOW_COPY_AND_ASSIGN(MojoDiscardableSharedMemoryManagerImpl);
 };
@@ -215,7 +222,9 @@ DiscardableSharedMemoryManager::DiscardableSharedMemoryManager()
       // Current thread might not have a task runner in tests.
       enforce_memory_policy_task_runner_(base::ThreadTaskRunnerHandle::Get()),
       enforce_memory_policy_pending_(false),
-      weak_ptr_factory_(this) {
+      mojo_thread_message_loop_(nullptr),
+      weak_ptr_factory_(this),
+      mojo_thread_weak_ptr_factory_(this) {
   DCHECK_NE(memory_limit_, 0u);
   enforce_memory_policy_callback_ =
       base::Bind(&DiscardableSharedMemoryManager::EnforceMemoryPolicy,
@@ -230,14 +239,43 @@ DiscardableSharedMemoryManager::~DiscardableSharedMemoryManager() {
   base::MemoryCoordinatorClientRegistry::GetInstance()->Unregister(this);
   base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider(
       this);
+
+  if (mojo_thread_message_loop_) {
+    if (mojo_thread_message_loop_ == base::MessageLoop::current()) {
+      mojo_thread_message_loop_->RemoveDestructionObserver(this);
+      mojo_thread_message_loop_ = nullptr;
+    } else {
+      // If mojom::DiscardableSharedMemoryManager implementation is running in
+      // another thread, we need invalidate all related weak ptrs on that
+      // thread.
+      base::WaitableEvent event(
+          base::WaitableEvent::ResetPolicy::MANUAL,
+          base::WaitableEvent::InitialState::NOT_SIGNALED);
+      bool result = mojo_thread_message_loop_->task_runner()->PostTask(
+          FROM_HERE,
+          base::BindOnce(
+              &DiscardableSharedMemoryManager::InvalidateMojoThreadWeakPtrs,
+              base::Unretained(this), &event));
+      LOG_IF(ERROR, !result) << "Invalidate mojo weak ptrs failed!";
+      if (result)
+        event.Wait();
+    }
+  }
 }
 
 void DiscardableSharedMemoryManager::Bind(
     mojom::DiscardableSharedMemoryManagerRequest request,
     const service_manager::BindSourceInfo& source_info) {
+  DCHECK(!mojo_thread_message_loop_ ||
+         mojo_thread_message_loop_ == base::MessageLoop::current());
+  if (!mojo_thread_message_loop_) {
+    mojo_thread_message_loop_ = base::MessageLoop::current();
+    mojo_thread_message_loop_->AddDestructionObserver(this);
+  }
+
   mojo::MakeStrongBinding(
-      base::MakeUnique<MojoDiscardableSharedMemoryManagerImpl>(
-          next_client_id_++, this),
+      std::make_unique<MojoDiscardableSharedMemoryManagerImpl>(
+          next_client_id_++, mojo_thread_weak_ptr_factory_.GetWeakPtr()),
       std::move(request));
 }
 
@@ -258,7 +296,7 @@ DiscardableSharedMemoryManager::AllocateLockedDiscardableMemory(size_t size) {
     base::TerminateBecauseOutOfMemory(size);
   // Close file descriptor to avoid running out.
   memory->Close();
-  return base::MakeUnique<DiscardableMemoryImpl>(
+  return std::make_unique<DiscardableMemoryImpl>(
       std::move(memory),
       base::Bind(
           &DiscardableSharedMemoryManager::DeletedDiscardableSharedMemory,
@@ -390,6 +428,16 @@ void DiscardableSharedMemoryManager::OnPurgeMemory() {
   ReduceMemoryUsageUntilWithinLimit(0);
 }
 
+void DiscardableSharedMemoryManager::WillDestroyCurrentMessageLoop() {
+  // The mojo thead is going to be destroyed. We should invalidate all related
+  // weak ptrs and remove the destrunction observer.
+  DCHECK_EQ(mojo_thread_message_loop_, base::MessageLoop::current());
+  DLOG_IF(WARNING, mojo_thread_weak_ptr_factory_.HasWeakPtrs())
+      << "Some MojoDiscardableSharedMemoryManagerImpls are still alive. They "
+         "will be leaked.";
+  InvalidateMojoThreadWeakPtrs(nullptr);
+}
+
 void DiscardableSharedMemoryManager::AllocateLockedDiscardableSharedMemory(
     int client_id,
     size_t size,
@@ -592,4 +640,14 @@ void DiscardableSharedMemoryManager::ScheduleEnforceMemoryPolicy() {
       base::TimeDelta::FromMilliseconds(kEnforceMemoryPolicyDelayMs));
 }
 
+void DiscardableSharedMemoryManager::InvalidateMojoThreadWeakPtrs(
+    base::WaitableEvent* event) {
+  DCHECK_EQ(mojo_thread_message_loop_, base::MessageLoop::current());
+  mojo_thread_weak_ptr_factory_.InvalidateWeakPtrs();
+  mojo_thread_message_loop_->RemoveDestructionObserver(this);
+  mojo_thread_message_loop_ = nullptr;
+  if (event)
+    event->Signal();
+}
+
 }  // namespace discardable_memory

components/discardable_memory/service/discardable_shared_memory_manager.h

@@ -22,6 +22,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/shared_memory.h"
 #include "base/memory/weak_ptr.h"
+#include "base/message_loop/message_loop.h"
 #include "base/process/process_handle.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -29,6 +30,10 @@
 #include "components/discardable_memory/common/discardable_memory_export.h"
 #include "components/discardable_memory/public/interfaces/discardable_shared_memory_manager.mojom.h"
 
+namespace base {
+class WaitableEvent;
+}
+
 namespace service_manager {
 struct BindSourceInfo;
 }
@@ -43,7 +48,8 @@ namespace discardable_memory {
 class DISCARDABLE_MEMORY_EXPORT DiscardableSharedMemoryManager
     : public base::DiscardableMemoryAllocator,
       public base::trace_event::MemoryDumpProvider,
-      public base::MemoryCoordinatorClient {
+      public base::MemoryCoordinatorClient,
+      public base::MessageLoop::DestructionObserver {
  public:
   DiscardableSharedMemoryManager();
   ~DiscardableSharedMemoryManager() override;
@@ -114,6 +120,9 @@ class DISCARDABLE_MEMORY_EXPORT DiscardableSharedMemoryManager
   void OnMemoryStateChange(base::MemoryState state) override;
   void OnPurgeMemory() override;
 
+  // base::MessageLoop::DestructionObserver implementation:
+  void WillDestroyCurrentMessageLoop() override;
+
   void AllocateLockedDiscardableSharedMemory(
       int client_id,
       size_t size,
@@ -131,6 +140,9 @@ class DISCARDABLE_MEMORY_EXPORT DiscardableSharedMemoryManager
   virtual base::Time Now() const;
   virtual void ScheduleEnforceMemoryPolicy();
 
+  // Invalidate weak pointers for the mojo thread.
+  void InvalidateMojoThreadWeakPtrs(base::WaitableEvent* event);
+
   int32_t next_client_id_;
 
   base::Lock lock_;
@@ -150,8 +162,17 @@ class DISCARDABLE_MEMORY_EXPORT DiscardableSharedMemoryManager
       enforce_memory_policy_task_runner_;
   base::Closure enforce_memory_policy_callback_;
   bool enforce_memory_policy_pending_;
+
+  // The message loop for running mojom::DiscardableSharedMemoryManger
+  // implementations.
+  base::MessageLoop* mojo_thread_message_loop_;
+
   base::WeakPtrFactory<DiscardableSharedMemoryManager> weak_ptr_factory_;
 
+  // WeakPtrFractory for generating weak pointers used in the mojo thread.
+  base::WeakPtrFactory<DiscardableSharedMemoryManager>
+      mojo_thread_weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(DiscardableSharedMemoryManager);
 };
 

content/browser/ppapi_plugin_process_host.cc

@@ -372,6 +372,9 @@ bool PpapiPluginProcessHost::Init(const PepperPluginInfo& info) {
       service_manager::switches::kDisableSeccompFilterSandbox,
 #if defined(OS_MACOSX)
       switches::kEnableSandboxLogging,
+#endif
+#if defined(USE_AURA)
+      switches::kMus,
 #endif
       switches::kNoSandbox,
       switches::kPpapiStartupDialog,

content/browser/service_manager/common_browser_interfaces.cc

@@ -8,6 +8,7 @@
 #include <utility>
 
 #include "base/callback.h"
+#include "base/command_line.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/task_runner.h"
@@ -19,6 +20,7 @@
 #include "device/geolocation/geolocation_config.h"
 #include "mojo/public/cpp/bindings/interface_request.h"
 #include "services/service_manager/public/cpp/binder_registry.h"
+#include "ui/base/ui_base_switches.h"
 
 #if defined(OS_WIN)
 #include "content/common/font_cache_dispatcher_win.h"
@@ -28,24 +30,38 @@ namespace content {
 
 namespace {
 
+bool IsRunningWithMus() {
+#if BUILDFLAG(ENABLE_MUS)
+  const base::CommandLine* cmdline = base::CommandLine::ForCurrentProcess();
+  return cmdline->HasSwitch(switches::kMus);
+#else
+  return false;
+#endif
+}
+
 class ConnectionFilterImpl : public ConnectionFilter {
  public:
   ConnectionFilterImpl()
       : main_thread_task_runner_(base::ThreadTaskRunnerHandle::Get()) {
-    RegisterMainThreadInterface(base::Bind(&device::GeolocationConfig::Create));
+    RegisterMainThreadInterface(
+        base::BindRepeating(&device::GeolocationConfig::Create));
 #if defined(OS_WIN)
-    registry_.AddInterface(base::Bind(&FontCacheDispatcher::Create));
+    registry_.AddInterface(base::BindRepeating(&FontCacheDispatcher::Create));
 #endif
+    if (!IsRunningWithMus()) {
+      // For mus, the mojom::discardable_memory::DiscardableSharedMemoryManager
+      // is exposed from ui::Service. So we don't need bind the interface here.
       auto* browser_main_loop = BrowserMainLoop::GetInstance();
       if (browser_main_loop) {
         auto* manager = browser_main_loop->discardable_shared_memory_manager();
         if (manager) {
-        registry_.AddInterface(base::Bind(
+          registry_.AddInterface(base::BindRepeating(
               &discardable_memory::DiscardableSharedMemoryManager::Bind,
               base::Unretained(manager)));
         }
       }
     }
+  }
 
   ~ConnectionFilterImpl() override {}