Stop calling EndSession() in chrome::ExitCleanly()

Stop calling EndSession() in chrome::ExitCleanly()

In application_lifetime.h, it is said that what ExitCleanly() does is "Shutdown chrome cleanly without blocking".
However, it actually calls BrowserProcessImpl::EndSession(), which blocks the UI thread waiting for Preferences file to be written.

This EndSession() call was added by https://codereview.chromium.org/7273038.

By getting rid of this EndSession() call in ExitCleanly(), we can avoid blocking the UI thread waiting for the disk IO.
Also, we'll be able to avoid writing the Preferences file multiple times during shutdown.
This should make it less likely to hit SessionManager's 3-second timeout.

BUG=470501
Committed: https://crrev.com/16a569a8340a53623aa0d76d4c2fcdc5e6831594
Cr-Commit-Position: refs/heads/master@{#324413}

[hashimoto, 2015-04-01 12:49:04]

hashimoto@chromium.org changed reviewers:
+ oshima@chromium.org

[oshima, 2015-04-01 14:22:05]

"Blocking" in this context means no "onbeforeunload" dialog, not IO blocking.
The shutdown process does write preference by design, to record the shutdown
status. IIRC, this must be written 2~3 times during shutdown. 

If you want to change this, please consult with jar@.

[hashimoto, 2015-04-01 16:14:31]

On 2015/04/01 14:22:05, oshima wrote:
> "Blocking" in this context means no "onbeforeunload" dialog, not IO blocking.
> The shutdown process does write preference by design, to record the shutdown
> status. IIRC, this must be written 2~3 times during shutdown. 
> 
> If you want to change this, please consult with jar@.

This EndSession() call was added by you in
https://codereview.chromium.org/7273038.
I think you are more familiar with this Chrome OS specific code than jar@.

I doubt it's effective to wait for the file write to be defensive against
SessionManager's 3-second timeout because
base::ImportantFileWriter::WriteAtomically() (the function used to write pref
files) may take a few seconds to complete when the disk is busy.
(e.g. running "dd if=/dev/zero of=~/foo bs=1M" on shell)
Additional CommitPending() + blocking the UI thread may make it more likely to
be caught by the timeout, rather than mitigating the damage.

Also, you changed EndSession() to skip flushing Local State in
https://codereview.chromium.org/25484004.
I think it's OK to do the same thing for Preferences.

[oshima, 2015-04-01 17:26:14]

+abodenha@

On Wed, Apr 1, 2015 at 9:14 AM, <hashimoto@chromium.org> wrote:

> On 2015/04/01 14:22:05, oshima wrote:
>
>> "Blocking" in this context means no "onbeforeunload" dialog, not IO
>> blocking.
>> The shutdown process does write preference by design, to record the
>> shutdown
>> status. IIRC, this must be written 2~3 times during shutdown.
>>
>
>  If you want to change this, please consult with jar@.
>>
>
> This EndSession() call was added by you in
> https://codereview.chromium.org/7273038.
> I think you are more familiar with this Chrome OS specific code than jar@.
>
> I doubt it's effective to wait for the file write to be defensive against
> SessionManager's 3-second timeout because
> base::ImportantFileWriter::WriteAtomically() (the function used to write
> pref
> files) may take a few seconds to complete when the disk is busy.
> (e.g. running "dd if=/dev/zero of=~/foo bs=1M" on shell)
>

I don't think this is valid use case that we should worry about. If you can
go into shell, you can do all sorts of things that will damage chromeos.


> Additional CommitPending() + blocking the UI thread may make it more
> likely to
> be caught by the timeout, rather than mitigating the damage.
>
>
We're well ware of the issue, and I believe Albert has a monitor to catch
the event if we see the jump in the number of sigaborts during shutdown.
Do we have more crashes during shutdown now?

> Also, you changed EndSession() to skip flushing Local State in
> https://codereview.chromium.org/25484004.
> I think it's OK to do the same thing for Preferences.

It's long time ago and I'm sorry if I'm wrong, but I believe this may have
visible impact (shows crash infobar), vs just stats. You should look into
why this is done before removing.
And as you can see there is a long discussion with jar@ about if this is
ok. That's why I suggested to discuss with jar@ (or whoever took over this
role).


>
> https://codereview.chromium.org/1057533003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hashimoto, 2015-04-02 07:20:43]

