Adding a SIGTERM handler for OS_POSIX builds.

Adding a SIGTERM handler for OS_POSIX builds.  This is needed so that Chrome can shut down gracefully when many posix-based system halt or reboot while Chrome is open.

SIGTERM may come in on any thread, so the handler creates a Task object that wraps up a call to BrowserList::CloseAllBrowsers(true) and Posts it to the message loop of the UI thread.  Thus, we both get out of the signal handler quickly and can deal with the signal on any thread.

BUG=23551
TEST=covered by BrowserTest.PosixSessionEnd

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=28225

[agl, 2009-10-01 22:20:58]

http://codereview.chromium.org/255036/diff/6002/5006
File base/process_util_posix.cc (right):

http://codereview.chromium.org/255036/diff/6002/5006#newcode108
Line 108: DCHECK(process_id > 1);
DCHECK_GT

http://codereview.chromium.org/255036/diff/6002/5003
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/255036/diff/6002/5003#newcode163
Line 163: CHECK(signal == SIGTERM);
DCHECK_EQ

http://codereview.chromium.org/255036/diff/6002/5003#newcode164
Line 164: if(ChromeThread::CurrentlyOn(ChromeThread::UI)) {
space after if

http://codereview.chromium.org/255036/diff/6002/5003#newcode165
Line 165: LOG(WARNING) << "Handling a SIGTERM for " << getpid();
What happens if the signal is handled by another thread?

http://codereview.chromium.org/255036/diff/6002/5004
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/255036/diff/6002/5004#newcode137
Line 137: LOG(WARNING) << "Spawning browser for " << ui_test_name_;
Did you want to include these in the checkin?

http://codereview.chromium.org/255036/diff/6002/5004#newcode143
Line 143: LOG(WARNING) << "Done tearing down browser";
(... and this)

[Chris Masone, 2009-10-01 22:32:16]

http://codereview.chromium.org/255036/diff/6002/5006
File base/process_util_posix.cc (right):

http://codereview.chromium.org/255036/diff/6002/5006#newcode108
Line 108: DCHECK(process_id > 1);
On 2009/10/01 22:20:58, agl wrote:
> DCHECK_GT

Done.

http://codereview.chromium.org/255036/diff/6002/5003
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/255036/diff/6002/5003#newcode163
Line 163: CHECK(signal == SIGTERM);
On 2009/10/01 22:20:58, agl wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/255036/diff/6002/5003#newcode164
Line 164: if(ChromeThread::CurrentlyOn(ChromeThread::UI)) {
On 2009/10/01 22:20:58, agl wrote:
> space after if

Done.

http://codereview.chromium.org/255036/diff/6002/5003#newcode165
Line 165: LOG(WARNING) << "Handling a SIGTERM for " << getpid();
On 2009/10/01 22:20:58, agl wrote:
> What happens if the signal is handled by another thread?

My understanding of signals + posix threads is that, unless I explicitly mask
out a signal for each thread, they're all going to get the signal in question. 
So, this should make other threads ignore SIGTERM, but allow the UI thread to
handle it.

http://codereview.chromium.org/255036/diff/6002/5004
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/255036/diff/6002/5004#newcode137
Line 137: LOG(WARNING) << "Spawning browser for " << ui_test_name_;
On 2009/10/01 22:20:58, agl wrote:
> Did you want to include these in the checkin?

nope.  You are correct, sir.

http://codereview.chromium.org/255036/diff/6002/5004#newcode143
Line 143: LOG(WARNING) << "Done tearing down browser";
On 2009/10/01 22:20:58, agl wrote:
> (... and this)

Done.

[Paweł Hajdan Jr., 2009-10-01 22:49:51]

+tony (I think I had earlier discussion with you about SIGKILL and you didn't
like it - please take a look at this CL)

I'm a little concerned that you had to use SIGKILL in test infrastructure to
make Chrome quit properly. Shouldn't it quit with SIGTERM even with the handler?

Also, is there some way to not introduce code duplication in browser_uitest.cc
(parts of code seem to be duplicated from ui_test.cc)?

[agl, 2009-10-01 22:51:01]

http://codereview.chromium.org/255036/diff/6002/5003
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/255036/diff/6002/5003#newcode165
Line 165: LOG(WARNING) << "Handling a SIGTERM for " << getpid();
On 2009/10/01 22:32:17, cmasone wrote:
> My understanding of signals + posix threads is that, unless I explicitly mask
> out a signal for each thread, they're all going to get the signal in question.

> So, this should make other threads ignore SIGTERM, but allow the UI thread to
> handle it.

Afraid not:

"A process-directed signal may be delivered to any one of the threads that does
not currently have the signal blocked.  If more  than  one  of  the threads has
the signal unblocked, then the kernel chooses an arbitrary thread to which to
deliver the signal."

[Chris Masone, 2009-10-01 23:08:31]

On Thu, Oct 1, 2009 at 3:51 PM, <agl@chromium.org> wrote:

>
> http://codereview.chromium.org/255036/diff/6002/5003
> File chrome/browser/browser_main.cc (right):
>
> http://codereview.chromium.org/255036/diff/6002/5003#newcode165
> Line 165: LOG(WARNING) << "Handling a SIGTERM for " << getpid();
> On 2009/10/01 22:32:17, cmasone wrote:
>
>> My understanding of signals + posix threads is that, unless I
>>
> explicitly mask
>
>> out a signal for each thread, they're all going to get the signal in
>>
> question.
>
>> So, this should make other threads ignore SIGTERM, but allow the UI
>>
> thread to
>
>> handle it.
>>
>
> Afraid not:
>
> "A process-directed signal may be delivered to any one of the threads
> that does not currently have the signal blocked.  If more  than  one  of
>  the threads has the signal unblocked, then the kernel chooses an
> arbitrary thread to which to deliver the signal."
>
>
> http://codereview.chromium.org/255036
>

Well, then I'm a twit :-)

Let me address this, and see if doing so will make it so that SIGTERM is
once again enough to kill processes during ui_tests

[tony, 2009-10-01 23:14:53]

On 2009/10/01 22:49:51, Paweł Hajdan Jr. wrote:
> +tony (I think I had earlier discussion with you about SIGKILL and you didn't
> like it - please take a look at this CL)
> 
> I'm a little concerned that you had to use SIGKILL in test infrastructure to
> make Chrome quit properly. Shouldn't it quit with SIGTERM even with the
handler?

It looks like the uitest uses SIGTERM, which seems fine to me.  It's just the
task manager that uses SIGKILL, which also seems fine to me.

I was concerned about using SIGKILL to stop the browser processes in UI tests
because we should always succeed with SIGTERM or using the automation IPC close.

[Paweł Hajdan Jr., 2009-10-01 23:17:08]

On 2009/10/01 23:14:53, tony wrote:
> On 2009/10/01 22:49:51, Paweł Hajdan Jr. wrote:
> > +tony (I think I had earlier discussion with you about SIGKILL and you
didn't
> > like it - please take a look at this CL)
> > 
> > I'm a little concerned that you had to use SIGKILL in test infrastructure to
> > make Chrome quit properly. Shouldn't it quit with SIGTERM even with the
> handler?
> 
> It looks like the uitest uses SIGTERM, which seems fine to me.

Not exactly. The cleanup routine in chrome_process_util will use SIGKILL after
this patch.

[Chris Masone, 2009-10-02 14:58:28]

On Thu, Oct 1, 2009 at 4:17 PM, <phajdan.jr@chromium.org> wrote:

> On 2009/10/01 23:14:53, tony wrote:
>
>> On 2009/10/01 22:49:51, Pawe=C5=82 Hajdan Jr. wrote:
>> > +tony (I think I had earlier discussion with you about SIGKILL and you
>>
> didn't
>
>> > like it - please take a look at this CL)
>> >
>> > I'm a little concerned that you had to use SIGKILL in test
>> infrastructure to
>> > make Chrome quit properly. Shouldn't it quit with SIGTERM even with th=
e
>> handler?
>>
>
>  It looks like the uitest uses SIGTERM, which seems fine to me.
>>
>
> Not exactly. The cleanup routine in chrome_process_util will use SIGKILL
> after
> this patch.


But isn't that for cleaning up processes that won't die?  Isn't that what
SIGKILL is there for?

Besides, on Linux at least, if you don't provide and handler for SIGTERM,
you get SIG_DFL which, according to
http://lxr.linux.no/#linux+v2.6.31/include/linux/signal.h#L261, does the
same thing for both SIGTERM and SIGKILL...terminate (RARRR!)


>
>
> http://codereview.chromium.org/255036
>

[Chris Masone, 2009-10-04 02:54:32]

I've made it so that a) only the UI thread gets signals now, and b) the testing
framework still sends SIGTERM and not SIGKILL.

[Mark Mentovai, 2009-10-04 03:50:09]

Maybe a better solution would be to let the SIGTERM handler run on any thread,
but have it post a terminate task to the main thread's message loop.

http://codereview.chromium.org/255036/diff/9014/9018
File base/thread.cc (right):

http://codereview.chromium.org/255036/diff/9014/9018#newcode137
Line 137: #if defined(OS_POSIX)
Can you do this in platform_thread_posix.cc instead?  This file shouldn't have
platform-specific stuff.

http://codereview.chromium.org/255036/diff/9014/9018#newcode139
Line 139: DCHECK_EQ(pthread_sigmask(SIG_BLOCK, NULL, &set), 0);  // get signal
mask.
Never do this!

DCHECK disappears in official release builds.

You've done this elsewhere in this change, please check it carefully.

http://codereview.chromium.org/255036/diff/9014/9018#newcode141
Line 141: DCHECK_EQ(pthread_sigmask(SIG_BLOCK, &set, NULL), 0);  // set signal
mask.
Also, this is only effective at blocking SIGTERM on threads that we create
through this interface.  Third-party code and OS code may still spawn their own
threads.

(I'm not sure if this ever actually happens on Linux, but it certainly does on
the Mac, and your change has effect on the Mac too by virtue of being OS_POSIX.)

[Chris Masone, 2009-10-04 04:08:09]

On 2009/10/04 03:50:09, Mark Mentovai wrote:
> Maybe a better solution would be to let the SIGTERM handler run on any thread,
> but have it post a terminate task to the main thread's message loop.
> 

That does make more sense.  Is there code I can look at for how to instantiate a
terminate task, and how to send it to the appropriate thread?  Then I can do
away with a lot of the extra code here.


> http://codereview.chromium.org/255036/diff/9014/9018
> File base/thread.cc (right):
> 
> http://codereview.chromium.org/255036/diff/9014/9018#newcode137
> Line 137: #if defined(OS_POSIX)
> Can you do this in platform_thread_posix.cc instead?  This file shouldn't have
> platform-specific stuff.
> 
> http://codereview.chromium.org/255036/diff/9014/9018#newcode139
> Line 139: DCHECK_EQ(pthread_sigmask(SIG_BLOCK, NULL, &set), 0);  // get signal
> mask.
> Never do this!
> 
> DCHECK disappears in official release builds.
> 
> You've done this elsewhere in this change, please check it carefully.
> 
> http://codereview.chromium.org/255036/diff/9014/9018#newcode141
> Line 141: DCHECK_EQ(pthread_sigmask(SIG_BLOCK, &set, NULL), 0);  // set signal
> mask.
> Also, this is only effective at blocking SIGTERM on threads that we create
> through this interface.  Third-party code and OS code may still spawn their
own
> threads.
> 
> (I'm not sure if this ever actually happens on Linux, but it certainly does on
> the Mac, and your change has effect on the Mac too by virtue of being
OS_POSIX.)

[Chris Masone, 2009-10-05 23:22:26]

Ok, now the signal can get handled anywhere, and it posts a task to the main UI
thread to close all the browsers.

I've also noticed that UITest::TearDown is taking a really long time on my
machine...the calls to "fuser <singleton socket>" take multiple seconds each,
even when all the browsers are closed already.  But it seems to do that even
when I don't register my signal handler.

[Chris Masone, 2009-10-06 03:09:21]

The test slowness seems to be confined to my own workstation, which is good.

The CL is ready for review again, now that it's passed all the trybots.  Anyone?


On 2009/10/05 23:22:26, cmasone wrote:
> Ok, now the signal can get handled anywhere, and it posts a task to the main
UI
> thread to close all the browsers.
> 
> I've also noticed that UITest::TearDown is taking a really long time on my
> machine...the calls to "fuser <singleton socket>" take multiple seconds each,
> even when all the browsers are closed already.  But it seems to do that even
> when I don't register my signal handler.

[agl, 2009-10-06 20:10:29]

LGTM

[Mark Mentovai, 2009-10-06 20:15:16]

LG.  This is much cleaner.

http://codereview.chromium.org/255036/diff/7005/7006
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/255036/diff/7005/7006#newcode167
Line 167: if (main_loop)
Minor style nit: this needs {braces} on the conditionalized clause because it
spans multiple lines.

http://codereview.chromium.org/255036/diff/7005/7006#newcode173
Line 173: struct sigaction term_action;
Minor nit: you can just say |struct sigaction term_action = {0};| and that'll
zero-fill it for you.  No need to explicitly memset.  Same thing below at your
other sigaction call.

[Chris Masone, 2009-10-06 20:47:33]

Thanks, everyone!

I'll submit, along with everyone else in the world, when the tree is open :-)

http://codereview.chromium.org/255036/diff/7005/7006
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/255036/diff/7005/7006#newcode167
Line 167: if (main_loop)
On 2009/10/06 20:15:17, Mark Mentovai wrote:
> Minor style nit: this needs {braces} on the conditionalized clause because it
> spans multiple lines.

Done.

http://codereview.chromium.org/255036/diff/7005/7006#newcode173
Line 173: struct sigaction term_action;
On 2009/10/06 20:15:17, Mark Mentovai wrote:
> Minor nit: you can just say |struct sigaction term_action = {0};| and that'll
> zero-fill it for you.  No need to explicitly memset.  Same thing below at your
> other sigaction call.

This doesn't seem to work, as the first member of sigaction is a union.  So,
stick with the memsets