Force HostContentSettingsMap to be destroyed on its owning thread.

It uses WeakPtrFactory and as such must be destroyed on its owning thread (UI).

The rest of the changes can then be inferred from the stack trace on the bug:

1) RefcountedKeyedServiceTraits::Destruct() must check for
   ThreadTaskRunnerHandle::IsSet() before invoking TTRH::Get().
  ** UPDATE in PS 5 : Use STR::RunsTasksOnCurrentThread() instead of STRH comparison.

2) Nothing about RefcountedKeyedService's use of its |task_runner| is thread-affine
   => using SequencedTaskRunner instead is inline with our desire to make all
   top-level API use sequence-affinity instead of thread-affinity for thread-safety.

3) Calling SequencedTaskRunnerHandle::IsSet() from the Callback's destructor requires
   the SequencedWorkerPool's task info to be set in that scope
   => brought changes from same requirements in another CL
      @ https://codereview.chromium.org/2491613004/#ps160001
   Note: intentionally not adding tests as this change highlights that this deletion is
   racy in theory (deleting without owning the sequence means potentially deleting
   things from the same SequenceToken in parallel... any good test would fail TSAN and
   I also don't want to address this issue given it's long standing and SWP is being
   replaced by TaskScheduler shortly).

4) Switch to ScopedMockTimeMessageLoopTaskRunner in NotificationPermissionContextTest
   => otherwise LSAN catches a leak in patch set 3 as the HostContentSettingsMap is
      sent for deletion on the wrong task runner and the task is never flushed.
      (ScopedMockTimeMessageLoopTaskRunner ensures the loop is flushed before replacing
       it, both ways).

Precursor requirement to https://codereview.chromium.org/2576243003/ which
for some reason makes this destruction race come to life.

BUG=674946, 665588, 622400
TBR=mukai (NotificationPermissionContextTest side-effects)

