Pending tasks in a message loop should be deleted before shutting down Blink

Currently Blink is shut down before all the pending tasks in the message loop are deleted. This is problematic in Oilpan because a destructor of the pending tasks can touch Oilpan objects. Because Oilpan is already detached from the renderer thread at that point, touching Oilpan objects in the destructor leads to a crash. (See the bug report for a concrete scenario.)

To prevent Blink objects from getting accessed after Blink is shut down, this CL deletes all pending tasks in a message loop before shutting down Blink.

BUG=411026
TEST=None. I cannot reproduce the crash.

Committed: https://crrev.com/fdd5612c20f777e1279efd7c1e99d82ed04afaaf
Cr-Commit-Position: refs/heads/master@{#296697}

Review URL: https://codereview.chromium.org/583043005

Cr-Commit-Position: refs/heads/master@{#297338}

content/renderer/render_thread_impl.cc

@@ -401,6 +401,13 @@ RenderThreadImpl::RenderThreadImpl(const std::string& channel_name)
   Init();
 }
 
+RenderThreadImpl::RenderThreadImpl(
+    scoped_ptr<base::MessageLoop> main_message_loop)
+    : ChildThread(Options(ShouldUseMojoChannel())),
+      main_message_loop_(main_message_loop.Pass()) {
+  Init();
+}
+
 void RenderThreadImpl::Init() {
   TRACE_EVENT_BEGIN_ETW("RenderThreadImpl::Init", 0, "");
 
@@ -657,16 +664,24 @@ void RenderThreadImpl::Shutdown() {
 
   main_thread_compositor_task_runner_ = NULL;
 
-  if (webkit_platform_support_)
-    blink::shutdown();
-
-  lazy_tls.Pointer()->Set(NULL);
+  gpu_channel_ = NULL;
 
   // TODO(port)
 #if defined(OS_WIN)
   // Clean up plugin channels before this thread goes away.
   NPChannelBase::CleanupChannels();
 #endif
+
+  // Shut down the message loop before shutting down Blink.
+  // This prevents a scenario where a pending task in the message loop accesses
+  // Blink objects after Blink shuts down.
+  // This must be at the very end of the shutdown sequence. You must not touch
+  // the message loop after this.
+  main_message_loop_.reset();
+  if (webkit_platform_support_)
+    blink::shutdown();
+
+  lazy_tls.Pointer()->Set(NULL);
 }
 
 bool RenderThreadImpl::Send(IPC::Message* msg) {

content/renderer/render_thread_impl.h

@@ -120,6 +120,8 @@ class CONTENT_EXPORT RenderThreadImpl : public RenderThread,
   RenderThreadImpl();
   // Constructor that's used when running in single process mode.
   explicit RenderThreadImpl(const std::string& channel_name);
+  // Constructor that's used in RendererMain.
+  explicit RenderThreadImpl(scoped_ptr<base::MessageLoop> main_message_loop);
   virtual ~RenderThreadImpl();
   virtual void Shutdown() OVERRIDE;
 
@@ -523,6 +525,11 @@ class CONTENT_EXPORT RenderThreadImpl : public RenderThread,
   // GpuChannelHostFactory methods.
   scoped_refptr<base::MessageLoopProxy> io_message_loop_proxy_;
 
+  // The message loop of the renderer main thread.
+  // This message loop should be destructed before the RenderThreadImpl
+  // shuts down Blink.
+  scoped_ptr<base::MessageLoop> main_message_loop_;
+
   // A lazily initiated thread on which file operations are run.
   scoped_ptr<base::Thread> file_thread_;
 

content/renderer/renderer_main.cc

@@ -149,12 +149,13 @@ int RendererMain(const MainFunctionParams& parameters) {
   // needs to be backed by a Foundation-level loop to process NSTimers. See
   // http://crbug.com/306348#c24 for details.
   scoped_ptr<base::MessagePump> pump(new base::MessagePumpNSRunLoop());
-  base::MessageLoop main_message_loop(pump.Pass());
+  scoped_ptr<base::MessageLoop> main_message_loop(
+      new base::MessageLoop(pump.Pass()));
 #else
   // The main message loop of the renderer services doesn't have IO or UI tasks.
-  base::MessageLoop main_message_loop;
+  scoped_ptr<base::MessageLoop> main_message_loop(new base::MessageLoop());
 #endif
-  main_message_loop.AddTaskObserver(&task_observer);
+  main_message_loop->AddTaskObserver(&task_observer);
 
   base::PlatformThread::SetName("CrRendererMain");
 
@@ -198,7 +199,7 @@ int RendererMain(const MainFunctionParams& parameters) {
     // TODO(markus): Check if it is OK to unconditionally move this
     // instruction down.
     RenderProcessImpl render_process;
-    new RenderThreadImpl();
+    new RenderThreadImpl(main_message_loop.Pass());
 #endif
     bool run_loop = true;
     if (!no_sandbox) {
@@ -214,7 +215,7 @@ int RendererMain(const MainFunctionParams& parameters) {
     }
 #if defined(OS_POSIX) && !defined(OS_MACOSX)
     RenderProcessImpl render_process;
-    new RenderThreadImpl();
+    new RenderThreadImpl(main_message_loop.Pass());
 #endif
 
     base::HighResolutionTimerManager hi_res_timer_manager;