993737 - LinuxPower.cpp can fire observer notifications on the wrong thread

Status: RESOLVED FIXED

(Core :: Hardware Abstraction Layer (HAL), defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

If the force quit shutdown feature activates, we reboot the system from a non-main thread.  But we try to fire observers first which a) is not allowed and b) means we would be allowing arbitrary Gecko code to run here if the observer service did not protect us from ourselves.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Now that we landed bug 993734 this will crash rather than cleanly reboot.

Comment 2

[Gregor Wagner [:gwagner]]

Dave, do you have time to fix this?

Comment 3

[Dave Hylands [:dhylands]]

So what's the desired behaviour here?

What happens today:
1 - StartForceQuitWatchdog is called
2 - this launches ForceQuitWatchdog on a separate thread
3 - ForceQuitWatchdog waits for a while
4 - ForceQuitWatchDog calls QuitHard
5 - QuitHard calls PowerOff or Reboot
6 - Poweroff/Reboot calls NotifyObservers
7 - Poweroff/Reboot goes the poweroff/reboot

I looked in gecko and gaia and couldn't find anybody who listens for either system-reboot or system-power-off, so the simplest solution seems to be to just remove step 6.

If we want to keep step 6, presumably it needs to be moved someplace else, like between 2 and 3, and also be dispatched to the main thread. I would expect that you'd tell people you're going to shutdown and then have the watchdog force the issue (the way its currently coded, we just seem to wait for a period of time and then just perform the action).

Comment 4

[Gregor Wagner [:gwagner]]

(In reply to Dave Hylands [:dhylands] from comment #3)
> So what's the desired behaviour here?
> 
> What happens today:
> 1 - StartForceQuitWatchdog is called
> 2 - this launches ForceQuitWatchdog on a separate thread
> 3 - ForceQuitWatchdog waits for a while
> 4 - ForceQuitWatchDog calls QuitHard
> 5 - QuitHard calls PowerOff or Reboot
> 6 - Poweroff/Reboot calls NotifyObservers
> 7 - Poweroff/Reboot goes the poweroff/reboot
> 
> I looked in gecko and gaia and couldn't find anybody who listens for either
> system-reboot or system-power-off, so the simplest solution seems to be to
> just remove step 6.
> 

Can we remove them?

Comment 5

[Michael Vines [:m1] [:evilmachines]]

We have XPCOM extensions that require these events to perform processing during system shutdown.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I think the simplest path forward here is to just not call the observer service if we're off the main thread. If we are off the main thread then presumably the main thread is hung or busy and notifying observers won't work anyway.

Comment 7

[Dave Hylands [:dhylands]]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #6)
> I think the simplest path forward here is to just not call the observer
> service if we're off the main thread. If we are off the main thread then
> presumably the main thread is hung or busy and notifying observers won't
> work anyway.

The "normal" place that PowerOff/Reboot is called from is here:
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/sleep_menu.js#L353
which will be on the main thread.

This then calls:
http://dxr.mozilla.org/mozilla-central/source/dom/power/PowerManagerService.cpp#133
which starts the watchdog and calls the Hal.

The non-main-thread path only occurs if the watchdog fires, and in this scenario, the notification will have already been delivered, so that sounds like a sensible approach to me as well.

Comment 8

[Dave Hylands [:dhylands]]

Attachment: bug-993737-power.patch

This implements bent's suggestion in comment 6, which is to only fire the notifications if we're on the main thread.

Comment 9

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8414873 [details] [diff] [review]
bug-993737-power.patch

Review of attachment 8414873 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

Comment 10

[Dave Hylands [:dhylands]]

https://hg.mozilla.org/integration/b2g-inbound/rev/aed338f33904

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/aed338f33904