Revert of Add thread checking to RunLoop, deprecate MessageLoopRunner…

…. (patchset chromium#4 id:20002 of https://codereview.chromium.org/2537893002/ ) Reason for revert: Speculative revert in an effort to pinpoint the cause of http://crbug.com/670844 - will reland if the flake doesn't clear Original issue's description: > Add thread checker to base::RunLoop. > > The purpose is to make sure that after switching users of > MessageLoopRunner to RunLoop we won't lose any correctness checks. > > Also add a comment that encourages to use RunLoop where possible, and > rename GetQuitTaskForRunLoop to GetDeferredQuitTaskForRunLoop to convey > is purpose clearly. > > BUG=668707 > > Committed: https://crrev.com/dd10720676efff002490f702118e052dcd210f18 > Cr-Commit-Position: refs/heads/master@{#435924} TBR=avi@chromium.org,thakis@chromium.org,asargent@chromium.org,ahest@yandex-team.ru # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=668707 Review-Url: https://codereview.chromium.org/2548883002 Cr-Commit-Position: refs/heads/master@{#436035}
Denger-Network · Dec 2, 2016 · f8675b3 · f8675b3
1 parent f31c5ca
commit f8675b3
Show file tree

Hide file tree

Showing 12 changed files with 37 additions and 37 deletions.
diff --git a/base/run_loop.cc b/base/run_loop.cc
@@ -25,7 +25,6 @@ RunLoop::~RunLoop() {
 }
 
 void RunLoop::Run() {
-  DCHECK(thread_checker_.CalledOnValidThread());
   if (!BeforeRun())
     return;
 
@@ -45,7 +44,6 @@ 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.
@@ -54,7 +52,6 @@ void RunLoop::Quit() {
 }
 
 void RunLoop::QuitWhenIdle() {
-  DCHECK(thread_checker_.CalledOnValidThread());
   quit_when_idle_received_ = true;
 }
 

diff --git a/base/run_loop.h b/base/run_loop.h
@@ -10,7 +10,6 @@
 #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 {
@@ -106,8 +105,6 @@ 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_;
 

diff --git a/chrome/browser/chromeos/file_manager/zip_file_creator_browsertest.cc b/chrome/browser/chromeos/file_manager/zip_file_creator_browsertest.cc
@@ -54,10 +54,11 @@ 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::GetDeferredQuitTaskForRunLoop(&run_loop)),
-       zip_base_dir(), paths, zip_archive_path()))
-      ->Start();
+       base::Bind(
+           &TestCallback, &success, content::GetQuitTaskForRunLoop(&run_loop)),
+       zip_base_dir(),
+       paths,
+       zip_archive_path()))->Start();
 
   content::RunThisRunLoop(&run_loop);
   EXPECT_FALSE(success);
@@ -83,10 +84,11 @@ IN_PROC_BROWSER_TEST_F(ZipFileCreatorTest, SomeFilesZip) {
   paths.push_back(kFile1);
   paths.push_back(kFile2);
   (new ZipFileCreator(
-       base::Bind(&TestCallback, &success,
-                  content::GetDeferredQuitTaskForRunLoop(&run_loop)),
-       zip_base_dir(), paths, zip_archive_path()))
-      ->Start();
+       base::Bind(
+           &TestCallback, &success, content::GetQuitTaskForRunLoop(&run_loop)),
+       zip_base_dir(),
+       paths,
+       zip_archive_path()))->Start();
 
   content::RunThisRunLoop(&run_loop);
   EXPECT_TRUE(success);

diff --git a/chrome/browser/sync/test/integration/dictionary_helper.cc b/chrome/browser/sync/test/integration/dictionary_helper.cc
@@ -56,8 +56,7 @@ void LoadDictionary(SpellcheckCustomDictionary* dictionary) {
   if (dictionary->IsLoaded())
     return;
   base::RunLoop run_loop;
-  DictionaryLoadObserver observer(
-      content::GetDeferredQuitTaskForRunLoop(&run_loop));
+  DictionaryLoadObserver observer(content::GetQuitTaskForRunLoop(&run_loop));
   dictionary->AddObserver(&observer);
   dictionary->Load();
   content::RunThisRunLoop(&run_loop);

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

diff --git a/chrome/browser/ui/views/ash/tab_scrubber_browsertest.cc b/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::GetDeferredQuitTaskForRunLoop(&run_loop);
+    quit_closure_ = content::GetQuitTaskForRunLoop(&run_loop);
     browser->tab_strip_model()->AddObserver(this);
     target_index_ = target;
     content::RunThisRunLoop(&run_loop);

diff --git a/chrome/test/base/testing_profile.cc b/chrome/test/base/testing_profile.cc
@@ -651,7 +651,7 @@ void TestingProfile::BlockUntilHistoryIndexIsRefreshed() {
     return;
   base::RunLoop run_loop;
   HistoryIndexRestoreObserver observer(
-      content::GetDeferredQuitTaskForRunLoop(&run_loop));
+      content::GetQuitTaskForRunLoop(&run_loop));
   index->set_restore_cache_observer(&observer);
   run_loop.Run();
   index->set_restore_cache_observer(NULL);

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

diff --git a/content/public/test/test_launcher.h b/content/public/test/test_launcher.h
@@ -14,6 +14,7 @@
 namespace base {
 class CommandLine;
 class FilePath;
+class RunLoop;
 }
 
 namespace content {
@@ -46,6 +47,8 @@ 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

diff --git a/content/public/test/test_utils.cc b/content/public/test/test_utils.cc
@@ -120,14 +120,23 @@ 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, GetDeferredQuitTaskForRunLoop(&run_loop));
+      FROM_HERE, GetQuitTaskForRunLoop(&run_loop));
   RunThisRunLoop(&run_loop);
 }
 
@@ -166,7 +175,7 @@ void RunAllBlockingPoolTasksUntilIdle() {
   }
 }
 
-base::Closure GetDeferredQuitTaskForRunLoop(base::RunLoop* run_loop) {
+base::Closure GetQuitTaskForRunLoop(base::RunLoop* run_loop) {
   return base::Bind(&DeferredQuitRunLoop, run_loop->QuitClosure(),
                     kNumQuitDeferrals);
 }
@@ -231,7 +240,7 @@ void MessageLoopRunner::Quit() {
   if (loop_running_) {
     switch (quit_mode_) {
       case QuitMode::DEFERRED:
-        GetDeferredQuitTaskForRunLoop(&run_loop_).Run();
+        GetQuitTaskForRunLoop(&run_loop_).Run();
         break;
       case QuitMode::IMMEDIATE:
         run_loop_.Quit();

diff --git a/content/public/test/test_utils.h b/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 GetDeferredQuitTaskForRunLoop(base::RunLoop* run_loop);
+base::Closure GetQuitTaskForRunLoop(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,12 +91,6 @@ 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 {

diff --git a/extensions/test/result_catcher.cc b/extensions/test/result_catcher.cc
@@ -30,7 +30,7 @@ bool ResultCatcher::GetNextResult() {
   // empty.
   if (results_.empty()) {
     base::RunLoop run_loop;
-    quit_closure_ = content::GetDeferredQuitTaskForRunLoop(&run_loop);
+    quit_closure_ = content::GetQuitTaskForRunLoop(&run_loop);
     content::RunThisRunLoop(&run_loop);
     quit_closure_ = base::Closure();
   }