On 2015/04/01 17:26:14, oshima wrote:
> +abodenha@
> 
> On Wed, Apr 1, 2015 at 9:14 AM, <mailto:hashimoto@chromium.org> wrote:
> 
> > On 2015/04/01 14:22:05, oshima wrote:
> >
> >> "Blocking" in this context means no "onbeforeunload" dialog, not IO
> >> blocking.
> >> The shutdown process does write preference by design, to record the
> >> shutdown
> >> status. IIRC, this must be written 2~3 times during shutdown.
> >>
> >
> >  If you want to change this, please consult with jar@.
> >>
> >
> > This EndSession() call was added by you in
> > https://codereview.chromium.org/7273038.
> > I think you are more familiar with this Chrome OS specific code than jar@.
> >
> > I doubt it's effective to wait for the file write to be defensive against
> > SessionManager's 3-second timeout because
> > base::ImportantFileWriter::WriteAtomically() (the function used to write
> > pref
> > files) may take a few seconds to complete when the disk is busy.
> > (e.g. running "dd if=/dev/zero of=~/foo bs=1M" on shell)
> >
> 
> I don't think this is valid use case that we should worry about. If you can
> go into shell, you can do all sorts of things that will damage chromeos.
Sorry for being unclear.
This is meant to be just an example way to reliably reproduce the situation on a
dev machine.

You can do anything to make the disk and worker threads busy. (e.g. copying a
large file in Files.app, downloading a large file)
> 
> 
> > Additional CommitPending() + blocking the UI thread may make it more
> > likely to
> > be caught by the timeout, rather than mitigating the damage.
> >
> >
> We're well ware of the issue, and I believe Albert has a monitor to catch
> the event if we see the jump in the number of sigaborts during shutdown.
> Do we have more crashes during shutdown now?
It's #3 browser crash on the current stable. (crbug.com/418627)
> 
> > Also, you changed EndSession() to skip flushing Local State in
> > https://codereview.chromium.org/25484004.
> > I think it's OK to do the same thing for Preferences.
> 
> It's long time ago and I'm sorry if I'm wrong, but I believe this may have
> visible impact (shows crash infobar), vs just stats. You should look into
> why this is done before removing.
I looked at the change list and the bug (crbug.com/85936 which contained no
information about this EndSession() usage).

IIUC the reasoning behind this EndSession() usage is to workaround 3-second
timeout crash so that it's not visible to the user.
This is reasonable if it is guaranteed that one additional file write doesn't
cost more than a few milliseconds.

However, one additional file write may cost a number of seconds when the disk is
busy.
This should make it more likely to hit the 3-second timeout.
I think the benefit is not worth paying this cost.

Do you have any number recorded which describes how many crashes have been
hidden by this EndSession() call workaround?

> And as you can see there is a long discussion with jar@ about if this is
> ok. That's why I suggested to discuss with jar@ (or whoever took over this
> role).
Here I'm introducing nothing new.
Just reverting it to the state before you replaced EndSession() with
MarkAsCleanShutdown().

In other EndSession() user sites (chrome::SessionEnding() and
ChromeRestartRequest), chrome kills itself immediately after calling
EndSession() so that waiting for Preferences file flush is important.
However, in this ExitCleanly() function, it'll continue to perform the normal
shutdown.
IMO EndSession() shouldn't be used when we can expect normal shutdown to write
the Preferences file properly.
(BTW, I found EndSession() is broken with freon and filed crbug.com/472981)
> 
> 
> >
> > https://codereview.chromium.org/1057533003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[oshima, 2015-04-02 14:23:22]

On Thu, Apr 2, 2015 at 12:20 AM, <hashimoto@chromium.org> wrote:

