Fire notifications when reboot is requested, not scheduled

Fire notifications when reboot is requested, not scheduled

This CL fixes two bugs in the original implementation of the
runtime.onRestartRequired() notification:

1) Fire a notification when a reboot is requested, not scheduled.
2) Fix race at start-up, ensuring that reboots scheduled before
   observer registration are not missed.

BUG=433811
TEST=Updated and extended unit and browser tests

Committed: https://crrev.com/89aa682cd901d837e84369d65480fa0806923ea1
Cr-Commit-Position: refs/heads/master@{#304838}

[bartfab (slow), 2014-11-17 12:48:21]

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

[bartfab (slow), 2014-11-17 12:48:22]

Hi Xiyuan,

Could you take a look please?

[xiyuan, 2014-11-17 18:42:52]

https://codereview.chromium.org/727363002/diff/1/chrome/browser/chromeos/app_...
File chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc (left):

https://codereview.chromium.org/727363002/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc:118:
automatic_reboot_manager_->RemoveObserver(this);
Why do we need to remove this?

https://codereview.chromium.org/727363002/diff/1/chrome/browser/chromeos/app_...
File chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc (right):

https://codereview.chromium.org/727363002/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc:49:
OnRebootRequested(automatic_reboot_manager_->reboot_reason());
Think we should make KioskAppUpdateService::set_app_id a method and move this
code there so that we do this with the app running. Doing this in ctor might be
too early.

[bartfab (slow), 2014-11-17 19:58:24]

https://codereview.chromium.org/727363002/diff/1/chrome/browser/chromeos/app_...
File chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc (left):

https://codereview.chromium.org/727363002/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc:118:
automatic_reboot_manager_->RemoveObserver(this);
On 2014/11/17 18:42:52, xiyuan wrote:
> Why do we need to remove this?

We want to make sure that all observers are notified when the manager is about
to be destroyed so that they can reset their pointers to it.

In the original implementation, each observer was additionally required to
remove itself as an observer. The ObserverList in the manager would then verify
that it is empty during construction. This explicit removal does not really gain
us anything but carries the cost of an additional method call fore-and-back. I
changed it so that the ObserverList implicitly removes all observers on
destruction. They still get notified before of course, as they always did, so
that they can reset their pointers to the manager.

https://codereview.chromium.org/727363002/diff/1/chrome/browser/chromeos/app_...
File chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc (right):

https://codereview.chromium.org/727363002/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc:49:
OnRebootRequested(automatic_reboot_manager_->reboot_reason());
On 2014/11/17 18:42:52, xiyuan wrote:
> Think we should make KioskAppUpdateService::set_app_id a method and move this
> code there so that we do this with the app running. Doing this in ctor might
be
> too early.

Done.

[xiyuan, 2014-11-17 20:08:03]

LGTM

https://codereview.chromium.org/727363002/diff/1/chrome/browser/chromeos/app_...
File chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc (left):

https://codereview.chromium.org/727363002/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc:118:
automatic_reboot_manager_->RemoveObserver(this);
On 2014/11/17 19:58:24, bartfab wrote:
> On 2014/11/17 18:42:52, xiyuan wrote:
> > Why do we need to remove this?
> 
> We want to make sure that all observers are notified when the manager is about
> to be destroyed so that they can reset their pointers to it.
> 
> In the original implementation, each observer was additionally required to
> remove itself as an observer. The ObserverList in the manager would then
verify
> that it is empty during construction. This explicit removal does not really
gain
> us anything but carries the cost of an additional method call fore-and-back. I
> changed it so that the ObserverList implicitly removes all observers on
> destruction. They still get notified before of course, as they always did, so
> that they can reset their pointers to the manager.

Thanks for the clarification. I am slightly prefer to do the empty check but
also okay with the proposed change.

https://codereview.chromium.org/727363002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc (right):

https://codereview.chromium.org/727363002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc:84: }
nit: unrelated to this CL but would you mind add 

if (automatic_reboot_manager_)
  automatic_reboot_manager_->RemoveObserver(this);

here?

[bartfab (slow), 2014-11-18 11:04:47]

https://codereview.chromium.org/727363002/diff/1/chrome/browser/chromeos/app_...
File chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc (left):

https://codereview.chromium.org/727363002/diff/1/chrome/browser/chromeos/app_...
chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc:118:
automatic_reboot_manager_->RemoveObserver(this);
On 2014/11/17 20:08:03, xiyuan wrote:
> On 2014/11/17 19:58:24, bartfab wrote:
> > On 2014/11/17 18:42:52, xiyuan wrote:
> > > Why do we need to remove this?
> > 
> > We want to make sure that all observers are notified when the manager is
about
> > to be destroyed so that they can reset their pointers to it.
> > 
> > In the original implementation, each observer was additionally required to
> > remove itself as an observer. The ObserverList in the manager would then
> verify
> > that it is empty during construction. This explicit removal does not really
> gain
> > us anything but carries the cost of an additional method call fore-and-back.
I
> > changed it so that the ObserverList implicitly removes all observers on
> > destruction. They still get notified before of course, as they always did,
so
> > that they can reset their pointers to the manager.
> 
> Thanks for the clarification. I am slightly prefer to do the empty check but
> also okay with the proposed change.

I changed back to the empty check. I slightly prefer the other way, but you are
an owner, so your preference wins :).

