[discardable] Fix ClientDiscardableSharedMemoryManager teardown.

ClientDiscardableMemoryManager was modified to be ref-counted to guard
against potential use-after-frees, but without correctly arranging for
teardown on its "home" sequence, as required by the WeakPtrFactory API.

Based on https://chromium-review.googlesource.com/c/chromium/src/+/2577676.

Bug: 1153322
Change-Id: I7dca52cdf01ad4bdc95e8b87e57a9a7d92a28288
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2578384Reviewed-by: Peng Huang <penghuang@chromium.org>
Reviewed-by: Wez <wez@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#834696}

components/discardable_memory/client/client_discardable_shared_memory_manager.cc

@@ -190,11 +190,11 @@ ClientDiscardableSharedMemoryManager::ClientDiscardableSharedMemoryManager(
     mojo::PendingRemote<mojom::DiscardableSharedMemoryManager> manager,
     scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
     scoped_refptr<base::SingleThreadTaskRunner> periodic_purge_task_runner)
-    : heap_(std::make_unique<DiscardableSharedMemoryHeap>()),
-      periodic_purge_task_runner_(std::move(periodic_purge_task_runner)),
-      io_task_runner_(std::move(io_task_runner)),
-      manager_mojo_(std::make_unique<
-                    mojo::Remote<mojom::DiscardableSharedMemoryManager>>()) {
+    : ClientDiscardableSharedMemoryManager(
+          std::move(io_task_runner),
+          std::move(periodic_purge_task_runner)) {
+  manager_mojo_ =
+      std::make_unique<mojo::Remote<mojom::DiscardableSharedMemoryManager>>();
   base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider(
       this, "ClientDiscardableSharedMemoryManager",
       base::ThreadTaskRunnerHandle::Get());
@@ -206,7 +206,9 @@ ClientDiscardableSharedMemoryManager::ClientDiscardableSharedMemoryManager(
 ClientDiscardableSharedMemoryManager::ClientDiscardableSharedMemoryManager(
     scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
     scoped_refptr<base::SingleThreadTaskRunner> periodic_purge_task_runner)
-    : heap_(std::make_unique<DiscardableSharedMemoryHeap>()),
+    : RefCountedDeleteOnSequence<ClientDiscardableSharedMemoryManager>(
+          base::ThreadTaskRunnerHandle::Get()),
+      heap_(std::make_unique<DiscardableSharedMemoryHeap>()),
       periodic_purge_task_runner_(std::move(periodic_purge_task_runner)),
       io_task_runner_(std::move(io_task_runner)) {}
 

components/discardable_memory/client/client_discardable_shared_memory_manager.h

@@ -12,7 +12,7 @@
 
 #include "base/feature_list.h"
 #include "base/memory/discardable_memory_allocator.h"
-#include "base/memory/ref_counted.h"
+#include "base/memory/ref_counted_delete_on_sequence.h"
 #include "base/memory/unsafe_shared_memory_region.h"
 #include "base/synchronization/lock.h"
 #include "base/trace_event/memory_dump_provider.h"
@@ -35,7 +35,8 @@ DISCARDABLE_MEMORY_EXPORT extern const base::Feature kSchedulePeriodicPurge;
 class DISCARDABLE_MEMORY_EXPORT ClientDiscardableSharedMemoryManager
     : public base::DiscardableMemoryAllocator,
       public base::trace_event::MemoryDumpProvider,
-      public base::RefCountedThreadSafe<ClientDiscardableSharedMemoryManager> {
+      public base::RefCountedDeleteOnSequence<
+          ClientDiscardableSharedMemoryManager> {
  public:
   ClientDiscardableSharedMemoryManager(
       mojo::PendingRemote<mojom::DiscardableSharedMemoryManager> manager,
@@ -96,7 +97,10 @@ class DISCARDABLE_MEMORY_EXPORT ClientDiscardableSharedMemoryManager
   // These fields are only protected for testing, they would otherwise be
   // private. Everything else should be either public or private.
  protected:
-  friend class base::RefCountedThreadSafe<ClientDiscardableSharedMemoryManager>;
+  friend class base::RefCountedDeleteOnSequence<
+      ClientDiscardableSharedMemoryManager>;
+  friend class base::DeleteHelper<ClientDiscardableSharedMemoryManager>;
+
   ~ClientDiscardableSharedMemoryManager() override;
   ClientDiscardableSharedMemoryManager(
       scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,

components/discardable_memory/client/client_discardable_shared_memory_manager_unittest.cc

@@ -78,18 +78,7 @@ class TestClientDiscardableSharedMemoryManager
 
 class ClientDiscardableSharedMemoryManagerTest : public testing::Test {
  public:
-  void SetUp() override {
-    client_ =
-        base::MakeRefCounted<TestClientDiscardableSharedMemoryManager>(nullptr);
-  }
-  scoped_refptr<TestClientDiscardableSharedMemoryManager> client_;
-  const size_t page_size_ = base::GetPageSize();
-};
-
-class ClientDiscardableSharedMemoryManagerPeriodicPurgingTest
-    : public ClientDiscardableSharedMemoryManagerTest {
- public:
-  ClientDiscardableSharedMemoryManagerPeriodicPurgingTest()
+  ClientDiscardableSharedMemoryManagerTest()
       : task_env_(base::test::TaskEnvironment::MainThreadType::UI,
                   base::test::TaskEnvironment::TimeSource::MOCK_TIME) {}
   void SetUp() override {
@@ -97,6 +86,8 @@ class ClientDiscardableSharedMemoryManagerPeriodicPurgingTest
         task_env_.GetMainThreadTaskRunner());
   }
   base::test::TaskEnvironment task_env_;
+  scoped_refptr<TestClientDiscardableSharedMemoryManager> client_;
+  const size_t page_size_ = base::GetPageSize();
 };
 
 // This test allocates a single piece of memory, then verifies that calling
@@ -286,8 +277,7 @@ TEST_F(ClientDiscardableSharedMemoryManagerTest, ReleaseUnlocked) {
 // This tests that memory is actually removed by the periodic purging. We mock a
 // task runner for this test and fast forward to make sure that the memory is
 // purged at the right time.
-TEST_F(ClientDiscardableSharedMemoryManagerPeriodicPurgingTest,
-       ScheduledReleaseUnlocked) {
+TEST_F(ClientDiscardableSharedMemoryManagerTest, ScheduledReleaseUnlocked) {
   base::test::ScopedFeatureList fl;
   fl.InitAndEnableFeature(discardable_memory::kSchedulePeriodicPurge);
   ASSERT_EQ(client_->GetBytesAllocated(), 0u);
@@ -310,7 +300,7 @@ TEST_F(ClientDiscardableSharedMemoryManagerPeriodicPurgingTest,
 
 // Same as the above test, but tests that multiple pieces of memory will be
 // handled properly.
-TEST_F(ClientDiscardableSharedMemoryManagerPeriodicPurgingTest,
+TEST_F(ClientDiscardableSharedMemoryManagerTest,
        ScheduledReleaseUnlockedMultiple) {
   base::test::ScopedFeatureList fl;
   fl.InitAndEnableFeature(discardable_memory::kSchedulePeriodicPurge);
@@ -395,8 +385,7 @@ TEST_F(ClientDiscardableSharedMemoryManagerTest, LockingSuccessUma) {
 
 // Test that a repeating timer for background purging is created when we
 // allocate memory and discarded when we run out of allocated memory.
-TEST_F(ClientDiscardableSharedMemoryManagerPeriodicPurgingTest,
-       SchedulingProactivePurging) {
+TEST_F(ClientDiscardableSharedMemoryManagerTest, SchedulingProactivePurging) {
   base::test::ScopedFeatureList fl;
   fl.InitAndEnableFeature(discardable_memory::kSchedulePeriodicPurge);
   ASSERT_TRUE(client_->TimerIsNull());
@@ -428,7 +417,7 @@ TEST_F(ClientDiscardableSharedMemoryManagerPeriodicPurgingTest,
 
 // This test is similar to the one above, but tests that creating and deleting
 // the timer still works with multiple pieces of allocated memory.
-TEST_F(ClientDiscardableSharedMemoryManagerPeriodicPurgingTest,
+TEST_F(ClientDiscardableSharedMemoryManagerTest,
        SchedulingProactivePurgingMultipleAllocations) {
   base::test::ScopedFeatureList fl;
   fl.InitAndEnableFeature(discardable_memory::kSchedulePeriodicPurge);