Remove RenderThreadImpl::Shutdown

RenderThreadImpl::Shutdown has been shutting down Blink and V8 gracefully,
but the graceful shutdown has caused tons of use-after-free bugs
(and many engineers has spent lots of time fixing ordering issues around the shutdown).

As discussed in chromium-dev@ (https://groups.google.com/a/chromium.org/d/topic/chromium-dev/OLS4JSZvowI/discussion) and
platform-architecture-dev@ (https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/Zc12k91NTFk/discussion),
there is no reason we have to shut down the renderer gracefully.
It's just causing use-after-free bugs and wasting performance.

Hence this CL removes RenderThreadImpl::Shutdown and instead calls _exit(0) in ~ChildProcess.
We need a couple of special handling in a single-process mode. See the comment in ~ChildProcess
for more details.

BUG=639244

Review-Url: https://codereview.chromium.org/2309153002
Cr-Commit-Position: refs/heads/master@{#443175}

base/at_exit.cc

@@ -22,6 +22,8 @@ namespace base {
 // this for thread-safe access, since it will only be modified in testing.
 static AtExitManager* g_top_manager = NULL;
 
+static bool g_disable_managers = false;
+
 AtExitManager::AtExitManager()
     : processing_callbacks_(false), next_manager_(g_top_manager) {
 // If multiple modules instantiate AtExitManagers they'll end up living in this
@@ -39,7 +41,8 @@ AtExitManager::~AtExitManager() {
   }
   DCHECK_EQ(this, g_top_manager);
 
-  ProcessCallbacksNow();
+  if (!g_disable_managers)
+    ProcessCallbacksNow();
   g_top_manager = next_manager_;
 }
 
@@ -88,6 +91,11 @@ void AtExitManager::ProcessCallbacksNow() {
   DCHECK(g_top_manager->stack_.empty());
 }
 
+void AtExitManager::DisableAllAtExitManagers() {
+  AutoLock lock(g_top_manager->lock_);
+  g_disable_managers = true;
+}
+
 AtExitManager::AtExitManager(bool shadow)
     : processing_callbacks_(false), next_manager_(g_top_manager) {
   DCHECK(shadow || !g_top_manager);

base/at_exit.h

@@ -49,6 +49,10 @@ class BASE_EXPORT AtExitManager {
   // is possible to register new callbacks after calling this function.
   static void ProcessCallbacksNow();
 
+  // Disable all registered at-exit callbacks. This is used only in a single-
+  // process mode.
+  static void DisableAllAtExitManagers();
+
  protected:
   // This constructor will allow this instance of AtExitManager to be created
   // even if one already exists.  This should only be used for testing!

content/child/child_process.cc

@@ -67,11 +67,15 @@ ChildProcess::~ChildProcess() {
   // notice shutdown before the render process begins waiting for them to exit.
   shutdown_event_.Signal();
 
-  // Kill the main thread object before nulling child_process, since
-  // destruction code might depend on it.
   if (main_thread_) {  // null in unittests.
     main_thread_->Shutdown();
-    main_thread_.reset();
+    if (main_thread_->ShouldBeDestroyed()) {
+      main_thread_.reset();
+    } else {
+      // Leak the main_thread_. See a comment in
+      // RenderThreadImpl::ShouldBeDestroyed.
+      main_thread_.release();
+    }
   }
 
   g_lazy_tls.Pointer()->Set(NULL);

content/child/child_thread_impl.cc

@@ -598,6 +598,10 @@ void ChildThreadImpl::Shutdown() {
   WebFileSystemImpl::DeleteThreadSpecificInstance();
 }
 
+bool ChildThreadImpl::ShouldBeDestroyed() {
+  return true;
+}
+
 void ChildThreadImpl::OnChannelConnected(int32_t peer_pid) {
   channel_connected_factory_.reset();
 }

content/child/child_thread_impl.h

@@ -82,6 +82,8 @@ class CONTENT_EXPORT ChildThreadImpl
   // should be joined in Shutdown().
   ~ChildThreadImpl() override;
   virtual void Shutdown();
+  // Returns true if the thread should be destroyed.
+  virtual bool ShouldBeDestroyed();
 
   // IPC::Sender implementation:
   bool Send(IPC::Message* msg) override;

content/renderer/render_thread_impl.cc

@@ -11,6 +11,7 @@
 #include <vector>
 
 #include "base/allocator/allocator_extension.h"
+#include "base/at_exit.h"
 #include "base/command_line.h"
 #include "base/debug/crash_logging.h"
 #include "base/lazy_instance.h"
@@ -937,132 +938,31 @@ RenderThreadImpl::~RenderThreadImpl() {
 }
 
 void RenderThreadImpl::Shutdown() {
-  for (auto& observer : observers_)
-    observer.OnRenderProcessShutdown();
-
-  if (memory_observer_) {
-    message_loop()->RemoveTaskObserver(memory_observer_.get());
-    memory_observer_.reset();
-  }
-
-  // Wait for all databases to be closed.
-  if (blink_platform_impl_) {
-    // Crash the process if they fail to close after a generous amount of time.
-    bool all_closed = blink_platform_impl_->web_database_observer_impl()
-        ->WaitForAllDatabasesToClose(base::TimeDelta::FromSeconds(60));
-    CHECK(all_closed);
-  }
-
-  // Shutdown in reverse of the initialization order.
-  if (devtools_agent_message_filter_.get()) {
-    RemoveFilter(devtools_agent_message_filter_.get());
-    devtools_agent_message_filter_ = nullptr;
-  }
-
-  RemoveFilter(audio_input_message_filter_.get());
-  audio_input_message_filter_ = nullptr;
-
-#if BUILDFLAG(ENABLE_WEBRTC)
-  RTCPeerConnectionHandler::DestructAllHandlers();
-  // |peer_connection_factory_| cannot be deleted until after the main message
-  // loop has been destroyed.  This is because there may be pending tasks that
-  // hold on to objects produced by the PC factory that depend on threads owned
-  // by the PC factory.  Once those tasks have been freed, the factory can be
-  // deleted.
-#endif
-  vc_manager_.reset();
-
-  RemoveFilter(db_message_filter_.get());
-  db_message_filter_ = nullptr;
-
-  // Shutdown the file thread if it's running.
-  if (file_thread_)
-    file_thread_->Stop();
-
-  if (compositor_message_filter_.get()) {
-    RemoveFilter(compositor_message_filter_.get());
-    compositor_message_filter_ = nullptr;
-  }
-
-#if defined(OS_ANDROID)
-  if (sync_compositor_message_filter_) {
-    RemoveFilter(sync_compositor_message_filter_.get());
-    sync_compositor_message_filter_ = nullptr;
-  }
-  stream_texture_factory_ = nullptr;
-#endif
-
-  media_thread_.reset();
-
-  blink_platform_impl_->SetCompositorThread(nullptr);
-
-  compositor_thread_.reset();
-
-  // AudioMessageFilter may be accessed on |media_thread_|, so shutdown after.
-  RemoveFilter(audio_message_filter_.get());
-  audio_message_filter_ = nullptr;
-
-  categorized_worker_pool_->Shutdown();
-
-  main_input_callback_.Cancel();
-  input_handler_manager_.reset();
-  if (input_event_filter_.get()) {
-    RemoveFilter(input_event_filter_.get());
-    input_event_filter_ = nullptr;
-  }
-
-  // RemoveEmbeddedWorkerRoute may be called while deleting
-  // EmbeddedWorkerDispatcher. So it must be deleted before deleting
-  // RenderThreadImpl.
-  embedded_worker_dispatcher_.reset();
-
-  // Ramp down IDB before we ramp down WebKit (and V8), since IDB classes might
-  // hold pointers to V8 objects (e.g., via pending requests).
-  main_thread_indexed_db_dispatcher_.reset();
-
-  main_thread_compositor_task_runner_ = nullptr;
-
-  gpu_factories_.clear();
-
-  // Context providers must be released prior to destroying the GPU channel.
-  shared_worker_context_provider_ = nullptr;
-  shared_main_thread_contexts_ = nullptr;
-
-  if (gpu_channel_.get())
-    gpu_channel_->DestroyChannel();
-
-  ChildThreadImpl::Shutdown();
-
-  // Shut down the message loop (if provided when the RenderThreadImpl was
-  // constructed) and the renderer scheduler before shutting down Blink. This
-  // prevents a scenario where a pending task in the message loop accesses Blink
-  // objects after Blink shuts down.
-  renderer_scheduler_->SetRAILModeObserver(nullptr);
-  renderer_scheduler_->Shutdown();
-  if (main_message_loop_)
-    base::RunLoop().RunUntilIdle();
-
-  if (blink_platform_impl_) {
-    blink_platform_impl_->Shutdown();
-    // This must be at the very end of the shutdown sequence.
-    // blink::shutdown() must be called after all strong references from
-    // Chromium to Blink are cleared.
-    blink::shutdown();
-  }
-
-  // Delay shutting down DiscardableSharedMemoryManager until blink::shutdown
-  // is complete, because blink::shutdown destructs Blink Resources and they
-  // may try to unlock their underlying discardable memory.
-  discardable_shared_memory_manager_.reset();
-
-  // The message loop must be cleared after shutting down
-  // the DiscardableSharedMemoryManager, which needs to send messages
-  // to the browser process.
-  main_message_loop_.reset();
-
-  lazy_tls.Pointer()->Set(nullptr);
-
-  base::MemoryCoordinatorClientRegistry::GetInstance()->Unregister(this);
+  // In a multi-process mode, we immediately exit the renderer.
+  // Historically we had a graceful shutdown sequence here but it was
+  // 1) a waste of performance and 2) a source of lots of complicated
+  // crashes caused by shutdown ordering. Immediate exit eliminates
+  // those problems.
+  //
+  // In a single-process mode, we cannot call _exit(0) in Shutdown() because
+  // it will exit the process before the browser side is ready to exit.
+  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kSingleProcess))
+    _exit(0);
+}
+
+bool RenderThreadImpl::ShouldBeDestroyed() {
+  DCHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kSingleProcess));
+  // In a single-process mode, it is unsafe to destruct this renderer thread
+  // because we haven't run the shutdown sequence. Hence we leak the render
+  // thread.
+  //
+  // In this case, we also need to disable at-exit callbacks because some of
+  // the at-exit callbacks are expected to run after the renderer thread
+  // has been destructed.
+  base::AtExitManager::DisableAllAtExitManagers();
+  return false;
 }
 
 bool RenderThreadImpl::Send(IPC::Message* msg) {

content/renderer/render_thread_impl.h

@@ -175,6 +175,7 @@ class CONTENT_EXPORT RenderThreadImpl
 
   ~RenderThreadImpl() override;
   void Shutdown() override;
+  bool ShouldBeDestroyed() override;
 
   // When initializing WebKit, ensure that any schemes needed for the content
   // module are registered properly.  Static to allow sharing with tests.

content/renderer/render_thread_impl_browsertest.cc

@@ -10,6 +10,7 @@
 
 #include "base/callback.h"
 #include "base/command_line.h"
+#include "base/debug/leak_annotations.h"
 #include "base/location.h"
 #include "base/memory/discardable_memory.h"
 #include "base/run_loop.h"
@@ -33,6 +34,7 @@
 #include "content/public/test/content_browser_test_utils.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/public/test/test_content_client_initializer.h"
+#include "content/public/test/test_launcher.h"
 #include "content/public/test/test_service_manager_context.h"
 #include "content/renderer/render_process_impl.h"
 #include "content/test/mock_render_process.h"
@@ -221,6 +223,17 @@ class RenderThreadImplBrowserTest : public testing::Test {
     thread_->AddFilter(test_msg_filter_.get());
   }
 
+  void TearDown() override {
+    if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+            kSingleProcessTestsFlag)) {
+      // In a single-process mode, we need to avoid destructing mock_process_
+      // because it will call _exit(0) and kill the process before the browser
+      // side is ready to exit.
+      ANNOTATE_LEAKING_OBJECT_PTR(mock_process_.get());
+      mock_process_.release();
+    }
+  }
+
   IPC::Sender* sender() { return channel_.get(); }
 
   scoped_refptr<TestTaskCounter> test_task_counter_;

third_party/WebKit/Source/wtf/ThreadSpecific.h

@@ -224,7 +224,9 @@ inline void ThreadSpecific<T>::set(T* ptr) {
 
 template <typename T>
 inline void ThreadSpecific<T>::destroy(void* ptr) {
-  if (isShutdown())
+  // Never call destructors on the main thread.
+  // This is fine because Blink no longer has a graceful shutdown sequence.
+  if (isMainThread())
     return;
 
   Data* data = static_cast<Data*>(ptr);