[Chromecast] Make SIGTERM/SIGINT handler safer

chromecast/browser/cast_browser_main_parts.cc

Index: chromecast/browser/cast_browser_main_parts.cc
diff --git a/chromecast/browser/cast_browser_main_parts.cc b/chromecast/browser/cast_browser_main_parts.cc
index 14f23316fdbcb325950edcc800116b23e304fcae..2c5a8aae18204da1db6d92b016f2e311b9ca7ffc 100644
--- a/chromecast/browser/cast_browser_main_parts.cc
+++ b/chromecast/browser/cast_browser_main_parts.cc
@@ -76,24 +76,27 @@ namespace {
 
 #if !defined(OS_ANDROID)
 int kSignalsToRunClosure[] = { SIGTERM, SIGINT, };
-
 // Closure to run on SIGTERM and SIGINT.
 base::Closure* g_signal_closure = NULL;
+base::PlatformThreadId g_main_thread_id;
+int g_quit_signal = 0;
 
 void RunClosureOnSignal(int signum) {
-  LOG(ERROR) << "Got signal " << signum;

[maclellant, 2015/12/28 19:30:32]

This printing is still useful to detect the cases where a signal comes in, but
the main message loop locks up and never exits (this has happened before). I
would print out simple raw log.

RAW_LOG(INFO, "Received close signal");

[kmackay, 2015/12/29 18:23:01]

Done.

-  DCHECK(g_signal_closure);
-  // Expect main thread got this signal. Otherwise, weak_ptr of run_loop will
-  // crash the process.
+  if (base::PlatformThread::CurrentId() != g_main_thread_id)
+    return;
+  if (!g_signal_closure)
+    return;
+  g_quit_signal = signum;
   g_signal_closure->Run();
 }
 
-void RegisterClosureOnSignal(const base::Closure& closure) {
+void RegisterClosureOnSignal(base::Closure* closure) {
+  DCHECK(closure);
   DCHECK(!g_signal_closure);
   DCHECK_GT(arraysize(kSignalsToRunClosure), 0U);
 
-  // Allow memory leak by intention.
-  g_signal_closure = new base::Closure(closure);
+  g_signal_closure = closure;
+  g_main_thread_id = base::PlatformThread::CurrentId();
 
   struct sigaction sa_new;
   memset(&sa_new, 0, sizeof(sa_new));
@@ -101,9 +104,9 @@ void RegisterClosureOnSignal(const base::Closure& closure) {
   sigfillset(&sa_new.sa_mask);
   sa_new.sa_flags = SA_RESTART;
 
-  for (size_t i = 0; i < arraysize(kSignalsToRunClosure); i++) {
+  for (int sig : kSignalsToRunClosure) {
     struct sigaction sa_old;
-    if (sigaction(kSignalsToRunClosure[i], &sa_new, &sa_old) == -1) {
+    if (sigaction(sig, &sa_new, &sa_old) == -1) {
       NOTREACHED();
     } else {
       DCHECK_EQ(sa_old.sa_handler, SIG_DFL);

[maclellant, 2015/12/28 19:30:32]

Since old handler is never unregistered this will crash if CastBrowserMainParts
stops the main loop and then tries to start it again. Does that ever happen?

[kmackay, 2015/12/29 18:23:01]

I don't think so. Note that this check is not new code.

@@ -114,6 +117,12 @@ void RegisterClosureOnSignal(const base::Closure& closure) {
   prctl(PR_SET_PDEATHSIG, kSignalsToRunClosure[0]);
 }
 
+void DeregisterClosureOnSignal(base::Closure* closure) {
+  DCHECK_EQ(g_signal_closure, closure);
+  DCHECK_EQ(g_main_thread_id, base::PlatformThread::CurrentId());
+  g_signal_closure = nullptr;

[maclellant, 2015/12/28 19:30:32]

Technically this is unsafe since the signal handler for SIGTERM and SIGINT are
still registered, so while setting it to NULL a signal may come in and the new
global value is not guaranteed to be valid inside the signal handler (only
sig_atomic_t and C++11 atomic types allow that).

But realistically I don't think this should be a problem since it's guarded on
the main thread and Closure remains alive for some time.

[kmackay, 2015/12/29 18:23:01]

Right, I think the previous code with the "memory leak" is actually better.

+}
+
 const int kKillOnAlarmTimeoutSec = 5;  // 5 seconds
 
 void KillOnAlarm(int signum) {
@@ -392,7 +401,7 @@ bool CastBrowserMainParts::MainMessageLoopRun(int* result_code) {
 #else
   base::RunLoop run_loop;
   base::Closure quit_closure(run_loop.QuitClosure());
-  RegisterClosureOnSignal(quit_closure);
+  RegisterClosureOnSignal(&quit_closure);
 
   // If parameters_.ui_task is not NULL, we are running browser tests.
   if (parameters_.ui_task) {
@@ -402,6 +411,9 @@ bool CastBrowserMainParts::MainMessageLoopRun(int* result_code) {
   }
 
   run_loop.Run();
+  DeregisterClosureOnSignal(&quit_closure);
+  if (g_quit_signal)
+    LOG(INFO) << "Quitting due to signal " << g_quit_signal;
 
   // Once the main loop has stopped running, we give the browser process a few
   // seconds to stop cast service and finalize all resources. If a hang occurs