Disallow QuitCurrent*Deprecated() on RunLoops with Quit*Closure()s.

Use of RunLoop::QuitCurrent*Deprecated() is deprecated, and risky,
potentially causing unexpected early-exits if run under a different
RunLoop to the one intended.

If a caller has taken a Quit*Closure() from the RunLoop then we now
assume that they intend to use it to quit the loop, and therefore use
of QuitCurrent*Deprecated() with it must be unintentional.

Bug: 844016
Change-Id: Ib181d0cb34cdba2af29f557646cbe631101bc6b0
Reviewed-on: https://chromium-review.googlesource.com/1082980
Commit-Queue: Nico Weber <thakis@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571499}

base/files/file_path_watcher_unittest.cc

@@ -188,12 +188,16 @@ class FilePathWatcherTest : public testing::Test {
                   bool recursive_watch) WARN_UNUSED_RESULT;
 
   bool WaitForEvents() WARN_UNUSED_RESULT {
+    return WaitForEventsWithTimeout(TestTimeouts::action_timeout());
+  }
+
+  bool WaitForEventsWithTimeout(TimeDelta timeout) WARN_UNUSED_RESULT {
     RunLoop run_loop;
     collector_->Reset(run_loop.QuitClosure());
 
     // Make sure we timeout if we don't get notified.
     ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-        FROM_HERE, run_loop.QuitClosure(), TestTimeouts::action_timeout());
+        FROM_HERE, run_loop.QuitClosure(), timeout);
     run_loop.Run();
     return collector_->Success();
   }
