Reland "Add thread checking to RunLoop, deprecate MessageLoopRunner. (patchset...

Reland "Add thread checking to RunLoop, deprecate MessageLoopRunner. (patchset #4 id:20002 of https://codereview.chromium.org/2537893002/ )"

Reverted in https://codereview.chromium.org/2548883002/
Reason for relanding: turns out something else was causing these failures.
See details in https://bugs.chromium.org/p/chromium/issues/detail?id=670844#c17

TBR=avi@chromium.org,thakis@chromium.org,asargent@chromium.org
BUG=668707

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

base/run_loop.cc

@@ -25,6 +25,7 @@ RunLoop::~RunLoop() {
 }
 
 void RunLoop::Run() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   if (!BeforeRun())
     return;
 
@@ -44,6 +45,7 @@ void RunLoop::RunUntilIdle() {
 }
 
 void RunLoop::Quit() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   quit_called_ = true;
   if (running_ && loop_->run_loop_ == this) {
     // This is the inner-most RunLoop, so quit now.
@@ -52,6 +54,7 @@ void RunLoop::Quit() {
 }
 
 void RunLoop::QuitWhenIdle() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   quit_when_idle_received_ = true;
 }
 

base/run_loop.h

@@ -10,6 +10,7 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/message_loop/message_loop.h"
+#include "base/threading/thread_checker.h"
 #include "build/build_config.h"
 
 namespace base {
@@ -105,6 +106,8 @@ class BASE_EXPORT RunLoop {
   // that we should quit Run once it becomes idle.
   bool quit_when_idle_received_;
 
+  base::ThreadChecker thread_checker_;
+
   // WeakPtrFactory for QuitClosure safety.
   base::WeakPtrFactory<RunLoop> weak_factory_;
 

chrome/browser/chromeos/file_manager/zip_file_creator_browsertest.cc

@@ -54,11 +54,10 @@ IN_PROC_BROWSER_TEST_F(ZipFileCreatorTest, FailZipForAbsentFile) {
   std::vector<base::FilePath> paths;
   paths.push_back(base::FilePath(FILE_PATH_LITERAL("not.exist")));
   (new ZipFileCreator(
-       base::Bind(
-           &TestCallback, &success, content::GetQuitTaskForRunLoop(&run_loop)),
-       zip_base_dir(),
-       paths,
-       zip_archive_path()))->Start();
+       base::Bind(&TestCallback, &success,
+                  content::GetDeferredQuitTaskForRunLoop(&run_loop)),
+       zip_base_dir(), paths, zip_archive_path()))
+      ->Start();
 
   content::RunThisRunLoop(&run_loop);
   EXPECT_FALSE(success);
@@ -84,11 +83,10 @@ IN_PROC_BROWSER_TEST_F(ZipFileCreatorTest, SomeFilesZip) {
   paths.push_back(kFile1);
   paths.push_back(kFile2);
   (new ZipFileCreator(
-       base::Bind(
-           &TestCallback, &success, content::GetQuitTaskForRunLoop(&run_loop)),
-       zip_base_dir(),
-       paths,
-       zip_archive_path()))->Start();
+       base::Bind(&TestCallback, &success,
+                  content::GetDeferredQuitTaskForRunLoop(&run_loop)),
+       zip_base_dir(), paths, zip_archive_path()))
+      ->Start();
 
   content::RunThisRunLoop(&run_loop);
   EXPECT_TRUE(success);

chrome/browser/sync/test/integration/dictionary_helper.cc

@@ -56,7 +56,8 @@ void LoadDictionary(SpellcheckCustomDictionary* dictionary) {
   if (dictionary->IsLoaded())
     return;
   base::RunLoop run_loop;
-  DictionaryLoadObserver observer(content::GetQuitTaskForRunLoop(&run_loop));
+  DictionaryLoadObserver observer(
+      content::GetDeferredQuitTaskForRunLoop(&run_loop));
   dictionary->AddObserver(&observer);
   dictionary->Load();
   content::RunThisRunLoop(&run_loop);

chrome/browser/sync_file_system/sync_file_system_service_unittest.cc

@@ -73,12 +73,11 @@ void AssignValueAndQuit(base::RunLoop* run_loop,
   run_loop->Quit();
 }
 
-// This is called on IO thread.
-void VerifyFileError(base::RunLoop* run_loop,
+// This is called on IO thread. Posts |callback| to be called on UI thread.
+void VerifyFileError(base::Closure callback,
                      base::File::Error error) {
-  DCHECK(run_loop);
   EXPECT_EQ(base::File::FILE_OK, error);
-  run_loop->Quit();
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, callback);
 }
 
 }  // namespace
@@ -430,7 +429,7 @@ TEST_F(SyncFileSystemServiceTest, SimpleSyncFlowWithFileBusy) {
       base::Bind(&CannedSyncableFileSystem::DoCreateFile,
                  base::Unretained(file_system_.get()),
                  kFile, base::Bind(&VerifyFileError,
-                                   &verify_file_error_run_loop)));
+                                   verify_file_error_run_loop.QuitClosure())));
 
   run_loop.Run();
 

chrome/browser/ui/views/ash/tab_scrubber_browsertest.cc

