Clean up Android dangling g_child_thread pointer on shutdown.

content/child/child_thread_impl.cc

Index: content/child/child_thread_impl.cc
diff --git a/content/child/child_thread_impl.cc b/content/child/child_thread_impl.cc
index d5d7a3d2589736b8d60225e710ed193d3fe2565e..bbaff5007e28a84d0396406161750feedc1aad74 100644
--- a/content/child/child_thread_impl.cc
+++ b/content/child/child_thread_impl.cc
@@ -161,6 +161,7 @@ class SuicideOnChannelErrorFilter : public IPC::MessageFilter {
 
 #if defined(OS_ANDROID)
 ChildThreadImpl* g_child_thread = NULL;
+bool g_child_thread_initialized = false;
 
 // A lock protects g_child_thread.
 base::LazyInstance<base::Lock> g_lazy_child_thread_lock =

[no sievers, 2015/02/18 20:48:30]

base::LazyInstance<base::Lock>::Leaky

[aelias_OOO_until_Jul13, 2015/02/19 01:00:28]

Done.

@@ -373,6 +374,7 @@ void ChildThreadImpl::Init(const Options& options) {
   {
     base::AutoLock lock(g_lazy_child_thread_lock.Get());
     g_child_thread = this;
+    g_child_thread_initialized = true;
   }
   // Signalling without locking is fine here because only
   // one thread can wait on the condition variable.
@@ -396,6 +398,13 @@ void ChildThreadImpl::Init(const Options& options) {
 }
 
 ChildThreadImpl::~ChildThreadImpl() {
+#if defined(OS_ANDROID)
+  {
+    base::AutoLock lock(g_lazy_child_thread_lock.Get());
+    g_child_thread = nullptr;
+  }
+#endif
+
 #ifdef IPC_MESSAGE_LOG_ENABLED
   IPC::Logging::GetInstance()->SetIPCSender(NULL);
 #endif
@@ -599,12 +608,17 @@ void ChildThreadImpl::ShutdownThread() {
       "this method should NOT be called from child thread itself";
   {
     base::AutoLock lock(g_lazy_child_thread_lock.Get());

[no sievers, 2015/02/18 19:19:42]

What about the two lazy instances (lock and cv), could they be also gone when we
get here, since they both get destroyed when the AtExit handler goes out of
scope (which I think is when the ContentMainRunner gets deleted)?

[aelias_OOO_until_Jul13, 2015/02/18 19:37:27]

One hint FWIW is that this is a preexisting problem: the fuzzer didn't crash on
the lazy instance, it crashed on the g_child_thread.   Does that prove that this
LazyInstance is OK?  I'm not familiar with the AtExit handler.

[no sievers, 2015/02/18 20:48:30]

I think it's wrong that we access the lazy instances here while we are not on
the thread where they get deleted. We can just make them leaky, see above.

[aelias_OOO_until_Jul13, 2015/02/19 01:00:28]

Done.

-    while (!g_child_thread)
+    while (!g_child_thread_initialized)
       g_lazy_child_thread_cv.Get().Wait();
+
+    // g_child_thread may already have been destructed elsewhere.

[no sievers, 2015/02/18 19:19:42]

nit: "...may already have been destructed while we didn't hold the lock" would
make it more obvious and also explain why we need the extra variable
(g_child_thread_initialized).

+    if (!g_child_thread)
+      return;

[Feng Qian, 2015/02/18 01:47:32]

Seems that only path to get here is that ChildThreadImpl::~ChildThreadImpl
called before ChildThreadImpl::ShutdownThread(), but this shouldn't happen.

[aelias_OOO_until_Jul13, 2015/02/18 02:46:52]

As discussed offline, ~ChildThreadImpl is called from one thread from GpuMain(),
while ShutdownThread is called from Java on the Android UI thread, so it seems
they're racing each other and it can happen.

+
+    DCHECK_NE(base::MessageLoop::current(), g_child_thread->message_loop());
+    g_child_thread->message_loop()->PostTask(
+        FROM_HERE, base::Bind(&QuitMainThreadMessageLoop));
   }
-  DCHECK_NE(base::MessageLoop::current(), g_child_thread->message_loop());
-  g_child_thread->message_loop()->PostTask(
-      FROM_HERE, base::Bind(&QuitMainThreadMessageLoop));
 }
 #endif