Fix Windows logoff race.

chrome/browser/browser_process_impl.cc

Index: chrome/browser/browser_process_impl.cc
diff --git a/chrome/browser/browser_process_impl.cc b/chrome/browser/browser_process_impl.cc
index 7f492c575c22e6ae13b52c0ff07ba29c145f9eb6..c8d0cf14442bcd85d5aa95433397edf962549691 100644
--- a/chrome/browser/browser_process_impl.cc
+++ b/chrome/browser/browser_process_impl.cc
@@ -8,6 +8,7 @@
 #include <map>
 #include <vector>
 
+#include "base/atomic_ref_count.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
@@ -310,12 +311,6 @@ void BrowserProcessImpl::PostDestroyThreads() {
   io_thread_.reset();
 }
 
-#if defined(USE_X11) || defined(OS_WIN)
-static void Signal(base::WaitableEvent* event) {
-  event->Signal();
-}
-#endif
-
 unsigned int BrowserProcessImpl::AddRefModule() {
   DCHECK(CalledOnValidThread());
 
@@ -379,12 +374,77 @@ unsigned int BrowserProcessImpl::ReleaseModule() {
   return module_ref_count_;
 }
 
+namespace {
+
+class RundownTaskCounter :

[gab, 2014/06/25 14:21:50]

Add a meta-comment explaining the general purpose of this class.

[Sigurður Ásgeirsson, 2014/06/25 15:05:02]

Done.

+    public base::RefCountedThreadSafe<RundownTaskCounter> {
+ public:
+  RundownTaskCounter();
+
+  // Posts a rundown task to |task_runner|, can be invoked an arbitrary number
+  // of times before calling TimedWait.
+  void Post(base::SequencedTaskRunner* task_runner);
+
+  // Waits until the count is zero or |max_time| has passed.
+  // This can only be called once per instance.
+  bool TimedWait(const base::TimeDelta& max_time);
+
+ private:
+  friend class base::RefCountedThreadSafe<RundownTaskCounter>;
+  ~RundownTaskCounter() {}
+
+  // Decrements the counter and releases the waitable
+  // event on transition to zero.

[gab, 2014/06/25 14:21:50]

Some of this second line fits on the previous line.

[Sigurður Ásgeirsson, 2014/06/25 15:05:02]

Done.

+  void Decrement();
+
+  // The count starts at one to defer the possibility of one->zero transitions
+  // until TimedWait is called.
+  base::AtomicRefCount count_;
+  base::WaitableEvent waitable_event_;
+
+  DISALLOW_COPY_AND_ASSIGN(RundownTaskCounter);
+};
+
+RundownTaskCounter::RundownTaskCounter()
+    : count_(1), waitable_event_(true, false) {
+}
+
+void RundownTaskCounter::Post(base::SequencedTaskRunner* task_runner) {
+  // As the count starts off at one, it should never get to zero unless
+  // TimedWait has been called.
+  DCHECK(!base::AtomicRefCountIsZero(&count_));
+
+  base::AtomicRefCountInc(&count_);
+
+  task_runner->PostTask(FROM_HERE,
+      base::Bind(&RundownTaskCounter::Decrement, this));
+}
+
+void RundownTaskCounter::Decrement() {
+  if (!base::AtomicRefCountDec(&count_))
+    waitable_event_.Signal();
+}
+
+bool RundownTaskCounter::TimedWait(const base::TimeDelta& max_time) {
+  // Decrement the excess count from the constructor.
+  Decrement();
+
+  return waitable_event_.TimedWait(max_time);
+}
+
+}  // namespace
+
 void BrowserProcessImpl::EndSession() {
   // Mark all the profiles as clean.
   ProfileManager* pm = profile_manager();
   std::vector<Profile*> profiles(pm->GetLoadedProfiles());
-  for (size_t i = 0; i < profiles.size(); ++i)
-    profiles[i]->SetExitType(Profile::EXIT_SESSION_ENDED);
+  scoped_refptr<RundownTaskCounter> rundown_counter(new RundownTaskCounter());
+  for (size_t i = 0; i < profiles.size(); ++i) {
+    Profile* profile = profiles[i];
+    profile->SetExitType(Profile::EXIT_SESSION_ENDED);
+
+    rundown_counter->Post(profile->GetIOTaskRunner());
+  }
 
   // Tell the metrics service it was cleanly shutdown.
   MetricsService* metrics = g_browser_process->metrics_service();
@@ -395,6 +455,8 @@ void BrowserProcessImpl::EndSession() {
     // On ChromeOS, chrome gets killed when hangs, so no need to
     // commit metrics::prefs::kStabilitySessionEndCompleted change immediately.
     local_state()->CommitPendingWrite();
+
+    rundown_counter->Post(local_state_task_runner_);

[gab, 2014/06/25 14:21:50]

This only happens inside the "if (metrics && local_state())" check; is the
assumption that local_state has already been flushed otherwise?

[Sigurður Ásgeirsson, 2014/06/25 15:05:02]

Apparently things work differently for local state on ChromeOS.

 #endif
   }
 
@@ -405,7 +467,9 @@ void BrowserProcessImpl::EndSession() {
   // otherwise on startup we'll think we crashed. So we block until done and
   // then proceed with normal shutdown.
 #if defined(USE_X11) || defined(OS_WIN)
-  // Create a waitable event to block on file writing being complete.
+  // Do a best-effort wait on the successful countdown of rundown tasks. Note
+  // that if we don't complete "quickly enough", Windows will terminate our

[gab, 2014/06/25 14:21:50]

Can you quantify "quickly enough" based on documentation/experimentation?

kEndSessionTimeoutSeconds is 10 seconds at the moment, is that "quickly enough"?

[Sigurður Ásgeirsson, 2014/06/25 15:05:02]

The timeout is documented something like 5 seconds, but it's not deterministic.
What happens when you overrun is that Windows (as of Vista or Win7) flings up a
dialog complaining that Chrome's tardy, and gives the user the option of blowing
it out of the sky manually. Normally this should complete much faster than 10
seconds, but I don't think the amount of delay is very important - it's only a
safeguard against deadlocking.

+  // process.
   //
   // On Windows, we previously posted a message to FILE and then ran a nested
   // message loop, waiting for that message to be processed until quitting.
@@ -416,16 +480,8 @@ void BrowserProcessImpl::EndSession() {
   // GPU process synchronously. Because the system may not be allowing
   // processes to launch, this can result in a hang. See
   // http://crbug.com/318527.
-  scoped_ptr<base::WaitableEvent> done_writing(
-      new base::WaitableEvent(false, false));
-  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-      base::Bind(Signal, done_writing.get()));
-  // If all file writes haven't cleared in the timeout, leak the WaitableEvent
-  // so that there's no race to reference it in Signal().
-  if (!done_writing->TimedWait(
-      base::TimeDelta::FromSeconds(kEndSessionTimeoutSeconds))) {
-    ignore_result(done_writing.release());
-  }
+  rundown_counter->TimedWait(
+      base::TimeDelta::FromSeconds(kEndSessionTimeoutSeconds));
 #else
   NOTIMPLEMENTED();
 #endif