https://codereview.chromium.org/727363002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc (right):

https://codereview.chromium.org/727363002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/app_mode/kiosk_app_update_service.cc:84: }
On 2014/11/17 20:08:03, xiyuan wrote:
> nit: unrelated to this CL but would you mind add 
> 
> if (automatic_reboot_manager_)
>   automatic_reboot_manager_->RemoveObserver(this);
> 
> here?

Done.

[bartfab (slow), 2014-11-18 11:05:51]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-11-18 11:06:36]

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

[commit-bot: I haz the power, 2014-11-18 11:57:26]

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

[commit-bot: I haz the power, 2014-11-18 11:57:27]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bartfab (slow), 2014-11-18 13:18:16]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-11-18 13:19:14]

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

[commit-bot: I haz the power, 2014-11-18 14:11:01]

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

[commit-bot: I haz the power, 2014-11-18 14:11:02]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bartfab (slow), 2014-11-18 16:17:51]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-11-18 16:19:07]

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

[commit-bot: I haz the power, 2014-11-18 16:59:20]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-11-18 17:00:07]

Patchset 5 (id:??) landed as
https://crrev.com/2bdfb49a86c46ac3a8a953a1e2f4e8e1bb41dc02
Cr-Commit-Position: refs/heads/master@{#304624}

[sky, 2014-11-18 17:58:32]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/741473002/ by sky@chromium.org.

The reason for reverting is: This seems to have caused crashes on a couple of
browser tests on the chromeos bots. See
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...
for details..

[bartfab (slow), 2014-11-18 21:47:37]

Hi Xiyuan,

Please take another look. The newest version of this CL fixes two problems:

1) It removes the source of the flakes. The issue was that the browser test was
trying to use a ScopedPathOverride(). It is too late at that point. The
AutomaticRebootManager has already started.

2) The test waits again until the AutomaticRebootManager has finished starting
up, but no longer uses the dangerous
content::RunAllBlockingPoolTasksUntilIdle(); method. Instead, it sends a task to
the blocking pool that is guaranteed to run after the AutomaticRebootManager's
initialization task and waits for it to finish.

[xiyuan, 2014-11-18 22:37:16]

Mostly good. Thank you for digging and get to the bottom of this.

https://codereview.chromium.org/727363002/diff/250001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc
(right):

https://codereview.chromium.org/727363002/diff/250001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:112:
base::RunLoop().RunUntilIdle();
Should we be doing "run_loop.Run()" so that it blocks until the posted quit
closure is called?

[bartfab (slow), 2014-11-19 09:21:09]

https://codereview.chromium.org/727363002/diff/250001/chrome/browser/chromeos...
File chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc
(right):

https://codereview.chromium.org/727363002/diff/250001/chrome/browser/chromeos...
chrome/browser/chromeos/app_mode/kiosk_app_update_service_browsertest.cc:112:
base::RunLoop().RunUntilIdle();
On 2014/11/18 22:37:16, xiyuan wrote:
> Should we be doing "run_loop.Run()" so that it blocks until the posted quit
> closure is called?

Oops, I was not spinning |run_loop| at all. My intention had been to use two
|RunLoop| instances: The first one is |run_loop|, which should wait until the
quit closure is called. A second instance should just flush any new tasks posted
by the UI loop to itself as a result. I put in the second but forgot the first
:).

[xiyuan, 2014-11-19 17:20:58]

LGTM++

[bartfab (slow), 2014-11-19 17:24:57]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-11-19 17:26:31]

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

[commit-bot: I haz the power, 2014-11-19 18:09:07]

Committed patchset #15 (id:270001)

[commit-bot: I haz the power, 2014-11-19 18:10:00]

Patchset 15 (id:??) landed as
https://crrev.com/89aa682cd901d837e84369d65480fa0806923ea1
Cr-Commit-Position: refs/heads/master@{#304838}