> On 2015/04/01 17:26:14, oshima wrote:
>
>> +abodenha@
>>
>
>  On Wed, Apr 1, 2015 at 9:14 AM, <mailto:hashimoto@chromium.org> wrote:
>>
>
>  > On 2015/04/01 14:22:05, oshima wrote:
>> >
>> >> "Blocking" in this context means no "onbeforeunload" dialog, not IO
>> >> blocking.
>> >> The shutdown process does write preference by design, to record the
>> >> shutdown
>> >> status. IIRC, this must be written 2~3 times during shutdown.
>> >>
>> >
>> >  If you want to change this, please consult with jar@.
>> >>
>> >
>> > This EndSession() call was added by you in
>> > https://codereview.chromium.org/7273038.
>> > I think you are more familiar with this Chrome OS specific code than
>> jar@.
>> >
>> > I doubt it's effective to wait for the file write to be defensive
>> against
>> > SessionManager's 3-second timeout because
>> > base::ImportantFileWriter::WriteAtomically() (the function used to
>> write
>> > pref
>> > files) may take a few seconds to complete when the disk is busy.
>> > (e.g. running "dd if=/dev/zero of=~/foo bs=1M" on shell)
>> >
>>
>
>  I don't think this is valid use case that we should worry about. If you
>> can
>> go into shell, you can do all sorts of things that will damage chromeos.
>>
> Sorry for being unclear.
> This is meant to be just an example way to reliably reproduce the
> situation on a
> dev machine.
>
> You can do anything to make the disk and worker threads busy. (e.g.
> copying a
> large file in Files.app, downloading a large file)


Ideally, we should stop/kill these tasks before shutdown starts instead.
(I fixed some of these that I found in the past) We have to update the
local state/preference regardless, so such tasks can still lead to sigabort.

Have you look into them?


>
>
>  > Additional CommitPending() + blocking the UI thread may make it more
>> > likely to
>> > be caught by the timeout, rather than mitigating the damage.
>> >
>> >
>> We're well ware of the issue, and I believe Albert has a monitor to catch
>> the event if we see the jump in the number of sigaborts during shutdown.
>> Do we have more crashes during shutdown now?
>>
> It's #3 browser crash on the current stable. (crbug.com/418627)




>
>  > Also, you changed EndSession() to skip flushing Local State in
>> > https://codereview.chromium.org/25484004.
>> > I think it's OK to do the same thing for Preferences.
>>
>
>  It's long time ago and I'm sorry if I'm wrong, but I believe this may have
>> visible impact (shows crash infobar), vs just stats. You should look into
>> why this is done before removing.
>>
> I looked at the change list and the bug (crbug.com/85936 which contained
> no
> information about this EndSession() usage).
>
> IIUC the reasoning behind this EndSession() usage is to workaround 3-second
> timeout crash so that it's not visible to the user.
> This is reasonable if it is guaranteed that one additional file write
> doesn't
> cost more than a few milliseconds.


> However, one additional file write may cost a number of seconds when the
> disk is
> busy.

This should make it more likely to hit the 3-second timeout.
> I think the benefit is not worth paying this cost.
>
>
Note that 3 second timeout is a workaround on ChromeOS in case chrome didn't
shutdown in time for no good reason (such as a bug, hang, etc). Other
platforms doesn't have this.
And the number was picked rather randomly. (We have discussed to increase
this on arm
device for example). We know that this can be the source of crash, but
there are many other
sources, so it was worth it.

If the source of most crash during shutdown is due to this now, and
eliminating this can
reduce it, then it may makes sense. Are you going to eliminate other places
as well?

Do you have any number recorded which describes how many crashes have been
> hidden by this EndSession() call workaround?


I don't (it's more than 3 yeas ago), sorry.


>
>  And as you can see there is a long discussion with jar@ about if this is
>> ok. That's why I suggested to discuss with jar@ (or whoever took over
>> this
>> role).
>>
> Here I'm introducing nothing new.
> Just reverting it to the state before you replaced EndSession() with
> MarkAsCleanShutdown().


> In other EndSession() user sites (chrome::SessionEnding() and
> ChromeRestartRequest), chrome kills itself immediately after calling
> EndSession() so that waiting for Preferences file flush is important.
> However, in this ExitCleanly() function, it'll continue to perform the
> normal
> shutdown.
> IMO EndSession() shouldn't be used when we can expect normal shutdown to
> write
> the Preferences file properly.
> (BTW, I found EndSession() is broken with freon and filed crbug.com/472981
> )
>

There are two types of shutdown on other platforms, interruptable and
non-interruptable.
(when shutting down). On chromeos, shutdown is always non-interruptable, so
technically
speaking we can kill chrome after this. ChromeOS just continues because
that's the only
shutdown path.

And ExitCleanly() can be called without AttemptUserExit(), when OS shutdown
chrome. (power button,
auto update), if we skip this, then chrome may not update the state in this
case. We may be able to
tolerate such case though.

