Closed Bug 1192122 Opened 5 years ago Closed 4 years ago

Safe mode startup

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(feature-b2g:2.5+, firefox43 verified)

VERIFIED FIXED
FxOS-S6 (04Sep)
feature-b2g 2.5+
Tracking Status
firefox43 --- verified

People

(Reporter: fabrice, Assigned: fabrice)

References

Details

Attachments

(2 files, 3 obsolete files)

No description provided.
Attached patch Part 1, set an android property (obsolete) — Splinter Review
This set the 'b2g.safe_mode' property to one if the power key is pressed during gecko startup.

I'm not really happy with the input device enumeration, but I didn't find any other way to know which device provides the power key (eg. on the z3c it's device id #4).
Attachment #8644767 - Flags: review?(mwu)
Comment on attachment 8644767 [details] [diff] [review]
Part 1, set an android property

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

::: widget/gonk/nsAppShell.cpp
@@ +915,5 @@
>      return rv;
>  }
>  
> +void
> +nsAppShell::CheckPowerKey()

We should be able to move this to GeckoInputDispatcher and avoid making changes to nsAppShell.

@@ +929,5 @@
> +    while (powerState == AKEY_STATE_UNKNOWN && deviceId < 32) {
> +        powerState = mEventHub->getKeyCodeState(deviceId++, AKEYCODE_POWER);
> +        if (powerState == AKEY_STATE_DOWN) {
> +            // Power is pressed while we startup, mark safe mode.
> +            property_set("b2g.safe_mode", "1");

In theory, this seems racy. If input enumeration takes too long, the code checking this might miss it.

I also wonder if we want to always set this. If b2g crashes and the user happened to be holding the power button, this will enable safe mode while b2g is restarting.
Michael, I added a pref to prevent the race, that the consumer code needs to observe. It's used in part 2.

I think it's ok to always set the property, since we show a confirmation screen to the user in all cases. We don't blindly restart in safe mode.
Attachment #8644767 - Attachment is obsolete: true
Attachment #8644767 - Flags: review?(mwu)
Attachment #8646562 - Flags: review?(mwu)
Attached patch Part 2, shell.js hookup (obsolete) — Splinter Review
This uses the infrastructure from part 1, and relies on gaia system app providing a safe_mode.html page.

I have a gaia implementation in https://github.com/fabricedesre/gaia/tree/safe_mode but it's not totally spec conformant (https://drive.google.com/a/mozilla.com/file/d/0B8kj4Mlm-HJeWm5pWUdLUGU4X00/view) in terms of colors...

Mike, do you want to take over the gaia patch?
Flags: needinfo?(mhenretty)
Attachment #8646565 - Flags: review?(21)
Comment on attachment 8646562 [details] [diff] [review]
Part 1, set an android property v2

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

::: widget/gonk/nsAppShell.cpp
@@ +931,5 @@
> +        powerState = mEventHub->getKeyCodeState(deviceId++, AKEYCODE_POWER);
> +
> +        if (powerState == AKEY_STATE_DOWN) {
> +            // Power is pressed while we startup, mark safe mode.
> +            property_set("b2g.safe_mode", "1");

I think if we set this to 0 when the power button isn't down, we don't need b2g.safe_mode_state_ready.
(In reply to Michael Wu [:mwu] from comment #5)
> Comment on attachment 8646562 [details] [diff] [review]
> Part 1, set an android property v2
> 
> Review of attachment 8646562 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/nsAppShell.cpp
> @@ +931,5 @@
> > +        powerState = mEventHub->getKeyCodeState(deviceId++, AKEYCODE_POWER);
> > +
> > +        if (powerState == AKEY_STATE_DOWN) {
> > +            // Power is pressed while we startup, mark safe mode.
> > +            property_set("b2g.safe_mode", "1");
> 
> I think if we set this to 0 when the power button isn't down, we don't need
> b2g.safe_mode_state_ready.

The nice thing with using a pref is that the preference service let us hook an observer for pref changes, instead of polling for the android property. Or I could get rid of the android property, and use a tri-state pref: not-ready, set-to-no, set-to-yes.
(In reply to [:fabrice] Fabrice Desré from comment #4)
> I have a gaia implementation in
> https://github.com/fabricedesre/gaia/tree/safe_mode but it's not totally
> spec conformant
> (https://drive.google.com/a/mozilla.com/file/d/0B8kj4Mlm-HJeWm5pWUdLUGU4X00/
> view) in terms of colors...
> 
> Mike, do you want to take over the gaia patch?

Sure! I'll work on it in bug 1193737.
Flags: needinfo?(mhenretty)
(In reply to [:fabrice] Fabrice Desré from comment #6)
> The nice thing with using a pref is that the preference service let us hook
> an observer for pref changes, instead of polling for the android property.
> Or I could get rid of the android property, and use a tri-state pref:
> not-ready, set-to-no, set-to-yes.

Oh, I didn't notice you were using a gecko pref. How does that work? Who resets that state when the device powers off/reboots?
(In reply to Michael Wu [:mwu] from comment #8)
> (In reply to [:fabrice] Fabrice Desré from comment #6)
> > The nice thing with using a pref is that the preference service let us hook
> > an observer for pref changes, instead of polling for the android property.
> > Or I could get rid of the android property, and use a tri-state pref:
> > not-ready, set-to-no, set-to-yes.
> 
> Oh, I didn't notice you were using a gecko pref. How does that work? Who
> resets that state when the device powers off/reboots?

We reset it in nsAppShell constructor which we know runs before we can load shell.js
feature-b2g: --- → 2.5+
Now using a tri-state (unset, yes, no) pref instead of the android prop + gecko pref.
Attachment #8646562 - Attachment is obsolete: true
Attachment #8646562 - Flags: review?(mwu)
Attachment #8648873 - Flags: review?(mwu)
Updated to match the pref change.
Attachment #8646565 - Attachment is obsolete: true
Attachment #8646565 - Flags: review?(21)
Attachment #8648874 - Flags: review?(21)
Comment on attachment 8648873 [details] [diff] [review]
Part 1, set a pref during startup

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

::: widget/gonk/nsAppShell.cpp
@@ +929,5 @@
> +    // EventHub doesn't report the number of devices.
> +    while (powerState != AKEY_STATE_DOWN && deviceId < 32) {
> +        powerState = mEventHub->getKeyCodeState(deviceId++, AKEYCODE_POWER);
> +
> +        if (powerState == AKEY_STATE_DOWN) {

I think this check would be easier to understand if it were moved out of the while loop.
Attachment #8648873 - Flags: review?(mwu) → review+
Comment on attachment 8648874 [details] [diff] [review]
Part 2, shell.js hookup

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

301 to ferjm
Attachment #8648874 - Flags: review?(21) → review?(ferjmoreno)
Target Milestone: --- → FxOS-S6 (04Sep)
Comment on attachment 8648874 [details] [diff] [review]
Part 2, shell.js hookup

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

LGTM.

I am not sure if this is possible, but could you add a chrome test to test SafeMode.jsm?

::: b2g/chrome/content/screen.js
@@ +15,1 @@
>  // We do this on ContentStart because querying the displayDPI fails otherwise.

...on ContentStart and SafeModeStart because...

::: b2g/components/SafeMode.jsm
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

A comment explaining what Safe Mode is and what do we do here (wait for pref, check pref, show top level fullscreen iframe, wait for user, etc.) would be highly appreciated :)

@@ +7,5 @@
> +this.EXPORTED_SYMBOLS = ["SafeMode"];
> +
> +const Cu = Components.utils;
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

I think there's no need to import XPCOMUtils here.

@@ +16,5 @@
> +const kSafeModePref = "b2g.safe_mode";
> +const kSafeModePage = "safe_mode.html";
> +
> +function debug(aStr) {
> +  dump("-*- SafeMode: " + aStr + "\n");

Comment this before landing, please.

@@ +20,5 @@
> +  dump("-*- SafeMode: " + aStr + "\n");
> +}
> +
> +this.SafeMode = {
> +  init: function(aWindow) {

It seems that we can get rid of the "init" function and pass the window to "check"

@@ +53,5 @@
> +
> +  // Returns a Promise that resolves once we have decided to run in safe mode
> +  // or not. All the safe mode switching actions happen before resolving the
> +  // promise.
> +  check: function() {

It would be good to expose only "check". Or at least, could you rename "waitForPref" and "waitForUser" to "_waitForPref" and "_waitForUser" and move "check" after these two functions, please?

@@ +64,5 @@
> +    return this.waitForPref().then(this.waitForUser);
> +  },
> +
> +  // Resolves once the user has decided how to start.
> +  // Note that all the actions happen here, so there no other action from

...so there is no other...
Attachment #8648874 - Flags: review?(ferjmoreno) → review+
Can we get QA verification that Safe Mode startup works of foxfood builds? You should be able to hold the power button while the device is booting to enable it, which gives you a dialog to disable all running add-ons.
Keywords: verifyme
I can confirm that if a user launches "Safe mode" in a portrait mode everything works fine, however if a user launches "Safe mode" in a landscape mode then Bug 1203219.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmercado)
Device: Aries 2.5
Build ID: 20150908152248
Gaia: b81185d30e548f782770b852473ffb53c641a490
Gecko: b23b2fa33a9dcda59dbbca1d157eca3c32c5b862
Gonk: 2916e2368074b5383c80bf5a0fba3fc83ba310bd
Version: 43.0a1 (2.5)
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:43.0) Gecko/43.0 Firefox/43.0
Keywords: verifyme
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmercado)
Depends on: 1215494
I can't trigger safe mode with the latest foxfooding build. Can QA re-confirm that this is still working?
Status: VERIFIED → RESOLVED
Closed: 4 years ago4 years ago
Keywords: qawanted, verifyme
I was able to trigger safe mode on today's dogfood debug build. But I was NOT able to trigger safe mode if I go to RC4 and then OTA to latest.

Able to enable Safe Mode on:
Device: Aries 2.5
BuildID: 20151022105033
Gaia: 29ce8ec8606e59f582375234440812b046346513
Gecko: 76bd0c01d72e64ca4f261ffdb2652a91f961e930
Gonk: 2916e2368074b5383c80bf5a0fba3fc83ba310bd
Version: 44.0a1 (2.5) 
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:44.0) Gecko/44.0 Firefox/44.0

NOT able to enable Safe Mode on:
Device: Aries 2.5 (RC4 > OTA to latest via default channel)
BuildID: 20151019104907
Gaia: f75bd584aca0a751a5bed115800250faa8412927
Gecko: d3e87bb40753327550143ba8ac8ee27b300cd4a9
Gonk: 2916e2368074b5383c80bf5a0fba3fc83ba310bd
Version: 44.0a1 (2.5) 
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:44.0) Gecko/44.0 Firefox/44.0

NI :mhenretty for this result and removing qawanted and verifyme.
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(mhenretty)
Flags: needinfo?(jmercado)
Keywords: qawanted, verifyme
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmercado)
(In reply to Pi Wei Cheng [:piwei] from comment #21)
> But I was
> NOT able to trigger safe mode if I go to RC4 and then OTA to latest.

Fabrice, any ideas on why this might be? I think I'm running into this same issue with my Foxfooding device. Perhaps there is a pref that foxfooders could be missing or something?
Flags: needinfo?(mhenretty) → needinfo?(fabrice)
On a fresh build I can't reproduce, so I'm not sure what's going on for you.
Flags: needinfo?(fabrice)
Depends on: 1227208
You need to log in before you can comment on or make changes to this bug.