Remove RenderThreadImpl::Shutdown

content/child/child_process.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_process.h"
 
 #include <string.h>
 
+#include "base/at_exit.h"
+#include "base/command_line.h"
 #include "base/lazy_instance.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/statistics_recorder.h"
 #include "base/process/process_handle.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_local.h"
 #include "build/build_config.h"
 #include "content/child/child_thread_impl.h"
+#include "content/public/common/content_switches.h"
 
 #if defined(OS_ANDROID)
 #include "base/debug/debugger.h"
 #endif
 
 #if defined(OS_POSIX) && !defined(OS_ANDROID)
 #include <signal.h>
 static void SigUSR1Handler(int signal) { }
 #endif
 
@@ skipping 30 matching lines @@
 
 ChildProcess::~ChildProcess() {
   DCHECK(g_lazy_tls.Pointer()->Get() == this);
 
   // Signal this event before destroying the child process.  That way all
   // background threads can cleanup.
   // For example, in the renderer the RenderThread instances will be able to
   // 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_->IsRenderThread()) {

[jochen (gone - plz use gerrit), 2017/01/11 12:26:25]

why not do this work in RenderThreadImpl::Shutdown()?

[Torne, 2017/01/11 12:50:58]

If there's a good way to do that it seems like a good idea, but I'm not entirely
sure; the other change here is that we leak the thread object instead of
destroying it.

[haraken, 2017/01/11 12:52:25]

Because we need to call main_thread_.release(). It needs to be done only on the
ChildProcess side...?

[jochen (gone - plz use gerrit), 2017/01/11 12:52:59]

Shutdown() could return an enum indicating what to do?

+      if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
+              switches::kSingleProcess)) {
+        // 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.
+        _exit(0);
+      } else {
+        // In a single-process mode, we cannot call _exit(0) here because
+        // it will exit the process before the browser side is ready to exit.
+        // On the other hand, we cannot call main_thread_.reset() because
+        // it is unsafe to destruct main_thread_ without shutting down the
+        // main_thread_ (we no longer have the shutdown sequence). Hence we
+        // leak main_thread_.
+        //
+        // Then we need to disable at-exit callbacks because some of the
+        // at-exit callbacks are expected to run after main_thread_ has been
+        // destructed.
+        base::AtExitManager::DisableAllAtExitManagers();

[jochen (gone - plz use gerrit), 2017/01/11 12:26:25]

won't that disable at exit managers for stuff that doesn't belong to this thread
as well (since we're running in single process mode)?

[Torne, 2017/01/11 12:50:58]

Yes. The assumption is that as the only shipping configuration for single
process is webview on android (where shutdown of the browser process never
happens at all as it only ever exits by SIGKILL from the OS), that this doesn't
matter, as long as all the tests that run in single process mode still pass
(which they appear to).

[haraken, 2017/01/11 12:52:25]

You're right. Ideally we should stop calling at-exit managers only on this
thread but currently there's no easy way to do that.

Given the discussion on chromium-dev, it seems like a single-process mode is
used only in:

- a number of browser tests: we want to rewrite them in multi-process mode.
- android webview: it never shuts down; i.e., it doesn't hit this call path.

So I guess it would be okay to not worry about the at-exit manager problem too
seriously here (as long as the existing single-process browser tests pass).

+        main_thread_.release();
+      }
+    } else {
+      main_thread_->Shutdown();
+      main_thread_.reset();
+    }
   }
 
   g_lazy_tls.Pointer()->Set(NULL);
   io_thread_.Stop();
 }
 
 ChildThreadImpl* ChildProcess::main_thread() {
   return main_thread_.get();
 }
 
@@ skipping 67 matching lines @@
   memset(&sa, 0, sizeof(sa));
   sa.sa_handler = SigUSR1Handler;
   sigaction(SIGUSR1, &sa, NULL);
 
   pause();
 #endif  // defined(OS_ANDROID)
 #endif  // defined(OS_POSIX)
 }
 
 }  // namespace content