Can we have VG today? I'm fine if this don't have negative impact, but want
to understand details
of what you're changing in other places first.

And thank you for looking into this. Shutdown is really nasty part of
chrome.


>
>  >
>> > https://codereview.chromium.org/1057533003/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
> https://codereview.chromium.org/1057533003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hashimoto, 2015-04-03 08:54:31]

On 2015/04/02 14:23:22, oshima wrote:
> On Thu, Apr 2, 2015 at 12:20 AM, <mailto:hashimoto@chromium.org> wrote:
> 
> > On 2015/04/01 17:26:14, oshima wrote:
> >
> >> +abodenha@
> >>
> >
> >  On Wed, Apr 1, 2015 at 9:14 AM, <mailto:hashimoto@chromium.org> wrote:
> >>
> >
> >  > On 2015/04/01 14:22:05, oshima wrote:
> >> >
> >> >> "Blocking" in this context means no "onbeforeunload" dialog, not IO
> >> >> blocking.
> >> >> The shutdown process does write preference by design, to record the
> >> >> shutdown
> >> >> status. IIRC, this must be written 2~3 times during shutdown.
> >> >>
> >> >
> >> >  If you want to change this, please consult with jar@.
> >> >>
> >> >
> >> > This EndSession() call was added by you in
> >> > https://codereview.chromium.org/7273038.
> >> > I think you are more familiar with this Chrome OS specific code than
> >> jar@.
> >> >
> >> > I doubt it's effective to wait for the file write to be defensive
> >> against
> >> > SessionManager's 3-second timeout because
> >> > base::ImportantFileWriter::WriteAtomically() (the function used to
> >> write
> >> > pref
> >> > files) may take a few seconds to complete when the disk is busy.
> >> > (e.g. running "dd if=/dev/zero of=~/foo bs=1M" on shell)
> >> >
> >>
> >
> >  I don't think this is valid use case that we should worry about. If you
> >> can
> >> go into shell, you can do all sorts of things that will damage chromeos.
> >>
> > Sorry for being unclear.
> > This is meant to be just an example way to reliably reproduce the
> > situation on a
> > dev machine.
> >
> > You can do anything to make the disk and worker threads busy. (e.g.
> > copying a
> > large file in Files.app, downloading a large file)
> 
> 
> Ideally, we should stop/kill these tasks before shutdown starts instead.
This is another concern about this usage of EndSession().
By blocking the UI thread, we may be preventing these tasks from aborting.
> (I fixed some of these that I found in the past) We have to update the
> local state/preference regardless, so such tasks can still lead to sigabort.
> 
> Have you look into them?
According to my crappy research
(https://code.google.com/p/chromium/issues/detail?id=418627#c14),
in 42% of the reports, the browser process is busy writing some kind of pref
file (Preferences, Local State, RLZ Data or something else) at the timing of the
crash.

SessionStorageDatabase is responsible to about 30% of the reports and it was
addressed in crbug.com/462500.

I haven't looked at processes other than chrome (e.g. update_engine) yet.
> 
> 
> >
> >
> >  > Additional CommitPending() + blocking the UI thread may make it more
> >> > likely to
> >> > be caught by the timeout, rather than mitigating the damage.
> >> >
> >> >
> >> We're well ware of the issue, and I believe Albert has a monitor to catch
> >> the event if we see the jump in the number of sigaborts during shutdown.
> >> Do we have more crashes during shutdown now?
> >>
> > It's #3 browser crash on the current stable. (crbug.com/418627)
> 
> 
> 
> 
> >
> >  > Also, you changed EndSession() to skip flushing Local State in
> >> > https://codereview.chromium.org/25484004.
> >> > I think it's OK to do the same thing for Preferences.
> >>
> >
> >  It's long time ago and I'm sorry if I'm wrong, but I believe this may have
> >> visible impact (shows crash infobar), vs just stats. You should look into
> >> why this is done before removing.
> >>
> > I looked at the change list and the bug (crbug.com/85936 which contained
> > no
> > information about this EndSession() usage).
> >
> > IIUC the reasoning behind this EndSession() usage is to workaround 3-second
> > timeout crash so that it's not visible to the user.
> > This is reasonable if it is guaranteed that one additional file write
> > doesn't
> > cost more than a few milliseconds.
> 
> 
> > However, one additional file write may cost a number of seconds when the
> > disk is
> > busy.
> 
> This should make it more likely to hit the 3-second timeout.
> > I think the benefit is not worth paying this cost.
> >
> >
> Note that 3 second timeout is a workaround on ChromeOS in case chrome didn't
> shutdown in time for no good reason (such as a bug, hang, etc). Other
> platforms doesn't have this.
> And the number was picked rather randomly. (We have discussed to increase
> this on arm
> device for example). We know that this can be the source of crash, but
> there are many other
> sources, so it was worth it.
> 
> If the source of most crash during shutdown is due to this now, and
> eliminating this can
> reduce it, then it may makes sense. Are you going to eliminate other places
> as well?
> 
> Do you have any number recorded which describes how many crashes have been
> > hidden by this EndSession() call workaround?
> 
> 
> I don't (it's more than 3 yeas ago), sorry.
> 
> 
> >
> >  And as you can see there is a long discussion with jar@ about if this is
> >> ok. That's why I suggested to discuss with jar@ (or whoever took over
> >> this
> >> role).
> >>
> > Here I'm introducing nothing new.
> > Just reverting it to the state before you replaced EndSession() with
> > MarkAsCleanShutdown().
> 
> 
> > In other EndSession() user sites (chrome::SessionEnding() and
> > ChromeRestartRequest), chrome kills itself immediately after calling
> > EndSession() so that waiting for Preferences file flush is important.
> > However, in this ExitCleanly() function, it'll continue to perform the
> > normal
> > shutdown.
> > IMO EndSession() shouldn't be used when we can expect normal shutdown to
> > write
> > the Preferences file properly.
> > (BTW, I found EndSession() is broken with freon and filed crbug.com/472981
> > )
> >
> 
> There are two types of shutdown on other platforms, interruptable and
> non-interruptable.
> (when shutting down). On chromeos, shutdown is always non-interruptable, so
> technically
> speaking we can kill chrome after this. ChromeOS just continues because
> that's the only
> shutdown path.
If it's really OK to terminate chrome at this point, we should call
chrome::SessionEnding() which intentionally kills chrome after calling
EndSession().
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/lif...)
Windows is calling SessionEnding() to exit quickly when the OS is shutting down.
> 
> And ExitCleanly() can be called without AttemptUserExit(), when OS shutdown
> chrome. (power button,
> auto update), if we skip this, then chrome may not update the state in this
> case. We may be able to
> tolerate such case though.
ExitCleanly() calls AttemptExitInternal() and IIUC this results in clean
shutdown so we can expect the pref to be written to the disk.
> 
> Can we have VG today? I'm fine if this don't have negative impact, but want
> to understand details
> of what you're changing in other places first.
> 
> And thank you for looking into this. Shutdown is really nasty part of
> chrome.
> 
> 
> >
> >  >
> >> > https://codereview.chromium.org/1057533003/
> >> >
> >>
> >
> >  To unsubscribe from this group and stop receiving emails from it, send an
> >>
> > email
> >
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >>
> >
> >
> > https://codereview.chromium.org/1057533003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[oshima, 2015-04-07 00:22:16]

discussed offline and I'm convinced that this is fine.

We should keep eye on feedback reports to make sure users won't gets "crash"
inforbar after logout, but this CL lgtm.

[hashimoto, 2015-04-07 13:50:41]

hashimoto@chromium.org changed reviewers:
+ sky@chromium.org

[hashimoto, 2015-04-07 13:50:42]

sky@, could you review this change as an owner of
chrome/browser/lifetime/application_lifetime.cc?

[sky, 2015-04-08 19:43:14]

LGTM - we need test coverage of this stuff. It's too easy to regress.

[hashimoto, 2015-04-09 09:11:54]

On 2015/04/08 19:43:14, sky wrote:
> LGTM - we need test coverage of this stuff. It's too easy to regress.
Thanks,
Filed http://crbug.com/475420 for the test.

[hashimoto, 2015-04-09 09:11:56]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2015-04-09 09:12:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1057533003/1

[commit-bot: I haz the power, 2015-04-09 11:25:38]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-04-09 11:26:23]

Patchset 1 (id:??) landed as
https://crrev.com/16a569a8340a53623aa0d76d4c2fcdc5e6831594
Cr-Commit-Position: refs/heads/master@{#324413}