Chrome OS: Shutdown without blocking when SIGTERM is received.

Chrome OS: Shutdown without blocking when SIGTERM is received.
 Made ShuttingDownWithoutCloseBrowsers a explicit flag.
 Use END_SESSION shutdown type if chrome recieves SIGTERM AND there are tabs that may
 block shutdown.
 Make sure APP_TERMINATING is sent only once.

This fixes SIGABORT crash in two shutdown scenarios:
1) powering off when chrome has beforeunload handler, or
downloads in progress.
2) singout from screen locker when chrome has beforeunload handler or downloads in progress.

 This is simple version of fix to merge to release branch. I'm working on
another CL that will (hopefully) cleanup a bit more.

BUG=chromium-os:20460
TEST=see bug for repro step


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

[DaveMoore, 2011-10-13 16:50:05]

Other than the comment this looks pretty good. Did you explore combining this
logic with TimeLimitedSessionEnding()?

Also, we need a test for this condition: User has page with beforeunload handler
and they power off. Chrome exits cleanly. It doesn't have to exercise the
SIGTERM handler (although that would be nice) but it has to at least exercise
everything from  ExitCleanly() on.

http://codereview.chromium.org/8262009/diff/7001/chrome/browser/ui/browser_li...
File chrome/browser/ui/browser_list.cc (right):

http://codereview.chromium.org/8262009/diff/7001/chrome/browser/ui/browser_li...
chrome/browser/ui/browser_list.cc:301: if (notified)
Is double protecting this the right thing to do? Can't NotifyAndTerminate() and
NotifyAppTerminating() get combined?

[oshima, 2011-10-13 17:54:53]

On 2011/10/13 16:50:05, DaveMoore wrote:
> Other than the comment this looks pretty good. Did you explore combining this
> logic with TimeLimitedSessionEnding()?

I looked at how we use ShutdownWatcherHelper briefly, and I'm not sure if it
makes
sense to combine. In normal shutodwn, it kicks in at very last moment
(PostMainMessageLoop), which is quite far from when we notifies SM. It does make
sense
to change the tiemout value though.

> Also, we need a test for this condition: User has page with beforeunload
handler
> and they power off. Chrome exits cleanly. It doesn't have to exercise the
> SIGTERM handler (although that would be nice) but it has to at least exercise
> everything from  ExitCleanly() on.

