851721 - Do not receive xpcom-shutdown event when phone is powering off

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Anshul]

When powering off the device we do not receive the xpcom-shutdown event in our XPCOM component. We need this event to run the shutdown sequence.

Comment 1

[Dave Hylands [:dhylands]]

I was told that the xpcom-shutdown event isn't sent in release builds (of b2g)

Comment 2

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

(tef+, but not necessarily on making xpcom-shutdown event fire in release builds but on the more general problem of, "how can a live xpcom component get notified of a device powerdown/reboot request" with the Gonk backend.   It looks like we could hack something into LinuxPower.cpp for Gonk but that feels like a last resort hack.)

Comment 3

[[:fabrice] Fabrice Desré]

I'm not sure if we can use xpcom from within hal/ but if we can you could send observer notifications from there.

Comment 4

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

Or if not from the hal, maybe the mozPower implementation could emit a notification?

Comment 5

[[:fabrice] Fabrice Desré]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #4)
> Or if not from the hal, maybe the mozPower implementation could emit a
> notification?

Sure. That looks trivial to add in dom/power/PowerManager.cpp

Comment 6

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

Kewl.  I'll take a stab at this.  Any preferences to what the event should be called?  For our use case we don't need to distinguish between power-off and reboot but do we want to add this now just to be more general?   Forgive my ignorance, are observer notifications synchronous?

Comment 7

[[:fabrice] Fabrice Desré]

Ha, let's bikeshed the naming... I would do something like system-power-off and system-reboot (and yes, keep 2 different ones). Observer notifications are synchronous, so the notifyObservers() call only returns after all observers have run. This let us use them to inspect/modify http requests for instance.

Comment 8

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

Wow I was thinking of those exact event names too!

dom/power/PowerManager.cpp looks like it runs in a content process, would an observer notification there make it up to the main processed?  I'm guessing no, and if so then maybe dom/power/PowerManagerService.cpp is a better place for this?

Comment 9

[[:fabrice] Fabrice Desré]

We need these do happen in the parent indeed. Quickly reading the power manager code, I don't think it's remoted, but I can  be very wrong.

Comment 10

[Kan-Ru Chen [:kanru] (UTC+9)]

All the parent/child communication is done in the hal/ code. Even PowerManagerService is not remoted.

Comment 11

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

Attachment: Emit "system-reboot" and "system-power-off" events

Attached patch emits the two new events from PowerManagerService.cpp.  While PowerManagerService isn't remoted, turns out that only the system app uses mozPower to shutdown/reboot so we're already in the main process.  Not so future proof but solves the v1.0.1 need nicely and with low risk, and avoids adding a Gonk-only notification into LinuxPower.cpp.

Comment 12

[[:fabrice] Fabrice Desré]

Comment on attachment 727332 [details] [diff] [review]
Emit "system-reboot" and "system-power-off" events

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

I'll defer to Kanru that knows this part of the code well. The power API is usable by any certified app so I'm not a big fan of having that not being properly remoted.

::: dom/power/PowerManagerService.cpp
@@ +116,5 @@
>  PowerManagerService::Reboot()
>  {
> +  nsCOMPtr<nsIObserverService> obsServ = services::GetObserverService();
> +  if (obsServ)
> +    obsServ->NotifyObservers(nullptr, "system-reboot", nullptr);

I haven't checked in this file, but usually the code style is to use  if () { } even for single liners.

@@ +117,5 @@
>  {
> +  nsCOMPtr<nsIObserverService> obsServ = services::GetObserverService();
> +  if (obsServ)
> +    obsServ->NotifyObservers(nullptr, "system-reboot", nullptr);
> + 

nit: trailing ws

Comment 13

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

Would folks prefer to add this to hal/linux/LinuxPower.cpp?

Comment 14

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

Attachment: Emit "system-reboot" and "system-power-off" events, v2

Same stuff moved to LinuxPower.cpp.

Comment 15

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

Attachment: Emit "system-reboot" and "system-power-off" events, v3

Same as patch 2 but in hg style.   Carrying forward r+

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78e778d7e8f4

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/78e778d7e8f4

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/11ce13afa92b
https://hg.mozilla.org/releases/mozilla-b2g18/rev/c4e97cdad62a

Comment 19

[Darren]

Issue covered by test case: https://moztrap.mozilla.org/manage/cases/?filter-id=1330

Comment 20

[Cody Roesch [:croesch]]

Can you please provide steps to verify this fix - as we will blackbox test from the UI?

Comment 21

[cbarker]

Anshul if you can Verify this bug out, it would be much appreciated.

Comment 22

[Anshul]

I have already verified this bug.

Comment 23

[Alissa（leave from Mozilla）]

According to comment 22,delete keywords verifyme.