CrOS: Do not allow notifications to be added during shutdown.

CrOS: Do not allow notifications to be added during shutdown.

Attempt to fix crash in the bug. I think the cause of this is a notification being added after the shutdown process has started. Somewhat confirmed this by sending a notification after shutdown.

BUG=660962
TEST=unit_tests --gtest_filter="MessageCenterNotificationManagerTest.AddNotificationOnShutdown"

Review-Url: https://codereview.chromium.org/2776933006
Cr-Commit-Position: refs/heads/master@{#460789}
Committed: https://chromium.googlesource.com/chromium/src/+/ee04538066ee663d06c4d502435cbad08106a816

[sammiequon, 2017-03-28 19:03:20]

Description was changed from

==========
CrOS: Do not allow notifications to be added during shutdown.

Attempt to fix crash in the bug. I think the cause of this is a notification
being added after the shutdown process has started. Somewhat confirmed this by
sending a notification after shutdown.

BUG=660962
TEST=unit_tests
--gtest_filter="MessageCenterNotificationManagerTest.AddNotificationOnShutdown"
==========

to

==========
CrOS: Do not allow notifications to be added during shutdown.

Attempt to fix crash in the bug. I think the cause of this is a notification
being added after the shutdown process has started. Somewhat confirmed this by
sending a notification after shutdown.

BUG=660962
TEST=unit_tests
--gtest_filter="MessageCenterNotificationManagerTest.AddNotificationOnShutdown"
==========

[sammiequon, 2017-03-28 19:03:21]

sammiequon@chromium.org changed reviewers:
+ xiyuan@chromium.org

[sammiequon, 2017-03-28 19:03:44]

xiyuan@ - What do you think? Thanks!

[xiyuan, 2017-03-28 20:32:45]

lgtm

[sammiequon, 2017-03-28 20:42:44]

sammiequon@chromium.org changed reviewers:
+ dgn@chromium.org, sky@chromium.org

[sammiequon, 2017-03-28 20:43:46]

On 2017/03/28 20:32:45, xiyuan wrote:
> lgtm

Thanks!
dgn@, sky@ - This is alternative to https://codereview.chromium.org/2719813005/.
Please take a look. Thanks!

[sky, 2017-03-28 22:00:34]

https://codereview.chromium.org/2776933006/diff/1/chrome/browser/notification...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/2776933006/diff/1/chrome/browser/notification...
chrome/browser/notifications/message_center_notification_manager.cc:82: void
MessageCenterNotificationManager::Add(const Notification& notification,
Would it make more sense for this class to be destroyed earlier on, so that you
never end up during shutdown?

[sammiequon, 2017-03-28 23:12:09]

https://codereview.chromium.org/2776933006/diff/1/chrome/browser/notification...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/2776933006/diff/1/chrome/browser/notification...
chrome/browser/notifications/message_center_notification_manager.cc:82: void
MessageCenterNotificationManager::Add(const Notification& notification,
On 2017/03/28 22:00:34, sky wrote:
> Would it make more sense for this class to be destroyed earlier on, so that
you
> never end up during shutdown?

It would, but would this cause a bunch of other bugs? Also I'm not 100% certain
this will fix the crash. If I were to destroy this class early I would do it at
[1]? and then change all instances to check if the notification_manager() exists
before using it?

[1]
https://cs.chromium.org/chromium/src/chrome/browser/lifetime/application_life...

[sky, 2017-03-28 23:55:40]

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

[sky, 2017-03-28 23:56:54]

On 2017/03/28 23:12:09, sammiequon wrote:
>
https://codereview.chromium.org/2776933006/diff/1/chrome/browser/notification...
> File chrome/browser/notifications/message_center_notification_manager.cc
> (right):
> 
>
https://codereview.chromium.org/2776933006/diff/1/chrome/browser/notification...
> chrome/browser/notifications/message_center_notification_manager.cc:82: void
> MessageCenterNotificationManager::Add(const Notification& notification,
> On 2017/03/28 22:00:34, sky wrote:
> > Would it make more sense for this class to be destroyed earlier on, so that
> you
> > never end up during shutdown?
> 
> It would, but would this cause a bunch of other bugs? Also I'm not 100%
certain
> this will fix the crash. If I were to destroy this class early I would do it
at
> [1]? and then change all instances to check if the notification_manager()
exists
> before using it?
> 
> [1]
>
https://cs.chromium.org/chromium/src/chrome/browser/lifetime/application_life...

I'm not familiar enough with notifications to answer that question. +stevenjb .
Steven, see change to
https://codereview.chromium.org/2776933006/diff/1/chrome/browser/notification...
. The question is, instead of the early out should we shutdown notifications
earlier on so that we never have to worry about ignoring notification requests
during shutdown?

[stevenjb, 2017-03-29 00:21:33]

lgtm

https://codereview.chromium.org/2776933006/diff/1/chrome/browser/notification...
File chrome/browser/notifications/message_center_notification_manager.cc
(right):

https://codereview.chromium.org/2776933006/diff/1/chrome/browser/notification...
chrome/browser/notifications/message_center_notification_manager.cc:82: void
MessageCenterNotificationManager::Add(const Notification& notification,
On 2017/03/28 23:12:08, sammiequon wrote:
> On 2017/03/28 22:00:34, sky wrote:
> > Would it make more sense for this class to be destroyed earlier on, so that
> you
> > never end up during shutdown?
> 
> It would, but would this cause a bunch of other bugs? Also I'm not 100%
certain
> this will fix the crash. If I were to destroy this class early I would do it
at
> [1]? and then change all instances to check if the notification_manager()
exists
> before using it?
> 
> [1]
>
https://cs.chromium.org/chromium/src/chrome/browser/lifetime/application_life...

I would also be worried about dereferencing errors. We have a lot of calls (36)
to g_browser_process->notification_ui_manager() in the code. This seems like a
reasonable approach to me.

[stevenjb, 2017-03-29 00:25:11]

On 2017/03/29 00:21:33, stevenjb wrote:
> lgtm
> 
>
https://codereview.chromium.org/2776933006/diff/1/chrome/browser/notification...
> File chrome/browser/notifications/message_center_notification_manager.cc
> (right):
> 
>
https://codereview.chromium.org/2776933006/diff/1/chrome/browser/notification...
> chrome/browser/notifications/message_center_notification_manager.cc:82: void
> MessageCenterNotificationManager::Add(const Notification& notification,
> On 2017/03/28 23:12:08, sammiequon wrote:
> > On 2017/03/28 22:00:34, sky wrote:
> > > Would it make more sense for this class to be destroyed earlier on, so
that
> > you
> > > never end up during shutdown?
> > 
> > It would, but would this cause a bunch of other bugs? Also I'm not 100%
> certain
> > this will fix the crash. If I were to destroy this class early I would do it
> at
> > [1]? and then change all instances to check if the notification_manager()
> exists
> > before using it?
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/lifetime/application_life...
> 
> I would also be worried about dereferencing errors. We have a lot of calls
(36)
> to g_browser_process->notification_ui_manager() in the code. This seems like a
> reasonable approach to me.

That said, I'm not sure this fix is completely robust. I suspect that a call
just before shutdown could be just as dangerous?

What I think we really want here is an explicit 'StartShutDown' method that (a)
aborts any queued notifications and (b) prevents new ones from being generated
(.e.g by setting a member or somesuch).

[dgn, 2017-03-29 09:11:31]

lgtm

[xiyuan, 2017-03-29 16:48:02]

On 2017/03/29 00:25:11, stevenjb wrote:
> That said, I'm not sure this fix is completely robust. I suspect that a call
> just before shutdown could be just as dangerous?
> 

Yep. Currently, MessageCenterNotificationManager is disposed in
BrowserProcessImpl::StartTearDown. So far we don't see deference crashes. What
we saw is a notification somehow successfully gets added and triggers a CHECK
crash when trying to remove it during shutdown. Hopefully, this CL can prevent
that (no in-house repro, so not 100% sure).

> What I think we really want here is an explicit 'StartShutDown' method that
(a)
> aborts any queued notifications and (b) prevents new ones from being generated
> (.e.g by setting a member or somesuch).

We already have something for (a): MessageCenterNotificationManager has a
NotificationSystemObserver member which removes all existing notification on
NOTIFICATION_APP_TERMINATING [1].

This patch is sort of an approach for (b), just that the state used for the test
is in BrowserProcess.

[1]
https://cs.chromium.org/chromium/src/chrome/browser/notifications/notificatio...

[stevenjb, 2017-03-29 17:22:59]

On 2017/03/29 16:48:02, xiyuan wrote:
> On 2017/03/29 00:25:11, stevenjb wrote:
> > That said, I'm not sure this fix is completely robust. I suspect that a call
> > just before shutdown could be just as dangerous?
> > 
> 
> Yep. Currently, MessageCenterNotificationManager is disposed in
> BrowserProcessImpl::StartTearDown. So far we don't see deference crashes. What
> we saw is a notification somehow successfully gets added and triggers a CHECK
> crash when trying to remove it during shutdown. Hopefully, this CL can prevent
> that (no in-house repro, so not 100% sure).
> 
> > What I think we really want here is an explicit 'StartShutDown' method that
> (a)
> > aborts any queued notifications and (b) prevents new ones from being
generated
> > (.e.g by setting a member or somesuch).
> 
> We already have something for (a): MessageCenterNotificationManager has a
> NotificationSystemObserver member which removes all existing notification on
> NOTIFICATION_APP_TERMINATING [1].
> 
> This patch is sort of an approach for (b), just that the state used for the
test
> is in BrowserProcess.
> 
> [1]
>
https://cs.chromium.org/chromium/src/chrome/browser/notifications/notificatio...

Ah. I think it would be a little more clear to add something like
system_observer_->WasCanceled() instead of calling
g_browser_process->IsShuttingDown() here (and maybe a little bit safer,
depending on how closely chrome::NOTIFICATION_APP_TERMINATING and
g_browser_process->IsShuttingDown() are tied?), but this is probably fine as-is,
my lg still stands.

[xiyuan, 2017-03-29 17:31:53]

On 2017/03/29 17:22:59, stevenjb wrote:
> On 2017/03/29 16:48:02, xiyuan wrote:
> > On 2017/03/29 00:25:11, stevenjb wrote:
> > > That said, I'm not sure this fix is completely robust. I suspect that a
call
> > > just before shutdown could be just as dangerous?
> > > 
> > 
> > Yep. Currently, MessageCenterNotificationManager is disposed in
> > BrowserProcessImpl::StartTearDown. So far we don't see deference crashes.
What
> > we saw is a notification somehow successfully gets added and triggers a
CHECK
> > crash when trying to remove it during shutdown. Hopefully, this CL can
prevent
> > that (no in-house repro, so not 100% sure).
> > 
> > > What I think we really want here is an explicit 'StartShutDown' method
that
> > (a)
> > > aborts any queued notifications and (b) prevents new ones from being
> generated
> > > (.e.g by setting a member or somesuch).
> > 
> > We already have something for (a): MessageCenterNotificationManager has a
> > NotificationSystemObserver member which removes all existing notification on
> > NOTIFICATION_APP_TERMINATING [1].
> > 
> > This patch is sort of an approach for (b), just that the state used for the
> test
> > is in BrowserProcess.
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/notifications/notificatio...
> 
> Ah. I think it would be a little more clear to add something like
> system_observer_->WasCanceled() instead of calling
> g_browser_process->IsShuttingDown() here (and maybe a little bit safer,
> depending on how closely chrome::NOTIFICATION_APP_TERMINATING and
> g_browser_process->IsShuttingDown() are tied?), but this is probably fine
as-is,
> my lg still stands.

I like the idea more than using g_browser_process.

sammiequon@, wdyt about revising to something similar?

[sammiequon, 2017-03-29 17:59:05]

On 2017/03/29 17:31:53, xiyuan wrote:
> On 2017/03/29 17:22:59, stevenjb wrote:
> > On 2017/03/29 16:48:02, xiyuan wrote:
> > > On 2017/03/29 00:25:11, stevenjb wrote:
> > > > That said, I'm not sure this fix is completely robust. I suspect that a
> call
> > > > just before shutdown could be just as dangerous?
> > > > 
> > > 
> > > Yep. Currently, MessageCenterNotificationManager is disposed in
> > > BrowserProcessImpl::StartTearDown. So far we don't see deference crashes.
> What
> > > we saw is a notification somehow successfully gets added and triggers a
> CHECK
> > > crash when trying to remove it during shutdown. Hopefully, this CL can
> prevent
> > > that (no in-house repro, so not 100% sure).
> > > 
> > > > What I think we really want here is an explicit 'StartShutDown' method
> that
> > > (a)
> > > > aborts any queued notifications and (b) prevents new ones from being
> > generated
> > > > (.e.g by setting a member or somesuch).
> > > 
> > > We already have something for (a): MessageCenterNotificationManager has a
> > > NotificationSystemObserver member which removes all existing notification
on
> > > NOTIFICATION_APP_TERMINATING [1].
> > > 
> > > This patch is sort of an approach for (b), just that the state used for
the
> > test
> > > is in BrowserProcess.
> > > 
> > > [1]
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/notifications/notificatio...
> > 
> > Ah. I think it would be a little more clear to add something like
> > system_observer_->WasCanceled() instead of calling
> > g_browser_process->IsShuttingDown() here (and maybe a little bit safer,
> > depending on how closely chrome::NOTIFICATION_APP_TERMINATING and
> > g_browser_process->IsShuttingDown() are tied?), but this is probably fine
> as-is,
> > my lg still stands.
> 
> I like the idea more than using g_browser_process.
> 
> sammiequon@, wdyt about revising to something similar?

I agree.
stevenjb@, xiyuan@ - Please take another look. Thanks!

[sammiequon, 2017-03-29 17:59:11]

The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 17:59:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xiyuan, 2017-03-29 18:02:07]

Cool. lgtm++

[stevenjb, 2017-03-29 18:36:25]

Nice. lgtm++

[commit-bot: I haz the power, 2017-03-29 18:53:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-29 18:53:27]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sammiequon, 2017-03-29 19:57:24]

The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 19:58:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2017-03-29 20:47:10]

LGTM

[commit-bot: I haz the power, 2017-03-29 20:55:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-29 20:55:58]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sammiequon, 2017-03-29 23:58:24]

The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 23:59:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 02:10:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 02:10:48]

Dry run: This issue passed the CQ dry run.

[sammiequon, 2017-03-30 16:16:45]

The CQ bit was checked by sammiequon@chromium.org

[sammiequon, 2017-03-30 16:16:50]

The patchset sent to the CQ was uploaded after l-g-t-m from dgn@chromium.org,
xiyuan@chromium.org, stevenjb@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2776933006/#ps40001
(title: "Trybots.")

[commit-bot: I haz the power, 2017-03-30 16:17:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 16:24:11]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490890605170560,
"parent_rev": "565131ae89b7f786dad9d8743975bc628ee24888", "commit_rev":
"ee04538066ee663d06c4d502435cbad08106a816"}

[commit-bot: I haz the power, 2017-03-30 16:25:00]

Description was changed from

==========
CrOS: Do not allow notifications to be added during shutdown.

Attempt to fix crash in the bug. I think the cause of this is a notification
being added after the shutdown process has started. Somewhat confirmed this by
sending a notification after shutdown.

BUG=660962
TEST=unit_tests
--gtest_filter="MessageCenterNotificationManagerTest.AddNotificationOnShutdown"
==========

to

==========
CrOS: Do not allow notifications to be added during shutdown.

Attempt to fix crash in the bug. I think the cause of this is a notification
being added after the shutdown process has started. Somewhat confirmed this by
sending a notification after shutdown.

BUG=660962
TEST=unit_tests
--gtest_filter="MessageCenterNotificationManagerTest.AddNotificationOnShutdown"

Review-Url: https://codereview.chromium.org/2776933006
Cr-Commit-Position: refs/heads/master@{#460789}
Committed:
https://chromium.googlesource.com/chromium/src/+/ee04538066ee663d06c4d502435c...
==========

[commit-bot: I haz the power, 2017-03-30 16:25:02]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/ee04538066ee663d06c4d502435c...