I'm planning to add tests in separate CL i'm working on
(http://codereview.chromium.org/8271003. note that this is very incomplete and
haven't incorporated what I found in this CL). Achuith is looking into exactly
how we can test shutdown.

> 
>
http://codereview.chromium.org/8262009/diff/7001/chrome/browser/ui/browser_li...
> File chrome/browser/ui/browser_list.cc (right):
> 
>
http://codereview.chromium.org/8262009/diff/7001/chrome/browser/ui/browser_li...
> chrome/browser/ui/browser_list.cc:301: if (notified)
> Is double protecting this the right thing to do? Can't NotifyAndTerminate()
and
> NotifyAppTerminating() get combined?

[oshima, 2011-10-13 17:55:02]

http://codereview.chromium.org/8262009/diff/7001/chrome/browser/ui/browser_li...
File chrome/browser/ui/browser_list.cc (right):

http://codereview.chromium.org/8262009/diff/7001/chrome/browser/ui/browser_li...
chrome/browser/ui/browser_list.cc:301: if (notified)
On 2011/10/13 16:50:06, DaveMoore wrote:
> Is double protecting this the right thing to do? Can't NotifyAndTerminate()
and
> NotifyAppTerminating() get combined?

The way I understanding is that once we notified WM and SM that we're exitting,
we don't have to do it again. Is this wrong?

[DaveMoore, 2011-10-13 20:49:53]

I'm thinking about the fact that you prevent it in both NotifyAndTerminate() and
NotifyAppTerminating(). Can those functions be combined to have the check only
in one place?

On 2011/10/13 17:55:02, oshima wrote:
>
http://codereview.chromium.org/8262009/diff/7001/chrome/browser/ui/browser_li...
> File chrome/browser/ui/browser_list.cc (right):
> 
>
http://codereview.chromium.org/8262009/diff/7001/chrome/browser/ui/browser_li...
> chrome/browser/ui/browser_list.cc:301: if (notified)
> On 2011/10/13 16:50:06, DaveMoore wrote:
> > Is double protecting this the right thing to do? Can't NotifyAndTerminate()
> and
> > NotifyAppTerminating() get combined?
> 
> The way I understanding is that once we notified WM and SM that we're
exitting,
> we don't have to do it again. Is this wrong?

[oshima, 2011-10-13 23:23:49]

On 2011/10/13 20:49:53, DaveMoore wrote:
> I'm thinking about the fact that you prevent it in both NotifyAndTerminate()
and
> NotifyAppTerminating(). Can those functions be combined to have the check only
> in one place?

It may be possible but it'd require major change because we're using
NotifyAndTerminate differently on chromeos
than other platform. On other platform, it's used when there is no brower left,
while chromeos uses this
to notify SM to start shutdown. I'd like wait and see how the shutdown
simplification that we just discussed goes first.

I actually wonder why we're sending APP_TERMINATING in chrome's fast shutdown
path. My guess is that
we should actually use false (in FastShutdown) because there will be browser
window. What do you think?
Then I can change if (notified) to DCHECK(!notified) because it should happen
only once.

> On 2011/10/13 17:55:02, oshima wrote:
> >
>
http://codereview.chromium.org/8262009/diff/7001/chrome/browser/ui/browser_li...
> > File chrome/browser/ui/browser_list.cc (right):
> > 
> >
>
http://codereview.chromium.org/8262009/diff/7001/chrome/browser/ui/browser_li...
> > chrome/browser/ui/browser_list.cc:301: if (notified)
> > On 2011/10/13 16:50:06, DaveMoore wrote:
> > > Is double protecting this the right thing to do? Can't
NotifyAndTerminate()
> > and
> > > NotifyAppTerminating() get combined?
> > 
> > The way I understanding is that once we notified WM and SM that we're
> exitting,
> > we don't have to do it again. Is this wrong?

[oshima, 2011-10-13 23:25:49]

Forgot to mention. I found that NotifyAndTerminate should ask SM only when
it's user initiated, so I added g_session_manager_requested_shutdown flag. PTAL.

On 2011/10/13 23:23:49, oshima wrote:
> On 2011/10/13 20:49:53, DaveMoore wrote:
> > I'm thinking about the fact that you prevent it in both NotifyAndTerminate()
> and
> > NotifyAppTerminating(). Can those functions be combined to have the check
only
> > in one place?
> 
> It may be possible but it'd require major change because we're using
> NotifyAndTerminate differently on chromeos
> than other platform. On other platform, it's used when there is no brower
left,
> while chromeos uses this
> to notify SM to start shutdown. I'd like wait and see how the shutdown
> simplification that we just discussed goes first.
> 
> I actually wonder why we're sending APP_TERMINATING in chrome's fast shutdown
> path. My guess is that
> we should actually use false (in FastShutdown) because there will be browser
> window. What do you think?
> Then I can change if (notified) to DCHECK(!notified) because it should happen
> only once.
> 
> > On 2011/10/13 17:55:02, oshima wrote:
> > >
> >
>
http://codereview.chromium.org/8262009/diff/7001/chrome/browser/ui/browser_li...
> > > File chrome/browser/ui/browser_list.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/8262009/diff/7001/chrome/browser/ui/browser_li...
> > > chrome/browser/ui/browser_list.cc:301: if (notified)
> > > On 2011/10/13 16:50:06, DaveMoore wrote:
> > > > Is double protecting this the right thing to do? Can't
> NotifyAndTerminate()
> > > and
> > > > NotifyAppTerminating() get combined?
> > > 
> > > The way I understanding is that once we notified WM and SM that we're
> > exitting,
> > > we don't have to do it again. Is this wrong?

[achuithb, 2011-10-14 00:07:09]

lgtm

[DaveMoore, 2011-10-14 19:56:46]

On 2011/10/14 00:07:09, achuith.bhandarkar wrote:
> lgtm

We still need a test for the scenario I described above.

[oshima, 2011-10-17 20:18:15]

On 2011/10/14 19:56:46, DaveMoore wrote:
> On 2011/10/14 00:07:09, achuith.bhandarkar wrote:
> > lgtm
> 
> We still need a test for the scenario I described above.

What do you think about my question regarding fast_path argument in FastShutdown
call?
This is what used to be when I changed the code, and it's not clear to me why
we're doing this.

I tested this myselft and confirmed that we're currently sending APP_TERMINATING
twice during shutdown
when a user explicitly exited using wrench menu or ctrl-shift-q.

[achuithb, 2011-10-20 20:27:01]

Btw, last beta cut was on Tue for M15. Is this CL still planned for M15?

[oshima, 2011-10-21 17:18:34]

On 2011/10/20 20:27:01, achuith.bhandarkar wrote:
> Btw, last beta cut was on Tue for M15. Is this CL still planned for M15?

Dave, as per discussion we had earlier, I believe we want to check this in first
and then work on better solution + tests, correct? If so, can you give me lgtm?

[DaveMoore, 2011-10-21 17:25:28]

I wanted a test as part of this and then work on a better solution that doesn't
break it.

On 2011/10/21 17:18:34, oshima wrote:
> On 2011/10/20 20:27:01, achuith.bhandarkar wrote:
> > Btw, last beta cut was on Tue for M15. Is this CL still planned for M15?
> 
> Dave, as per discussion we had earlier, I believe we want to check this in
first
> and then work on better solution + tests, correct? If so, can you give me
lgtm?

[achuithb, 2011-10-21 21:08:53]

I'll take a look at testing this. 

It'll probably take a few days to develop tests - I'm thinking mid-week next
week at the earliest. After talking to Pawel, we feel like a ui test would be
the right approach, but I know very little about ui tests, and afaik, we don't
have any tests that cover shutdown behavior so there are quite a few unknowns
here.

[achuithb, 2011-10-23 01:11:11]

Test is here: http://codereview.chromium.org/8376001/

Only passes with this CL, as we expect.

[DaveMoore, 2011-10-24 16:50:53]

lgtm, now that there's a test.

[commit-bot: I haz the power, 2011-10-24 20:51:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/8262009/21001

[commit-bot: I haz the power, 2011-10-24 22:16:32]

Change committed as 106988