@@ -853,10 +857,7 @@ TEST_F(FilePathWatcherTest, DirAttributesChanged) {
   // We should not get notified in this case as it hasn't affected our ability
   // to access the file.
   ASSERT_TRUE(ChangeFilePermissions(test_dir1, Read, false));
-  loop_.task_runner()->PostDelayedTask(
-      FROM_HERE, RunLoop::QuitCurrentWhenIdleClosureDeprecated(),
-      TestTimeouts::tiny_timeout());
-  ASSERT_FALSE(WaitForEvents());
+  ASSERT_FALSE(WaitForEventsWithTimeout(TestTimeouts::tiny_timeout()));
   ASSERT_TRUE(ChangeFilePermissions(test_dir1, Read, true));
 
   // We should get notified in this case because filepathwatcher can no

base/message_loop/message_loop_unittest.cc

@@ -1627,6 +1627,8 @@ TEST_P(MessageLoopTypedTest, RunLoopQuitOrderAfter) {
   ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
                                           BindOnce(&FuncThatQuitsNow));
 
+  run_loop.allow_quit_current_deprecated_ = true;
+
   RunLoop outer_run_loop;
   outer_run_loop.Run();
 

base/run_loop.cc

@@ -152,6 +152,7 @@ void RunLoop::QuitWhenIdle() {
 base::Closure RunLoop::QuitClosure() {
   // TODO(gab): Fix bad usage and enable this check, http://crbug.com/715235.
   // DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  allow_quit_current_deprecated_ = false;
 
   // Need to use ProxyToTaskRunner() as WeakPtrs vended from
   // |weak_factory_| may only be accessed on |origin_task_runner_|.
@@ -163,6 +164,7 @@ base::Closure RunLoop::QuitClosure() {
 base::Closure RunLoop::QuitWhenIdleClosure() {
   // TODO(gab): Fix bad usage and enable this check, http://crbug.com/715235.
   // DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  allow_quit_current_deprecated_ = false;
 
   // Need to use ProxyToTaskRunner() as WeakPtrs vended from
   // |weak_factory_| may only be accessed on |origin_task_runner_|.
@@ -201,17 +203,29 @@ void RunLoop::RemoveNestingObserverOnCurrentThread(NestingObserver* observer) {
 // static
 void RunLoop::QuitCurrentDeprecated() {
   DCHECK(IsRunningOnCurrentThread());
-  tls_delegate.Get().Get()->active_run_loops_.top()->Quit();
+  Delegate* delegate = tls_delegate.Get().Get();
+  CHECK(delegate->active_run_loops_.top()->allow_quit_current_deprecated_)
+      << "Please migrate off QuitCurrentDeprecated(), e.g. to QuitClosure().";
+  delegate->active_run_loops_.top()->Quit();
 }
 
 // static
 void RunLoop::QuitCurrentWhenIdleDeprecated() {
   DCHECK(IsRunningOnCurrentThread());
-  tls_delegate.Get().Get()->active_run_loops_.top()->QuitWhenIdle();
+  Delegate* delegate = tls_delegate.Get().Get();
+  CHECK(delegate->active_run_loops_.top()->allow_quit_current_deprecated_)
+      << "Please migrate off QuitCurrentWhenIdleDeprecated(), e.g. to "
+         "QuitWhenIdleClosure().";
+  delegate->active_run_loops_.top()->QuitWhenIdle();
 }
 
 // static
 Closure RunLoop::QuitCurrentWhenIdleClosureDeprecated() {
+  // TODO(https://crbug.com/844016): Migrate call-sites and enable this check.
+  // Delegate* delegate = tls_delegate.Get().Get();
+  // CHECK(delegate->active_run_loops_.top()->allow_quit_current_deprecated_)
+  //     << "Please migrate off QuitCurrentWhenIdleClosureDeprecated(), e.g to "
+  //        "QuitWhenIdleClosure().";
   return Bind(&RunLoop::QuitCurrentWhenIdleDeprecated);
 }
 

base/run_loop.h

@@ -248,6 +248,8 @@ class BASE_EXPORT RunLoop {
   };
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(MessageLoopTypedTest, RunLoopQuitOrderAfter);
+
 #if defined(OS_ANDROID)
   // Android doesn't support the blocking RunLoop::Run, so it calls
   // BeforeRun and AfterRun directly.
@@ -283,6 +285,11 @@ class BASE_EXPORT RunLoop {
   // rather than pushed to Delegate to support nested RunLoops.
   bool quit_when_idle_received_ = false;
 
+  // True if use of QuitCurrent*Deprecated() is allowed. Taking a Quit*Closure()
+  // from a RunLoop implicitly sets this to false, so QuitCurrent*Deprecated()
+  // cannot be used while that RunLoop is being Run().
+  bool allow_quit_current_deprecated_ = true;
+
   // RunLoop is not thread-safe. Its state/methods, unless marked as such, may
   // not be accessed from any other sequence than the thread it was constructed
   // on. Exception: RunLoop can be safely accessed from one other sequence (or

chrome/browser/extensions/api/tab_capture/tab_capture_apitest.cc

@@ -435,14 +435,21 @@ IN_PROC_BROWSER_TEST_F(TabCaptureApiTest, MAYBE_TabIndicator) {
           chrome::GetTabAlertStateForContents(contents);
       if (alert_state != last_alert_state_) {
         last_alert_state_ = alert_state;
-        base::ThreadTaskRunnerHandle::Get()->PostTask(
-            FROM_HERE, base::RunLoop::QuitCurrentWhenIdleClosureDeprecated());
+        if (on_tab_changed_)
+          std::move(on_tab_changed_).Run();
       }
     }
 
+    void WaitForTabChange() {
+      base::RunLoop run_loop;
+      on_tab_changed_ = run_loop.QuitClosure();
+      run_loop.Run();
+    }
+
    private:
     Browser* const browser_;
     TabAlertState last_alert_state_;
+    base::OnceClosure on_tab_changed_;
   };
 
   IndicatorChangeObserver observer(browser());
@@ -462,7 +469,7 @@ IN_PROC_BROWSER_TEST_F(TabCaptureApiTest, MAYBE_TabIndicator) {
       EXPECT_EQ(TabAlertState::TAB_CAPTURING, observer.last_alert_state());
       return;
     }
-    content::RunMessageLoop();
+    observer.WaitForTabChange();
   }
 }
 

chrome/browser/sync_file_system/local/syncable_file_operation_runner_unittest.cc

@@ -137,7 +137,6 @@ class SyncableFileOperationRunnerTest : public testing::Test {
     SCOPED_TRACE(testing::Message() << location.ToString());
     EXPECT_EQ(expect, status);
     ++callback_count_;
-    base::RunLoop::QuitCurrentWhenIdleDeprecated();
   }
 
   bool CreateTempFile(base::FilePath* path) {
@@ -409,7 +408,7 @@ TEST_F(SyncableFileOperationRunnerTest, Cancel) {
           URL(kFile), 10, ExpectStatus(FROM_HERE, File::FILE_OK));
   file_system_.operation_runner()->Cancel(
       id, ExpectStatus(FROM_HERE, File::FILE_ERROR_INVALID_OPERATION));
-  base::RunLoop().Run();
+  content::RunAllTasksUntilIdle();
   EXPECT_EQ(2, callback_count_);
 }
 

chrome/common/mac/mock_launchd.cc

@@ -227,8 +227,8 @@ CFDictionaryRef MockLaunchd::CopyDictionaryByCheckingIn(CFErrorRef* error) {
 
 bool MockLaunchd::RemoveJob(CFStringRef label, CFErrorRef* error) {
   remove_called_ = true;
-  main_task_runner_->PostTask(
-      FROM_HERE, base::RunLoop::QuitCurrentWhenIdleClosureDeprecated());
+  if (quit_closure_)
+    std::move(quit_closure_).Run();
   return true;
 }
 
@@ -237,8 +237,8 @@ bool MockLaunchd::RestartJob(Domain domain,
                              CFStringRef name,
                              CFStringRef session_type) {
   restart_called_ = true;
-  main_task_runner_->PostTask(
-      FROM_HERE, base::RunLoop::QuitCurrentWhenIdleClosureDeprecated());
+  if (quit_closure_)
+    std::move(quit_closure_).Run();
   return true;
 }
 

chrome/common/mac/mock_launchd.h

@@ -10,6 +10,7 @@
 #include <memory>
 #include <string>
 
+#include "base/callback.h"
 #include "base/files/file_path.h"
 #include "base/mac/scoped_cftyperef.h"
 #include "base/memory/scoped_refptr.h"
@@ -35,6 +36,10 @@ class MockLaunchd : public Launchd {
               bool as_service);
   ~MockLaunchd() override;
 
+  void set_quit_closure(base::OnceClosure quit_closure) {
+    quit_closure_ = std::move(quit_closure);
+  }
+
   CFDictionaryRef CopyJobDictionary(CFStringRef label) override;
   CFDictionaryRef CopyDictionaryByCheckingIn(CFErrorRef* error) override;
   bool RemoveJob(CFStringRef label, CFErrorRef* error) override;
@@ -63,6 +68,7 @@ class MockLaunchd : public Launchd {
   base::FilePath file_;
   std::string pipe_name_;
   scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
+  base::OnceClosure quit_closure_;
   std::unique_ptr<MultiProcessLock> running_lock_;
   bool create_socket_;
   bool as_service_;

chrome/common/service_process_util_mac_unittest.mm

@@ -50,6 +50,7 @@ class ServiceProcessStateFileManipulationTest : public ::testing::Test {
                                          &bundle_path_, &executable_path_));
     mock_launchd_.reset(
         new MockLaunchd(executable_path_, loop_.task_runner(), false, false));
+    mock_launchd_->set_quit_closure(run_loop_.QuitClosure());
     scoped_launchd_instance_.reset(
         new Launchd::ScopedInstance(mock_launchd_.get()));
     ASSERT_TRUE(service_process_state_.Initialize());