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..a611417e8c55b42ba53b4f37af54f8aaed100867 100644
--- a/chrome/browser/browser_process_impl.cc
+++ b/chrome/browser/browser_process_impl.cc
@@ -379,12 +379,66 @@ unsigned int BrowserProcessImpl::ReleaseModule() {
   return module_ref_count_;
 }
 
+namespace {
+
+class RundownTaskCounter {

[gab, 2014/06/23 19:53:48]

This could simply be a RefCounted object (especially since atomic_ref_count.h
mentions that its preferable to use RefCounted rather than this low level
implementation). 

Each task would hold a reference to the object (bound to a no-op method).

This class would then simply signal the WaitableEvent in its destructor.

You then don't need to initialize this class with a specific |count|, you
instead hand as many references as you want and don't run the risk of
initializing with an incorrect |count|.

i.e., either all tasks complete and this RefCounted object goes away, signaling
the event, or the timeout on the event fires and the remaining tasks become
irrelevant.

[Sigurður Ásgeirsson, 2014/06/23 20:20:12]

Conflating lifetime management with state is generally a bad idea, so I don't
think it's advisable to do that. Also the count must be set up such that there's
precisely a single transition from one to zero. Posting the object to multiple
threads potentially interleaves the up and down counts, which can lead to
multiple one->zero transitions.

Making the object RefCounted for lifetime management alone is however not crazy.

[gab, 2014/06/23 21:09:53]

Ah right, good point about the possibility of a thread running its task before
other tasks have time to be posted.

[erikwright (departed), 2014/06/24 16:10:53]

The concern about multiple 1->0 transitions is not founded as you would
presumably hold a local scoped_refptr while you are busy posting all of the
tasks.

+ public:
+  // Creates a rundown task counter with |count|.
+  explicit RundownTaskCounter(size_t count);
+  ~RundownTaskCounter() {}
+
+  // Decrements the counter and releases the wait on zero.
+  static void Decrement(RundownTaskCounter* counter);
+
+  // Waits until the count is zero or |max_time| has passed.
+  bool TimedWait(const base::TimeDelta& max_time);
+
+ private:
+  base::AtomicRefCount count_;
+  base::WaitableEvent waitable_event_;
+
+  DISALLOW_COPY_AND_ASSIGN(RundownTaskCounter);
+};
+
+RundownTaskCounter::RundownTaskCounter(size_t count)
+    : count_(count), waitable_event_(true, false) {
+  DCHECK_LT(1U, count);

[gab, 2014/06/23 21:09:53]

DCHECK_GT(count, 1U);

(We always put constants on the RHS of equations in Chromium)

+}
+
+void RundownTaskCounter::Decrement(RundownTaskCounter* counter) {
+  DCHECK_NE(static_cast<RundownTaskCounter*>(NULL), counter);

[gab, 2014/06/23 21:09:53]

DCHECK(counter);

+
+  if (!base::AtomicRefCountDec(&counter->count_)) {
+    counter->waitable_event_.Signal();
+  }
+}
+
+bool RundownTaskCounter::TimedWait(const base::TimeDelta& max_time) {
+  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);
+
+  // We create the rundown task counter with a count for each profile,
+  // plus one for local state.
+  scoped_ptr<RundownTaskCounter> counter(
+      new RundownTaskCounter(profiles.size() + 1));
+
+  for (size_t i = 0; i < profiles.size(); ++i) {
+    Profile* profile = profiles[i];
+    profile->SetExitType(Profile::EXIT_SESSION_ENDED);
+
+    scoped_refptr<base::SequencedTaskRunner> io_task_runner =
+        profile->GetIOTaskRunner();
+    io_task_runner->PostTask(FROM_HERE,
+                             base::Bind(RundownTaskCounter::Decrement,
+                             counter.get()));
+  }
 
   // Tell the metrics service it was cleanly shutdown.
   MetricsService* metrics = g_browser_process->metrics_service();
@@ -395,6 +449,11 @@ void BrowserProcessImpl::EndSession() {
     // On ChromeOS, chrome gets killed when hangs, so no need to
     // commit metrics::prefs::kStabilitySessionEndCompleted change immediately.
     local_state()->CommitPendingWrite();
+
+    local_state_task_runner_->PostTask(FROM_HERE,
+                                       base::Bind(RundownTaskCounter::Decrement,

[gab, 2014/06/23 21:09:53]

Now that RundownTaskCounter is refcounted, Decrement can be non-static (having
it static yet take a pointer to a RundownTaskCounter is essentially the same as
non-static but less readable IMO).

[Sigurður Ásgeirsson, 2014/06/23 21:39:59]

Done.

+                                       counter.get()));

[gab, 2014/06/23 21:09:53]

On ChromeOS this never gets posted (as per the ifdef above) so that the
RundownTaskCounter will never hit 0.

This is why I would have preferred to explicitly hand references/ups to tasks
rather than initialize a global count above, but this doesn't work as-is because
of the possibility of a thread finishing its task before we've posted other
tasks here (as you mentioned above).

One way could perhaps be to post another task in front of this one which would
force each thread hitting it early to spin until we release it here after all
tasks have been posted (and all ups have thus been registered)?
Or even better, without posting another task, the RundownTaskCounter could have
an event which it signals when its ready for threads to start decrementing (and
Decrement would block until that event is signaled). Allowing the
RundownTaskCounter to register all its ups prior to allowing Decrements to start
kicking in.

[Sigurður Ásgeirsson, 2014/06/23 21:39:59]

Thanks great catch. I've added some robustness against mis-counting here by
asserting on the correct count at wait time.
I'm disinclined to block or serialize everything at this critical lifetime point
of Windows applications. Also KISS :).

+

[gab, 2014/06/23 21:09:53]

rm empty line

[Sigurður Ásgeirsson, 2014/06/23 21:39:59]

Done.

 #endif
   }
 
@@ -405,7 +464,7 @@ 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.
+  // Wait on the successful countdown of rundown tasks.
   //
   // 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,15 +475,11 @@ 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(
+  if (!counter.get()->TimedWait(
       base::TimeDelta::FromSeconds(kEndSessionTimeoutSeconds))) {
-    ignore_result(done_writing.release());
+    // If the rundown tasks don't finish in time, we leak the counter, to
+    // avoid use-after-free in the tasks.
+    ignore_result(counter.release());
   }
 #else
   NOTIMPLEMENTED();