[discardable] Fix setting task runner for Periodic Purge.

The task runner used to run the periodic periodic was not set properly,
causing DCHECKs to fail. This CL fixes this issue.

Periodic Purging continues to be disabled by default, so this CL causes
no change in functionality.

Bug: 1131857
Change-Id: If3bd626e9f5ac3c52b7d9e181605882c134d115e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2519032
Commit-Queue: Thiabaud Engelbrecht <thiabaud@google.com>
Reviewed-by: Peng Huang <penghuang@chromium.org>
Reviewed-by: Benoit L <lizeb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#825203}

components/discardable_memory/client/client_discardable_shared_memory_manager.cc

@@ -223,6 +223,11 @@ ClientDiscardableSharedMemoryManager::~ClientDiscardableSharedMemoryManager() {
   // which needs |manager_mojo_|.
   heap_.reset();
 
+  if (base::FeatureList::IsEnabled(kSchedulePeriodicPurge) &&
+      periodic_purge_task_runner_) {
+    DCHECK(periodic_purge_task_runner_->RunsTasksInCurrentSequence());
+  }
+
   // Delete the |manager_mojo_| on IO thread, so any pending tasks on IO thread
   // will be executed before the |manager_mojo_| is deleted.
   bool posted = io_task_runner_->DeleteSoon(FROM_HERE, manager_mojo_.release());
@@ -243,8 +248,19 @@ ClientDiscardableSharedMemoryManager::AllocateLockedDiscardableMemory(
     size_t size) {
   base::AutoLock lock(lock_);
 
-  if (timer_ == nullptr && periodic_purge_task_runner_)
-    StartScheduledPurging(periodic_purge_task_runner_);
+  if (base::FeatureList::IsEnabled(kSchedulePeriodicPurge)) {
+    if (timer_ == nullptr && periodic_purge_task_runner_) {
+      // A Timer can only be started from its sequence, but this does not
+      // necessarily execute on the right thread. Post a task to make sure
+      // we're starting the timer correctly.
+      timer_ = std::make_unique<base::RepeatingTimer>();
+      periodic_purge_task_runner_->PostTask(
+          FROM_HERE,
+          base::BindOnce(
+              &ClientDiscardableSharedMemoryManager::StartScheduledPurging,
+              weak_factory_.GetWeakPtr()));
+    }
+  }
 
   DCHECK_NE(size, 0u);
 
@@ -375,24 +391,23 @@ ClientDiscardableSharedMemoryManager::AllocateLockedDiscardableMemory(
   return std::move(discardable_memory);
 }
 
-void ClientDiscardableSharedMemoryManager::StartScheduledPurging(
-    scoped_refptr<base::SequencedTaskRunner> task_runner) {
-  DCHECK(task_runner);
-  if (base::FeatureList::IsEnabled(kSchedulePeriodicPurge)) {
+void ClientDiscardableSharedMemoryManager::StartScheduledPurging() {
   // The expected cost of purging should be very small (< 1ms), so it can be
   // scheduled frequently. However, we don't purge memory that has been
   // touched recently (see: |BackgroundPurge| and |kMinAgeForScheduledPurge|),
   // so there is no benefit to running this more than once per minute.
   constexpr base::TimeDelta kInterval = base::TimeDelta::FromMinutes(1);
 
-    timer_ = std::make_unique<base::RepeatingTimer>();
-    timer_->SetTaskRunner(task_runner);
+  base::AutoLock lock(lock_);
+  timer_->Start(
+      FROM_HERE, kInterval,
+      base::BindRepeating(&ClientDiscardableSharedMemoryManager::ScheduledPurge,
+                          weak_factory_.GetWeakPtr()));
+}
 
-    timer_->Start(FROM_HERE, kInterval,
-                  base::BindRepeating(
-                      &ClientDiscardableSharedMemoryManager::ScheduledPurge,
-                      base::Unretained(this)));
-  }
+void ClientDiscardableSharedMemoryManager::StopScheduledPurging() {
+  base::AutoLock lock(lock_);
+  timer_ = nullptr;
 }
 
 bool ClientDiscardableSharedMemoryManager::OnMemoryDump(
@@ -470,8 +485,15 @@ void ClientDiscardableSharedMemoryManager::ReleaseFreeMemory() {
     ReleaseFreeMemoryImpl();
   }
 
-  if (GetBytesAllocated() == 0)
-    timer_ = nullptr;
+  if (base::FeatureList::IsEnabled(kSchedulePeriodicPurge)) {
+    if (GetBytesAllocated() == 0 && periodic_purge_task_runner_) {
+      periodic_purge_task_runner_->PostTask(
+          FROM_HERE,
+          base::BindOnce(
+              &ClientDiscardableSharedMemoryManager::StopScheduledPurging,
+              weak_factory_.GetWeakPtr()));
+    }
+  }
 }
 
 void ClientDiscardableSharedMemoryManager::ReleaseFreeMemoryImpl() {

components/discardable_memory/client/client_discardable_shared_memory_manager.h

@@ -76,8 +76,8 @@ class DISCARDABLE_MEMORY_EXPORT ClientDiscardableSharedMemoryManager
       const char* name,
       base::trace_event::ProcessMemoryDump* pmd) const;
 
-  void StartScheduledPurging(
-      scoped_refptr<base::SequencedTaskRunner> task_runner);
+  void StartScheduledPurging();
+  void StopScheduledPurging();
 
   struct Statistics {
     size_t total_size;
@@ -101,7 +101,7 @@ class DISCARDABLE_MEMORY_EXPORT ClientDiscardableSharedMemoryManager
       scoped_refptr<base::SingleThreadTaskRunner> periodic_purge_task_runner);
   std::unique_ptr<DiscardableSharedMemoryHeap> heap_ GUARDED_BY(lock_);
   mutable base::Lock lock_;
-  std::unique_ptr<base::RepeatingTimer> timer_;
+  std::unique_ptr<base::RepeatingTimer> timer_ GUARDED_BY(lock_);
   scoped_refptr<base::SingleThreadTaskRunner> periodic_purge_task_runner_;
 
  private:
@@ -187,6 +187,9 @@ class DISCARDABLE_MEMORY_EXPORT ClientDiscardableSharedMemoryManager
   // RenderThreadImpl.
   bool foregrounded_ = false;
 
+  base::WeakPtrFactory<ClientDiscardableSharedMemoryManager> weak_factory_{
+      this};
+
   DISALLOW_COPY_AND_ASSIGN(ClientDiscardableSharedMemoryManager);
 };
 

components/discardable_memory/client/client_discardable_shared_memory_manager_unittest.cc

@@ -69,7 +69,10 @@ class TestClientDiscardableSharedMemoryManager
     return heap_->GetSizeOfFreeLists();
   }
 
-  base::RepeatingTimer* timer() const { return timer_.get(); }
+  bool TimerIsNull() const {
+    base::AutoLock lock(lock_);
+    return timer_ == nullptr;
+  }
 };
 
 class ClientDiscardableSharedMemoryManagerTest : public testing::Test {
@@ -395,27 +398,31 @@ TEST_F(ClientDiscardableSharedMemoryManagerPeriodicPurgingTest,
        SchedulingProactivePurging) {
   base::test::ScopedFeatureList fl;
   fl.InitAndEnableFeature(discardable_memory::kSchedulePeriodicPurge);
-  ASSERT_EQ(client_->timer(), nullptr);
+  ASSERT_TRUE(client_->TimerIsNull());
 
   // the amount of memory allocated here is arbitrary, we're only trying to get
   // the timer started.
   auto mem = client_->AllocateLockedDiscardableMemory(200);
 
-  EXPECT_NE(client_->timer(), nullptr);
+  task_env_.FastForwardBy(TimeDelta::FromSeconds(0));
+  EXPECT_FALSE(client_->TimerIsNull());
 
   // This does not destroy the timer because there is still memory allocated.
   client_->ReleaseFreeMemory();
 
-  EXPECT_NE(client_->timer(), nullptr);
+  task_env_.FastForwardBy(TimeDelta::FromSeconds(0));
+  EXPECT_FALSE(client_->TimerIsNull());
 
   mem = nullptr;
 
-  EXPECT_NE(client_->timer(), nullptr);
+  task_env_.FastForwardBy(TimeDelta::FromSeconds(0));
+  EXPECT_FALSE(client_->TimerIsNull());
 
   // Now that all memory is freed, destroy the timer.
   client_->ReleaseFreeMemory();
 
-  EXPECT_EQ(client_->timer(), nullptr);
+  task_env_.FastForwardBy(TimeDelta::FromSeconds(0));
+  EXPECT_TRUE(client_->TimerIsNull());
 }
 
 // This test is similar to the one above, but tests that creating and deleting
@@ -424,34 +431,40 @@ TEST_F(ClientDiscardableSharedMemoryManagerPeriodicPurgingTest,
        SchedulingProactivePurgingMultipleAllocations) {
   base::test::ScopedFeatureList fl;
   fl.InitAndEnableFeature(discardable_memory::kSchedulePeriodicPurge);
-  ASSERT_EQ(client_->timer(), nullptr);
+  ASSERT_TRUE(client_->TimerIsNull());
 
   // the amount of memory allocated here is arbitrary, we're only trying to get
   // the timer started.
   auto mem = client_->AllocateLockedDiscardableMemory(200);
   auto mem2 = client_->AllocateLockedDiscardableMemory(100);
 
-  EXPECT_NE(client_->timer(), nullptr);
+  task_env_.FastForwardBy(TimeDelta::FromSeconds(0));
+  EXPECT_FALSE(client_->TimerIsNull());
 
   client_->ReleaseFreeMemory();
 
-  EXPECT_NE(client_->timer(), nullptr);
+  task_env_.FastForwardBy(TimeDelta::FromSeconds(0));
+  EXPECT_FALSE(client_->TimerIsNull());
 
   mem = nullptr;
 
-  EXPECT_NE(client_->timer(), nullptr);
+  task_env_.FastForwardBy(TimeDelta::FromSeconds(0));
+  EXPECT_FALSE(client_->TimerIsNull());
 
   client_->ReleaseFreeMemory();
 
-  EXPECT_NE(client_->timer(), nullptr);
+  task_env_.FastForwardBy(TimeDelta::FromSeconds(0));
+  EXPECT_FALSE(client_->TimerIsNull());
 
   mem2 = nullptr;
 
-  EXPECT_NE(client_->timer(), nullptr);
+  task_env_.FastForwardBy(TimeDelta::FromSeconds(0));
+  EXPECT_FALSE(client_->TimerIsNull());
 
   client_->ReleaseFreeMemory();
 
-  EXPECT_EQ(client_->timer(), nullptr);
+  task_env_.FastForwardBy(TimeDelta::FromSeconds(0));
+  EXPECT_TRUE(client_->TimerIsNull());
 }
 
 }  // namespace