Closed Bug 993737 Opened 10 years ago Closed 10 years ago

LinuxPower.cpp can fire observer notifications on the wrong thread

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- fixed

People

(Reporter: khuey, Assigned: dhylands)

References

Details

Attachments

(1 file)

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?
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: 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.
(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.
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: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.