@@ -213,7 +213,7 @@ class TabScrubberTest : public InProcessBrowserTest,
  private:
   void RunUntilTabActive(Browser* browser, int target) {
     base::RunLoop run_loop;
-    quit_closure_ = content::GetQuitTaskForRunLoop(&run_loop);
+    quit_closure_ = content::GetDeferredQuitTaskForRunLoop(&run_loop);
     browser->tab_strip_model()->AddObserver(this);
     target_index_ = target;
     content::RunThisRunLoop(&run_loop);

chrome/test/base/testing_profile.cc

@@ -651,7 +651,7 @@ void TestingProfile::BlockUntilHistoryIndexIsRefreshed() {
     return;
   base::RunLoop run_loop;
   HistoryIndexRestoreObserver observer(
-      content::GetQuitTaskForRunLoop(&run_loop));
+      content::GetDeferredQuitTaskForRunLoop(&run_loop));
   index->set_restore_cache_observer(&observer);
   run_loop.Run();
   index->set_restore_cache_observer(NULL);

content/browser/loader/async_revalidation_manager_browsertest.cc

@@ -90,8 +90,10 @@ class AsyncRevalidationManagerBrowserTest : public ContentBrowserTest {
 
     // The second time this handler is run is the async revalidation. Tests can
     // use this for synchronisation.
-    if (version == 2)
-      run_loop_.Quit();
+    if (version == 2) {
+      BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+                              run_loop_.QuitClosure());
+    }
     return std::move(http_response);
   }
 

content/public/test/test_launcher.h

@@ -14,7 +14,6 @@
 namespace base {
 class CommandLine;
 class FilePath;
-class RunLoop;
 }
 
 namespace content {
@@ -47,8 +46,6 @@ class TestLauncherDelegate {
   virtual bool AdjustChildProcessCommandLine(
       base::CommandLine* command_line,
       const base::FilePath& temp_data_dir) = 0;
-  virtual void PreRunMessageLoop(base::RunLoop* run_loop) {}
-  virtual void PostRunMessageLoop() {}
   virtual ContentMainDelegate* CreateContentMainDelegate() = 0;
 
   // Called prior to running each test. The delegate may alter the CommandLine

content/public/test/test_utils.cc

@@ -120,23 +120,14 @@ void RunMessageLoop() {
 void RunThisRunLoop(base::RunLoop* run_loop) {
   base::MessageLoop::ScopedNestableTaskAllower allow(
       base::MessageLoop::current());
-
-  // If we're running inside a browser test, we might need to allow the test
-  // launcher to do extra work before/after running a nested message loop.
-  TestLauncherDelegate* delegate = NULL;
-  delegate = GetCurrentTestLauncherDelegate();
-  if (delegate)
-    delegate->PreRunMessageLoop(run_loop);
   run_loop->Run();
-  if (delegate)
-    delegate->PostRunMessageLoop();
 }
 
 void RunAllPendingInMessageLoop() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   base::RunLoop run_loop;
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, GetQuitTaskForRunLoop(&run_loop));
+      FROM_HERE, GetDeferredQuitTaskForRunLoop(&run_loop));
   RunThisRunLoop(&run_loop);
 }
 
@@ -175,7 +166,7 @@ void RunAllBlockingPoolTasksUntilIdle() {
   }
 }
 
-base::Closure GetQuitTaskForRunLoop(base::RunLoop* run_loop) {
+base::Closure GetDeferredQuitTaskForRunLoop(base::RunLoop* run_loop) {
   return base::Bind(&DeferredQuitRunLoop, run_loop->QuitClosure(),
                     kNumQuitDeferrals);
 }
@@ -240,7 +231,7 @@ void MessageLoopRunner::Quit() {
   if (loop_running_) {
     switch (quit_mode_) {
       case QuitMode::DEFERRED:
-        GetQuitTaskForRunLoop(&run_loop_).Run();
+        GetDeferredQuitTaskForRunLoop(&run_loop_).Run();
         break;
       case QuitMode::IMMEDIATE:
         run_loop_.Quit();

content/public/test/test_utils.h

@@ -62,7 +62,7 @@ void RunAllBlockingPoolTasksUntilIdle();
 
 // Get task to quit the given RunLoop. It allows a few generations of pending
 // tasks to run as opposed to run_loop->QuitClosure().
-base::Closure GetQuitTaskForRunLoop(base::RunLoop* run_loop);
+base::Closure GetDeferredQuitTaskForRunLoop(base::RunLoop* run_loop);
 
 // Executes the specified JavaScript in the specified frame, and runs a nested
 // MessageLoop. When the result is available, it is returned.
@@ -91,6 +91,12 @@ bool RegisterJniForTesting(JNIEnv* env);
 // has returned is safe and has no effect.
 // Note that by default Quit does not quit immediately. If that is not what you
 // really need, pass QuitMode::IMMEDIATE in the constructor.
+//
+// DEPRECATED. Consider using base::RunLoop, in most cases MessageLoopRunner is
+// not needed.  If you need to defer quitting the loop, use
+// GetDeferredQuitTaskForRunLoop directly.
+// If you found a case where base::RunLoop is inconvenient or can not be used at
+// all, please post details in a comment on https://crbug.com/668707.
 class MessageLoopRunner : public base::RefCounted<MessageLoopRunner> {
  public:
   enum class QuitMode {

extensions/test/result_catcher.cc

@@ -30,7 +30,7 @@ bool ResultCatcher::GetNextResult() {
   // empty.
   if (results_.empty()) {
     base::RunLoop run_loop;
-    quit_closure_ = content::GetQuitTaskForRunLoop(&run_loop);
+    quit_closure_ = content::GetDeferredQuitTaskForRunLoop(&run_loop);
     content::RunThisRunLoop(&run_loop);
     quit_closure_ = base::Closure();
   }