Review-Url: https://codereview.chromium.org/2581213002
Cr-Commit-Position: refs/heads/master@{#439534}

base/threading/sequenced_worker_pool.cc

@@ -383,6 +383,12 @@ class SequencedWorkerPool::Inner {
     CLEANUP_DONE,
   };
 
+  // Clears ScheduledTasks in |tasks_to_delete| while ensuring that
+  // |this_worker| has the desired task info context during ~ScheduledTask() to
+  // allow sequence-checking.
+  void DeleteWithoutLock(std::vector<SequencedTask>* tasks_to_delete,
+                         Worker* this_worker);
+
   // Helper used by PostTask() to complete the work when redirection is on.
   // Returns true if the task may run at some point in the future and false if
   // it will definitely not run.
@@ -420,7 +426,7 @@ class SequencedWorkerPool::Inner {
   // See the implementation for a more detailed description.
   GetWorkStatus GetWork(SequencedTask* task,
                         TimeDelta* wait_time,
-                        std::vector<Closure>* delete_these_outside_lock);
+                        std::vector<SequencedTask>* delete_these_outside_lock);
 
   void HandleCleanup();
 
@@ -983,7 +989,7 @@ void SequencedWorkerPool::Inner::ThreadLoop(Worker* this_worker) {
       // See GetWork for what delete_these_outside_lock is doing.
       SequencedTask task;
       TimeDelta wait_time;
-      std::vector<Closure> delete_these_outside_lock;
+      std::vector<SequencedTask> delete_these_outside_lock;
       GetWorkStatus status =
           GetWork(&task, &wait_time, &delete_these_outside_lock);
       if (status == GET_WORK_FOUND) {
@@ -1000,7 +1006,7 @@ void SequencedWorkerPool::Inner::ThreadLoop(Worker* this_worker) {
           // already get a signal for each new task, but it doesn't
           // hurt.)
           SignalHasWork();
-          delete_these_outside_lock.clear();
+          DeleteWithoutLock(&delete_these_outside_lock, this_worker);
 
           // Complete thread creation outside the lock if necessary.
           if (new_thread_id)
@@ -1031,7 +1037,7 @@ void SequencedWorkerPool::Inner::ThreadLoop(Worker* this_worker) {
         switch (status) {
           case GET_WORK_WAIT: {
               AutoUnlock unlock(lock_);
-              delete_these_outside_lock.clear();
+              DeleteWithoutLock(&delete_these_outside_lock, this_worker);
             }
             break;
           case GET_WORK_NOT_FOUND:
@@ -1053,7 +1059,7 @@ void SequencedWorkerPool::Inner::ThreadLoop(Worker* this_worker) {
         // help this case.
         if (shutdown_called_ && blocking_shutdown_pending_task_count_ == 0) {
           AutoUnlock unlock(lock_);
-          delete_these_outside_lock.clear();
+          DeleteWithoutLock(&delete_these_outside_lock, this_worker);
           break;
         }
 
@@ -1061,7 +1067,7 @@ void SequencedWorkerPool::Inner::ThreadLoop(Worker* this_worker) {
         // deletion must happen outside of the lock.
         if (delete_these_outside_lock.size()) {
           AutoUnlock unlock(lock_);
-          delete_these_outside_lock.clear();
+          DeleteWithoutLock(&delete_these_outside_lock, this_worker);
 
           // Since the lock has been released, |status| may no longer be
           // accurate. It might read GET_WORK_WAIT even if there are tasks
@@ -1084,6 +1090,9 @@ void SequencedWorkerPool::Inner::ThreadLoop(Worker* this_worker) {
         }
         waiting_thread_count_--;
       }
+      // |delete_these_outside_lock| should have been cleared via
+      // DeleteWithoutLock() above already.
+      DCHECK(delete_these_outside_lock.empty());
     }
   }  // Release lock_.
 
@@ -1095,6 +1104,19 @@ void SequencedWorkerPool::Inner::ThreadLoop(Worker* this_worker) {
   can_shutdown_cv_.Signal();
 }
 
+void SequencedWorkerPool::Inner::DeleteWithoutLock(
+    std::vector<SequencedTask>* tasks_to_delete,
+    Worker* this_worker) {
+  while (!tasks_to_delete->empty()) {
+    const SequencedTask& deleted_task = tasks_to_delete->back();
+    this_worker->set_running_task_info(
+        SequenceToken(deleted_task.sequence_token_id),
+        deleted_task.shutdown_behavior);
+    tasks_to_delete->pop_back();
+  }
+  this_worker->reset_running_task_info();
+}
+
 void SequencedWorkerPool::Inner::HandleCleanup() {
   DCHECK_EQ(AllPoolsState::USE_WORKER_POOL, g_all_pools_state);
 
@@ -1162,7 +1184,7 @@ int64_t SequencedWorkerPool::Inner::LockedGetNextSequenceTaskNumber() {
 SequencedWorkerPool::Inner::GetWorkStatus SequencedWorkerPool::Inner::GetWork(
     SequencedTask* task,
     TimeDelta* wait_time,
-    std::vector<Closure>* delete_these_outside_lock) {
+    std::vector<SequencedTask>* delete_these_outside_lock) {
   DCHECK_EQ(AllPoolsState::USE_WORKER_POOL, g_all_pools_state);
 
   lock_.AssertAcquired();
@@ -1208,18 +1230,17 @@ SequencedWorkerPool::Inner::GetWorkStatus SequencedWorkerPool::Inner::GetWork(
       // shutdown. Delete it and get more work.
       //
       // Note that we do not want to delete unrunnable tasks. Deleting a task
-      // can have side effects (like freeing some objects) and deleting a
-      // task that's supposed to run after one that's currently running could
-      // cause an obscure crash.
+      // can have side effects (like freeing some objects) and deleting a task
+      // that's supposed to run after one that's currently running could cause
+      // an obscure crash.
       //
       // We really want to delete these tasks outside the lock in case the
-      // closures are holding refs to objects that want to post work from
-      // their destructorss (which would deadlock). The closures are
-      // internally refcounted, so we just need to keep a copy of them alive
-      // until the lock is exited. The calling code can just clear() the
-      // vector they passed to us once the lock is exited to make this
-      // happen.
-      delete_these_outside_lock->push_back(i->task);
+      // closures are holding refs to objects that want to post work from their
+      // destructors (which would deadlock). The closures are internally
+      // refcounted, so we just need to keep a copy of them alive until the lock
+      // is exited. The calling code can just clear() the vector they passed to
+      // us once the lock is exited to make this happen.
+      delete_these_outside_lock->push_back(*i);
       pending_tasks_.erase(i++);
       continue;
     }
@@ -1230,7 +1251,7 @@ SequencedWorkerPool::Inner::GetWorkStatus SequencedWorkerPool::Inner::GetWork(
       status = GET_WORK_WAIT;
       if (cleanup_state_ == CLEANUP_RUNNING) {
         // Deferred tasks are deleted when cleaning up, see Inner::ThreadLoop.
-        delete_these_outside_lock->push_back(i->task);
+        delete_these_outside_lock->push_back(*i);
         pending_tasks_.erase(i);
       }
       break;

chrome/browser/notifications/notification_permission_context_unittest.cc

@@ -5,9 +5,8 @@
 #include "chrome/browser/notifications/notification_permission_context.h"
 
 #include "base/bind.h"
-#include "base/message_loop/message_loop.h"
-#include "base/test/test_mock_time_task_runner.h"
-#include "base/threading/thread_task_runner_handle.h"
+#include "base/memory/ptr_util.h"
+#include "base/test/scoped_mock_time_message_loop_task_runner.h"
 #include "base/time/time.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/notifications/desktop_notification_profile_util.h"
@@ -81,24 +80,19 @@ class TestNotificationPermissionContext : public NotificationPermissionContext {
 class NotificationPermissionContextTest
     : public ChromeRenderViewHostTestHarness {
  public:
-  scoped_refptr<base::TestMockTimeTaskRunner> SwitchToMockTime() {
-    old_task_runner_ = base::ThreadTaskRunnerHandle::Get();
-    scoped_refptr<base::TestMockTimeTaskRunner> task_runner(
-        new base::TestMockTimeTaskRunner(base::Time::Now(),
-                                         base::TimeTicks::Now()));
-    base::MessageLoop::current()->SetTaskRunner(task_runner);
-    return task_runner;
-  }
-
   void TearDown() override {
-    if (old_task_runner_) {
-      base::MessageLoop::current()->SetTaskRunner(old_task_runner_);
-      old_task_runner_ = nullptr;
-    }
+    mock_time_task_runner_.reset();
     ChromeRenderViewHostTestHarness::TearDown();
   }
 
  protected:
+  base::TestMockTimeTaskRunner* SwitchToMockTime() {
+    EXPECT_FALSE(mock_time_task_runner_);
+    mock_time_task_runner_ =
+        base::MakeUnique<base::ScopedMockTimeMessageLoopTaskRunner>();
+    return mock_time_task_runner_->task_runner();
+  }
+
   void UpdateContentSetting(NotificationPermissionContext* context,
                             const GURL& requesting_origin,
                             const GURL& embedding_origin,
@@ -107,7 +101,8 @@ class NotificationPermissionContextTest
   }
 
  private:
-  scoped_refptr<base::SingleThreadTaskRunner> old_task_runner_;
+  std::unique_ptr<base::ScopedMockTimeMessageLoopTaskRunner>
+      mock_time_task_runner_;
 };
 
 // Web Notification permission requests will completely ignore the embedder
@@ -215,7 +210,7 @@ TEST_F(NotificationPermissionContextTest, TestDenyInIncognitoAfterDelay) {
                                web_contents()->GetMainFrame()->GetRoutingID(),
                                -1);
 
-  scoped_refptr<base::TestMockTimeTaskRunner> task_runner(SwitchToMockTime());
+  base::TestMockTimeTaskRunner* task_runner = SwitchToMockTime();
 
   ASSERT_EQ(0, permission_context.permission_set_count());
   ASSERT_FALSE(permission_context.last_permission_set_persisted());
@@ -281,7 +276,7 @@ TEST_F(NotificationPermissionContextTest, TestCancelledIncognitoRequest) {
                                web_contents()->GetMainFrame()->GetRoutingID(),
                                -1);
 
-  scoped_refptr<base::TestMockTimeTaskRunner> task_runner(SwitchToMockTime());
+  base::TestMockTimeTaskRunner* task_runner = SwitchToMockTime();
 
   content::PermissionManager* permission_manager =
       PermissionManagerFactory::GetForProfile(
@@ -318,7 +313,7 @@ TEST_F(NotificationPermissionContextTest, TestParallelDenyInIncognito) {
                                 web_contents()->GetMainFrame()->GetRoutingID(),
                                 1);
 
-  scoped_refptr<base::TestMockTimeTaskRunner> task_runner(SwitchToMockTime());
+  base::TestMockTimeTaskRunner* task_runner = SwitchToMockTime();
 
   ASSERT_EQ(0, permission_context.permission_set_count());
   ASSERT_FALSE(permission_context.last_permission_set_persisted());

components/content_settings/core/browser/cookie_settings_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "components/content_settings/core/browser/cookie_settings.h"
 
+#include "base/message_loop/message_loop.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/content_settings/core/common/content_settings_pattern.h"
 #include "components/content_settings/core/common/pref_names.h"
@@ -38,6 +39,10 @@ class CookieSettingsTest : public testing::Test {
   ~CookieSettingsTest() override { settings_map_->ShutdownOnUIThread(); }
 
  protected:
+  // There must be a valid ThreadTaskRunnerHandle in HostContentSettingsMap's
+  // scope.
+  base::MessageLoop message_loop_;
+
   sync_preferences::TestingPrefServiceSyncable prefs_;
   scoped_refptr<HostContentSettingsMap> settings_map_;
   scoped_refptr<CookieSettings> cookie_settings_;

components/content_settings/core/browser/host_content_settings_map.cc

@@ -14,6 +14,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "base/time/clock.h"
 #include "build/build_config.h"
 #include "components/content_settings/core/browser/content_settings_default_provider.h"
@@ -178,7 +179,7 @@ content_settings::PatternPair GetPatternsForContentSettingsType(
 HostContentSettingsMap::HostContentSettingsMap(PrefService* prefs,
                                                bool is_incognito_profile,
                                                bool is_guest_profile)
-    :
+    : RefcountedKeyedService(base::ThreadTaskRunnerHandle::Get()),
 #ifndef NDEBUG
       used_from_thread_id_(base::PlatformThread::CurrentId()),
 #endif
@@ -827,6 +828,7 @@ void HostContentSettingsMap::OnContentSettingChanged(
 }
 
 HostContentSettingsMap::~HostContentSettingsMap() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!prefs_);
 }
 

components/keyed_service/core/refcounted_keyed_service.cc

@@ -4,15 +4,13 @@
 
 #include "components/keyed_service/core/refcounted_keyed_service.h"
 
-#include "base/location.h"
-#include "base/threading/thread_task_runner_handle.h"
+#include <utility>
 
 namespace impl {
 
 // static
 void RefcountedKeyedServiceTraits::Destruct(const RefcountedKeyedService* obj) {
-  if (obj->task_runner_.get() != nullptr &&
-      obj->task_runner_.get() != base::ThreadTaskRunnerHandle::Get()) {
+  if (obj->task_runner_ && !obj->task_runner_->RunsTasksOnCurrentThread()) {
     obj->task_runner_->DeleteSoon(FROM_HERE, obj);
   } else {
     delete obj;
@@ -25,9 +23,7 @@ RefcountedKeyedService::RefcountedKeyedService() : task_runner_(nullptr) {
 }
 
 RefcountedKeyedService::RefcountedKeyedService(
-    const scoped_refptr<base::SingleThreadTaskRunner>& task_runner)
-    : task_runner_(task_runner) {
-}
+    scoped_refptr<base::SequencedTaskRunner> task_runner)
+    : task_runner_(std::move(task_runner)) {}
 
-RefcountedKeyedService::~RefcountedKeyedService() {
-}
+RefcountedKeyedService::~RefcountedKeyedService() = default;

components/keyed_service/core/refcounted_keyed_service.h

@@ -6,8 +6,8 @@
 #define COMPONENTS_KEYED_SERVICE_CORE_REFCOUNTED_KEYED_SERVICE_H_
 
 #include "base/memory/ref_counted.h"
+#include "base/sequenced_task_runner.h"
 #include "base/sequenced_task_runner_helpers.h"
-#include "base/single_thread_task_runner.h"
 #include "components/keyed_service/core/keyed_service_export.h"
 
 class RefcountedKeyedService;
@@ -29,7 +29,7 @@ struct KEYED_SERVICE_EXPORT RefcountedKeyedServiceTraits {
 // be after the corresponding BrowserContext has been destroyed.
 //
 // Optionally, if you initialize your service with the constructor that takes a
-// single thread task runner, your service will be deleted on that thread. We
+// SequencedTaskRunner, your service will be deleted on that sequence. We
 // can't use content::DeleteOnThread<> directly because RefcountedKeyedService
 // must not depend on //content.
 class KEYED_SERVICE_EXPORT RefcountedKeyedService
@@ -44,19 +44,19 @@ class KEYED_SERVICE_EXPORT RefcountedKeyedService
   virtual void ShutdownOnUIThread() = 0;
 
  protected:
-  // If your service does not need to be deleted on a specific thread, use the
+  // If your service does not need to be deleted on a specific sequence, use the
   // default constructor.
   RefcountedKeyedService();
 
-  // If you need your service to be deleted on a specific thread (for example,
+  // If you need your service to be deleted on a specific sequence (for example,
   // you're converting a service that used content::DeleteOnThread<IO>), then
-  // use this constructor with a reference to the SingleThreadTaskRunner (you
+  // use this constructor with a reference to the SequencedTaskRunner (e.g., you
   // can get it from content::BrowserThread::GetTaskRunnerForThread).
   explicit RefcountedKeyedService(
-      const scoped_refptr<base::SingleThreadTaskRunner>& task_runner);
+      scoped_refptr<base::SequencedTaskRunner> task_runner);
 
   // The second pass destruction can happen anywhere unless you specify which
-  // thread this service must be destroyed on by using the second constructor.
+  // sequence this service must be destroyed on by using the second constructor.
   virtual ~RefcountedKeyedService();
 
  private:
@@ -65,8 +65,8 @@ class KEYED_SERVICE_EXPORT RefcountedKeyedService
   friend class base::RefCountedThreadSafe<RefcountedKeyedService,
                                           impl::RefcountedKeyedServiceTraits>;
 
-  // Do we have to delete this object on a specific thread?
-  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+  // Do we have to delete this object on a specific sequence?
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
 };
 
 #endif  // COMPONENTS_KEYED_SERVICE_CORE_REFCOUNTED_KEYED_SERVICE_H_