LinuxPower.cpp can fire observer notifications on the wrong thread

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: khuey, Assigned: dhylands)

Tracking

unspecified
mozilla32
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.0+, b2g-v2.0 fixed)

Details

Attachments

(1 attachment)

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.
Now that we landed bug 993734 this will crash rather than cleanly reboot.
blocking-b2g: --- → 2.0?
blocking-b2g: 2.0? → 2.0+
Dave, do you have time to fix this?
Assignee

Comment 3

5 years ago
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).
Flags: needinfo?(anygregor)
(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?
Flags: needinfo?(anygregor) → needinfo?(mvines)
We have XPCOM extensions that require these events to perform processing during system shutdown.
Flags: needinfo?(mvines)
Assignee

Updated

5 years ago
Assignee: nobody → dhylands
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.
Assignee

Comment 7

5 years ago
(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.
Assignee

Comment 8

5 years ago
This implements bent's suggestion in comment 6, which is to only fire the notifications if we're on the main thread.
Attachment #8414873 - Flags: review?(gsvelto)
Comment on attachment 8414873 [details] [diff] [review]
bug-993737-power.patch

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

Looks good to me!
Attachment #8414873 - Flags: review?(gsvelto) → review+
https://hg.mozilla.org/mozilla-central/rev/aed338f33904
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.