Clean up Android dangling g_child_thread pointer on shutdown.

content/child/child_thread_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/child/child_thread_impl.h"
 
 #include <signal.h>
 
 #include <string>
 
@@ skipping 143 matching lines @@
   }
 
  protected:
   ~SuicideOnChannelErrorFilter() override {}
 };
 
 #endif  // OS(POSIX)
 
 #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.

     LAZY_INSTANCE_INITIALIZER;
 
 // base::ConditionVariable has an explicit constructor that takes
 // a base::Lock pointer as parameter. The base::DefaultLazyInstanceTraits
 // doesn't handle the case. Thus, we need our own class here.
 struct CondVarLazyInstanceTraits {
   static const bool kRegisterOnExit = true;

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

kRegisterOnExit = false;

should have the same effect as ::Leaky.

(I guess it would be more straightforward to wrap ConditionVariable in another
class than overriding all the traits and new/delete function here.)

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

OK, set to "false".  I stayed with the traits to keep this patch minimal as we
plan to cherry-pick it.

 #ifndef NDEBUG
   static const bool kAllowedToAccessOnNonjoinableThread = false;

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

Hmm, this might be wrong too since the thread we call Shutdown() from (Java UI
thread) is likely not set as joinable and could cause an assertion in debug
builds.

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

Changed to "true".

 #endif
 
   static base::ConditionVariable* New(void* instance) {
     return new (instance) base::ConditionVariable(
         g_lazy_child_thread_lock.Pointer());
   }
   static void Delete(base::ConditionVariable* instance) {
     instance->~ConditionVariable();
   }
 };
@@ skipping 180 matching lines @@
   base::MessageLoop::current()->PostDelayedTask(
       FROM_HERE,
       base::Bind(&ChildThreadImpl::EnsureConnected,
                  channel_connected_factory_.GetWeakPtr()),
       base::TimeDelta::FromSeconds(connection_timeout));
 
 #if defined(OS_ANDROID)
   {
     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.
   g_lazy_child_thread_cv.Get().Signal();
 #endif
 
 #if defined(TCMALLOC_TRACE_MEMORY_SUPPORTED)
   trace_memory_controller_.reset(new base::trace_event::TraceMemoryController(
       message_loop_->message_loop_proxy(), ::HeapProfilerWithPseudoStackStart,
       ::HeapProfilerStop, ::GetHeapProfile));
 #endif
 
   shared_bitmap_manager_.reset(
       new ChildSharedBitmapManager(thread_safe_sender()));
 
   gpu_memory_buffer_manager_.reset(
       new ChildGpuMemoryBufferManager(thread_safe_sender()));
 
   discardable_shared_memory_manager_.reset(
       new ChildDiscardableSharedMemoryManager(thread_safe_sender()));
 }
 
 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
 
   channel_->RemoveFilter(histogram_message_filter_.get());
   channel_->RemoveFilter(sync_message_filter_.get());
 
   // The ChannelProxy object caches a pointer to the IPC thread, so need to
   // reset it as it's not guaranteed to outlive this object.
   // NOTE: this also has the side-effect of not closing the main IPC channel to
@@ skipping 182 matching lines @@
   return g_lazy_tls.Pointer()->Get();
 }
 
 #if defined(OS_ANDROID)
 // The method must NOT be called on the child thread itself.
 // It may block the child thread if so.
 void ChildThreadImpl::ShutdownThread() {
   DCHECK(!ChildThreadImpl::current()) <<
       "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
 
 void ChildThreadImpl::OnProcessFinalRelease() {
   if (on_channel_error_called_) {
     base::MessageLoop::current()->Quit();
     return;
   }
 
   // The child process shutdown sequence is a request response based mechanism,
@@ skipping 12 matching lines @@
 
 void ChildThreadImpl::OnProcessBackgrounded(bool background) {
   // Set timer slack to maximum on main thread when in background.
   base::TimerSlack timer_slack = base::TIMER_SLACK_NONE;
   if (background)
     timer_slack = base::TIMER_SLACK_MAXIMUM;
   base::MessageLoop::current()->SetTimerSlack(timer_slack);
 }
 
 }  // namespace content