Status

Firefox OS
General
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: fabrice, Assigned: fabrice)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
FxOS-S6 (04Sep)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.5+, firefox43 verified)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

2 years ago
Created attachment 8644767 [details] [diff] [review]
Part 1, set an android property

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 2

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

Comment 3

2 years ago
Created attachment 8646562 [details] [diff] [review]
Part 1, set an android property v2

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)
(Assignee)

Comment 4

2 years ago
Created attachment 8646565 [details] [diff] [review]
Part 2, shell.js hookup

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 5

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

Comment 6

2 years ago
(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.
Blocks: 1193737
(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)

Comment 8

2 years ago
(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?
(Assignee)

Comment 9

2 years ago
(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

Updated

2 years ago
feature-b2g: --- → 2.5+
(Assignee)

Comment 10

2 years ago
Created attachment 8648873 [details] [diff] [review]
Part 1, set a pref during startup

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)
(Assignee)

Comment 11

2 years ago
Created attachment 8648874 [details] [diff] [review]
Part 2, shell.js hookup

Updated to match the pref change.
Attachment #8646565 - Attachment is obsolete: true
Attachment #8646565 - Flags: review?(21)
Attachment #8648874 - Flags: review?(21)

Comment 12

2 years ago
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+
(Assignee)

Comment 13

2 years ago
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)

Updated

2 years ago
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+

Comment 15

2 years ago
https://hg.mozilla.org/integration/b2g-inbound/rev/7e937f8865ec
https://hg.mozilla.org/integration/b2g-inbound/rev/21d807284999
https://hg.mozilla.org/mozilla-central/rev/7e937f8865ec
https://hg.mozilla.org/mozilla-central/rev/21d807284999
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
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+]
status-firefox43: fixed → verified
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
Last Resolved: 2 years ago2 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)
(Assignee)

Comment 23

2 years ago
On a fresh build I can't reproduce, so I'm not sure what's going on for you.
Flags: needinfo?(fabrice)
Depends on: 1225050

Updated

2 years ago
Depends on: 1227208
Depends on: 1230397
You need to log in before you can comment on or make changes to this bug.