790393 - "Factory reset"

Status: RESOLVED FIXED

(Core :: General, defect, P1)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

More precisely, "blowing away all user-customized data", aka reformat /data and /cache partitions.  (Oy, how did this not get filed ...)

I suggest we make this easy on ourselves and implement this by repurposing FOTA glue. What we could do is
 - package with gecko a dummy update package that only includes a script to reformat /data and /cache
 - for factory reset, pretend like we downloaded that package and go through normal FOTA application process to apply it.

This is a v1 product requirement.

Comment 1

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

Check out the section starting at line 81:  https://android.googlesource.com/platform/bootable/recovery/+/fadc5ac81d6400ebdd041f7d4ea64021596d6b7d/recovery.c#81

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Fabulous!  Thanks for the save.

Gene, Shian-Yow --- ignore our discussion earlier.  This is ready for you to take.

Comment 3

[Kai-Chih Hu [:khu]]

Based on the source codes provided by Michael: 
 * FACTORY RESET
 * 1. user selects "factory reset"
 * 2. main system writes "--wipe_data" to /cache/recovery/command
 * 3. main system reboots into recovery
 * 4. get_args() writes BCB with "boot-recovery" and "--wipe_data"
 *    -- after this, rebooting will restart the erase --
 * 5. erase_volume() reformats /data
 * 6. erase_volume() reformats /cache
 * 7. finish_recovery() erases BCB
 *    -- after this, rebooting will restart the main system --
 * 8. main() calls reboot() to boot main system
it looks like that it only wipe out the data without doing the rollback for firmware part. Is this the scope for the 'factory reset' we're talking about here? Just want to double confirm. Thanks!

Comment 4

[Kai-Chih Hu [:khu]]

By the way, because Gene has some tasks on hand, after talking to Gene, Shian-yow, Thinker and Steven, we agreed to assign this case to Steven Lee.

Comment 5

[Dietrich Ayala (:dietrich)]

Corresponding Gaia issue for front-end work. https://github.com/mozilla-b2g/gaia/issues/1529

Comment 6

[StevenLee[:slee]]

Hi all,

After discussed with schien and Thinker, we want to implement this feature as following.
System app sends "reset-factory" through mozContentEvent. shell.js gets this event and 
calls the external executable that is in comment 1 to reset the device.
How do you think?

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The UX designs have the settings app controlling factory reset.  It can't send content events.  We need to flip a setting to trigger this.  Gecko needs to use edge triggering on a settings change like reset.factory.schedule=true.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #7)
> The UX designs have the settings app controlling factory reset.  It can't
> send content events.  We need to flip a setting to trigger this.  Gecko
> needs to use edge triggering on a settings change like
> reset.factory.schedule=true.

And the triggering magic for such a thing usually lives in http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/settings.js#84

Comment 9

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

@cjones & vingtetun: Thanks for your suggestion. Steven and I will use settings API to implement Factory reset, but we have some concern about malicious access to settings. I'm not sure if we can restrict the accessibility of |reset.factory.schedule|, because I didn't find the permission checking mechanism for settings API. Do you have any suggestion about this part?

Comment 10

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: WIP - create RecoveryService that delegate factory reset request to external shared library

allow Gaia to initiate factory reset via settings |factory.reset.scheduled|, accept a string type of configuration.
RecoveryService will pass this configuration to librecovery.so, which will trigger Android factory reset procedure.

p.s. the implementation of librecovery.so will be provide by Steven Lee.

Comment 11

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: template for librecovery

template for librecovery.

Comment 12

[StevenLee[:slee]]

Attachment: Factory reset

This code will be put in gonk. When settings app calls |recovery("--wipe_data")|, 
it records the reset command to /cache/recovery/command then reboot. After 
rebooting, |recovery| process will be invoked and do factory reset.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 662006 [details] [diff] [review]
WIP - create RecoveryService that delegate factory reset request to external shared library

Please make sure to check the "patch" checkbox when you submit these to bugzilla.  Bugzilla isn't smart enough to figure that out that these files are patches, sadly :).

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 662100 [details] [diff] [review]
Factory reset

I'm in favor of this approach.  In the future, we'll need to deal with OEM-specific code here (sadly!), and I think this is a reasonable approach.

I don't know the android build system very well though, so f? to mwu on those parts.

Note, we'll need to be more careful about fsyncing the command file and syncing the file system before rebooting, but we can cover that in review.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 662006 [details] [diff] [review]
WIP - create RecoveryService that delegate factory reset request to external shared library

This approach looks good to me too.  I think we can repurpose this for the glue code we need to apply FOTA updates.

But I'm ok with this *ONLY IF* we can separate settings-read/settings-write permissions, and *ONLY IF* we're only handing out settings-write to the certified settings app.  f? to sicking for that.

f? to marshall to see if he likes the way this fits into his FOTA-apply scheme.

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 662006 [details] [diff] [review]
WIP - create RecoveryService that delegate factory reset request to external shared library

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

I share cjones opinion, giving permission to all applications to wipe your phone is too much. Splitting read/write permission would be handy.

::: b2g/chrome/content/settings.js
@@ +115,5 @@
> +// ================= Reset Factory ================
> +SettingsListener.observe('factory.reset.scheduled', '', function(value) {
> +  let recoveryService = Components.classes['@mozilla.org/recovery-service;1']
> +                          .getService(Components.interfaces.nsIRecoveryService);
> +  recoveryService.recovery(value);

Components.classes -> Cc
Components.interfaces -> Ci

::: b2g/components/RecoveryService.js
@@ +37,5 @@
> +    classDescription: 'B2G Recovery Service'
> +  }),
> +
> +  recovery: function recovery(params) {
> +    // load b2grecovery.so

The jsctypes code above says that you're loading librecovery.so, not b2grecovery.so

@@ +52,5 @@
> +    dump('-*- RecoveryService component: ' + s + '\n');
> +  };
> +} else {
> +  debug = function (s) {};
> +}

This code has always made me crazy. What about:

function debug(str) {
  // dump('-*- RecoveryService component: ' + str + '\n');
}

::: b2g/components/b2g.idl
@@ +42,5 @@
> +
> +[scriptable, uuid(acb93ff8-aa6d-4bc8-bedd-2a6a3b802a74)]
> +interface nsIRecoveryService : nsISupports
> +{
> +  void recovery(in jsval params);

I would like to see a list of parameters that can be passed to the recovery service.

Comment 17

[Marshall Culpepper [:marshall_law]]

Comment on attachment 662006 [details] [diff] [review]
WIP - create RecoveryService that delegate factory reset request to external shared library

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

Hrm, I have mixed feelings on this approach. I agree with the general concern over settings permissions, but see more below..

::: b2g/components/RecoveryService.js
@@ +2,5 @@
> +/* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */
> +/* 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/. */
> +'use stricts';

'use stricts' -> 'use strict'

@@ +20,5 @@
> +  return {
> +    recovery: library.declare('recovery',
> +                              ctypes.default_abi,
> +                              ctypes.void_t,
> +                              ctypes.char.ptr)

I like the idea of separating this out into a service, but I'm not convinced we need to delegate to a separate shared library here. It seems like we could write to /cache/recovery/command easily from Gecko Javascript or C++ without needing a new AOSP component. Is there a concern that librecovery might have some device specific code, and need to be tagged in the b2g manifest?

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The good news is that we've been planning on splitting settings-read and settings-write.

The bad news I think we have a number of apps which are currently writing settings. For example I believe the alarm app writes to the volume setting so that it can ensure to sound an alarm even if the phone is put in mute. My impression is that other apps also have been writing settings, but I don't have any examples off the top of my head.

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Marshall Culpepper [:marshall_law] from comment #17)
> Comment on attachment 662006 [details] [diff] [review]
> WIP - create RecoveryService that delegate factory reset request to external
> shared library
> 
> I like the idea of separating this out into a service, but I'm not convinced
> we need to delegate to a separate shared library here. It seems like we
> could write to /cache/recovery/command easily from Gecko Javascript or C++
> without needing a new AOSP component. Is there a concern that librecovery
> might have some device specific code, and need to be tagged in the b2g
> manifest?

We're going to need to deal with vendor-specific code to do this.  That code shouldn't live in gecko.  See comment 14.

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Jonas Sicking (:sicking) from comment #18)
> The good news is that we've been planning on splitting settings-read and
> settings-write.
> 

This is a blocker, right?

> The bad news I think we have a number of apps which are currently writing
> settings. For example I believe the alarm app writes to the volume setting
> so that it can ensure to sound an alarm even if the phone is put in mute. My
> impression is that other apps also have been writing settings, but I don't
> have any examples off the top of my head.

Orthogonally, do you have an alternative suggestion than the approach of tweaking a setting?

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #20)
> This is a blocker, right?

For some definitions of "blocker", yes.

> Orthogonally, do you have an alternative suggestion than the approach of
> tweaking a setting?

It would be great if we could add per-<audio> stream controls over which hardware it's output through, and with which volume. Probably we'd only need the ability to direct a given <audio> to the built-in speakers with a reasonably high volume.

But I'm not sure if there are other cases where we write settings, so that might not cover everything.

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Jonas Sicking (:sicking) from comment #21)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #20)
> > This is a blocker, right?
> 
> For some definitions of "blocker", yes.
> 

I only know of one.

> > Orthogonally, do you have an alternative suggestion than the approach of
> > tweaking a setting?
> 
> It would be great if we could add per-<audio> stream controls over which
> hardware it's output through, and with which volume. Probably we'd only need
> the ability to direct a given <audio> to the built-in speakers with a
> reasonably high volume.
> 
> But I'm not sure if there are other cases where we write settings, so that
> might not cover everything.

It sounds like we need another approach then.  Do you have a suggestion?

Comment 23

[Marshall Culpepper [:marshall_law]]

Comment on attachment 662006 [details] [diff] [review]
WIP - create RecoveryService that delegate factory reset request to external shared library

(In reply to Chris Jones [:cjones] [:warhammer] from comment #19)
> We're going to need to deal with vendor-specific code to do this.  That code
> shouldn't live in gecko.  See comment 14.

My apologies then :) feedback+ under the assumption that the settings mechanism gets changed or secured somehow.

Comment 24

[Michael Wu [:mwu]]

Comment on attachment 662100 [details] [diff] [review]
Factory reset

Is this something that vendors would want to customize? If not, I would put this function into gecko/libxul since it should work the same in there.

Comment 25

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

As I said in comment 14 and comment 19, yes, vendors have their own solutions for this part of the stack.

Comment 26

[Michael Wu [:mwu]]

Comment on attachment 662100 [details] [diff] [review]
Factory reset

Well, fair enough then. This looks fine to me.

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #22)
> > > Orthogonally, do you have an alternative suggestion than the approach of
> > > tweaking a setting?
> > 
> > It would be great if we could add per-<audio> stream controls over which
> > hardware it's output through, and with which volume. Probably we'd only need
> > the ability to direct a given <audio> to the built-in speakers with a
> > reasonably high volume.
> > 
> > But I'm not sure if there are other cases where we write settings, so that
> > might not cover everything.
> 
> It sounds like we need another approach then.  Do you have a suggestion?

Not without knowing what other reasons we have for writing settings from other apps. There may be non, in which case the <audio> solution might be enough. I've never looked at the <audio> code though so I can't speak to how feasible adding that would be.

Comment 28

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Sorry, I meant

 "It sounds like we need another approach for factory reset.  Do you have a suggestion?"

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

No great ideas. One would be to add an explicit API for this and only hand out the permission to use that API out to the settings app.

Or add a function on the power API for factory reset and then make the system app monitor the "reset.factory.schedule" setting. If set it would pop up a dialog asking the user if he/she is sure and if the user presses "yes" call that function. We probably need a dialog anyway though I presume that the plan was for the setting app to show that dialog.


Adding the ability to do per-setting write permission would be hard right now since we modify the indexedDB database backing settings directly from the child process. So the parent process doesn't have any semantic understanding of what settings are changed.

We could check with Gregor what the plan is for doing the parent process checks for settings in general. Right now I think the indexedDB code will let any process modify the settings database.

Comment 30

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I kind of like the idea of a parameter to power.reboot(andThenWhat).  Then we could do power.reboot("factoryreset").  We only hand that permission out to the system app right now (IIRC), but could extend it to the settings app to satisfy UX requirements.

wdyk?

Comment 31

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 1 - RecoveryService implementation

update according to comment 16 and comment 17. The parameter of RecoveryService.recovery() represents the action that user want to do. I define two actions, "full" and "update".
"full" means both settings and user data will be removed. 
"update" means FOTA update.
librecovery.so can use this parameter to determine the actual procedure of factory reset and FOTA.

Comment 32

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Part 2 - Expose RecoveryService API via MozSettings

Just put the settings API approach in case we decide to use this one.
I think using power.reboot(andThenWhat) is ok if we cannot have stronger permission check on MozSettings in V1.

Comment 33

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 662860 [details] [diff] [review]
Part 1 - RecoveryService implementation

I'm not the best reviewer of this kind of code but this patch looks
mostly OK to me.

>diff --git a/b2g/components/Makefile.in b/b2g/components/Makefile.in
>--- a/b2g/components/Makefile.in +++ b/b2g/components/Makefile.in @@
>-21,15 +21,16 @@ EXTRA_PP_COMPONENTS = \ AlertsService.js \
>B2GComponents.manifest \ ContentHandler.js \
>ContentPermissionPrompt.js \ DirectoryProvider.js \ MozKeyboard.js \
>ProcessGlobal.js \ PaymentGlue.js \ + RecoveryService.js \

Desktop b2g builds will end up trying to run this code, right?  We
need to avoid that by conditional compilation, or provide a fallback
implementation of the service that's a no-op on desktop.

>diff --git a/b2g/components/b2g.idl b/b2g/components/b2g.idl

>+[scriptable, uuid(acb93ff8-aa6d-4bc8-bedd-2a6a3b802a74)] +interface
>nsIRecoveryService : nsISupports +{ + void recovery(in jsval params);

We don't need this generic of an API; we control the interface into
the C library.  I don't think that's going to help anyone.  Let's just
add a |void factoryReset()| method here.

r- because I think this will cause bad things to happen in b2g desktop 
builds (possibly security problems).

Comment 34

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 662864 [details] [diff] [review]
Part 2 - Expose RecoveryService API via MozSettings

This patch is perfectly fine as-is, but until we lock down settings-write permissions to just the system app (and maybe settings), we can't expose this interface; it's just too dangerous.

Clearing review request not because I think this patch itself has problems, but because we need to find a solid approach.  Let's continue to use this patch for testing :).

Comment 35

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Jonas/Gregor: per discussion above, just the split of settings-read/settings-write is a blocker.  What's the bug#?  Is it also a blocker to remove settings-write from everything but the System and Settings apps?

Comment 36

[Marshall Culpepper [:marshall_law]]

Comment on attachment 662860 [details] [diff] [review]
Part 1 - RecoveryService implementation

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

::: b2g/components/RecoveryService.js
@@ +42,5 @@
> +   */
> +  recovery: function recovery(params) {
> +    // load librecovery.so
> +    debug('recovery with params: ' + params);
> +    librecovery.recovery(params);

To echo cjones, the librecovery and RecoveryService APIs don't need to be this open ended. We should be careful and expose only the interface we need, maybe something along the lines of:

void factoryReset();
void otaInstall(string updatePackage)

Comment 37

[Marshall Culpepper [:marshall_law]]

Comment on attachment 662044 [details] [diff] [review]
template for librecovery

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

Looks mostly good :) Things will need to be shuffled around a bit, and we need to make sure we have a good API.

::: core/user_tags.mk
@@ +309,4 @@
>  	libprotobuf-java-2.3.0-lite \
>  	libprotobuf-java-2.3.0-micro \
>  	librecovery_ui_htc \
> +	librecovery \

GRANDFATHERED_USER_MODULES is actually meant to be for backwards compatibility with older versions of AOSP. Can you instead add this to build/target/product/b2g.mk?

::: librecovery/Android.mk
@@ +6,5 @@
> +LOCAL_MODULE:= librecovery
> +
> +LOCAL_SRC_FILES := recovery.c
> +
> +LOCAL_SHARED_LIBRARIES := libcutils

You'll also need to make sure to set the "optional" and "eng" AOSP build tags here:

LOCAL_MODULE_TAGS := optional eng

::: librecovery/recovery.c
@@ +1,4 @@
> +#include "stdio.h"
> +#include "cutils/log.h"
> +
> +void recovery(char *params)

We should restrict this API so we aren't beholden to the google recovery tool's command syntax for factory reset and FOTA updates. Also, see comment 36

Comment 38

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #35)
> Jonas/Gregor: per discussion above, just the split of
> settings-read/settings-write is a blocker.  What's the bug#?  Is it also a
> blocker to remove settings-write from everything but the System and Settings
> apps?

In order to display an 'alarm' icon in the system tray - the clock application write a setting that let the system now if there is an alarm set of not. It would have been nice to have that as a boolean value on the alarm API (mozAlam.hasAlarm or something like that) but you don't always want to show this alarm icon (for example an alarm from the calendar).

Comment 39

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Why can't the alarm API do this itself?

Comment 40

[Marshall Culpepper [:marshall_law]]

Comment on attachment 662100 [details] [diff] [review]
Factory reset

I've got an initial implementation of librecovery for both factory reset and OTA install on my github account here:

https://github.com/marshall/librecovery

This users a stricter API, and has OTA install has been tested succesfully.

Comment 41

[Marshall Culpepper [:marshall_law]]

Attachment: Part 1 - RecoveryService implementation - v2

Updated to match new API for librecovery, and only enables librecovery functions
for Gonk.

Graciously adopted from Shih-Chiang Chien's original code :)

Comment 42

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 663645 [details] [diff] [review]
Part 1 - RecoveryService implementation - v2

>diff --git a/b2g/components/RecoveryService.js b/b2g/components/RecoveryService.js

>+    otaInstall:   library.declare("otaInstall",

Nit: let's call this installFotaUpdate().

r=me with that.  I like how this turned out :).  Nice job,
Shih-Chiang, Steven and Marshall!

Please file a followup on making these functions asynchronous.  That's
more general and will allow gaia to present a better UX while we do
the (sometimes nontrivial) work needed to prepare for
update/factory-reset.  But that doesn't block this initial work.

Comment 43

[Marshall Culpepper [:marshall_law]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #42)
> Please file a followup on making these functions asynchronous.  That's
> more general and will allow gaia to present a better UX while we do
> the (sometimes nontrivial) work needed to prepare for
> update/factory-reset.  But that doesn't block this initial work.

I don't disagree, but the work these functions do is relatively quick (writing to a file, and issuing a reboot). 

Are you thinking we should separate out the final reboot into a separate call to give Gecko a chance to clean up more properly?

Comment 44

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Marshall Culpepper [:marshall_law] from comment #43)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #42)
> > Please file a followup on making these functions asynchronous.  That's
> > more general and will allow gaia to present a better UX while we do
> > the (sometimes nontrivial) work needed to prepare for
> > update/factory-reset.  But that doesn't block this initial work.
> 
> I don't disagree, but the work these functions do is relatively quick
> (writing to a file, and issuing a reboot). 
> 

That's not particularly quick in general :).

> Are you thinking we should separate out the final reboot into a separate
> call to give Gecko a chance to clean up more properly?

That's not what I had in mind, but that's a good idea!

Comment 45

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Do we have conclusion about how to expose API to Gaia? Maybe we could land the core service if we cannot finish the discussion before 9/28.

Comment 46

[Marshall Culpepper [:marshall_law]]

Comment on attachment 663645 [details] [diff] [review]
Part 1 - RecoveryService implementation - v2

I've moved RecoveryService over to Bug 794092, so we can land it separately.

Comment 47

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 662864 [details] [diff] [review]
Part 2 - Expose RecoveryService API via MozSettings

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

I would say this is ok to land as if so we can prototype on top of it. The read/write permission settings could be applied later once we have this ability. I would like to know a bug number about that though. So r+ with a bug number.

Comment 48

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 662864 [details] [diff] [review]
Part 2 - Expose RecoveryService API via MozSettings

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

We didn't want to do this based on the number of apps which have access to settings, right?

Comment 49

[StevenLee[:slee]]

(In reply to Jonas Sicking (:sicking) from comment #48)
> We didn't want to do this based on the number of apps which have access to
> settings, right?
I know this implementation is dangerous now. But as comment 35, I thought we need to file a bug that remove settings-write from everything but the System and Settings apps. Is that right?

Comment 50

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The current situation is that we have to hand out settings-write access to a number of applications, see comment 18.

It's unlikely that we'll be able to fix this in time for shipping.

So I thought the agreed upon plan was to expose the ability to do a factory reset through nsIDOMMozPowerManager. This API is currently only handed out to the system app. So we'd need to also hand it out to the settings app, but I think that's ok. And it's definitely better than handing it out to anyone which has settings-write permission.

Comment 51

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: proposed mozPower interface change

We only need to create a way for invoking factory reset since FOTA already uses mozChromeEvent according to 778349.

Comment 52

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 666486 [details] [diff] [review]
proposed mozPower interface change

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

::: dom/power/nsIDOMPowerManager.idl
@@ +14,4 @@
>  interface nsIDOMMozPowerManager : nsISupports
>  {
>      void    powerOff();
> +    void    reboot(in bool needFactoryReset);

Could be [optional].

Comment 53

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Expose RecoveryService API via MozPower.reboot()

I cannot include b2g.h in PowerManager.cpp because the dist/include/b2g.h is not existed while compiling dom related source code. Therefore, I use a RecoveryServiceDelegator to avoid this compile error.

A second thought: Should we use mozContentEvent for factory reset instead? We'll have a consistent interface for gaia to do reset and FOTA. Suppose FOTA and reset should have the same level of permission protection, right?

Comment 54

[Marshall Culpepper [:marshall_law]]

Comment on attachment 667874 [details] [diff] [review]
Expose RecoveryService API via MozPower.reboot()

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

Hrm, the RecoveryServiceDelegator seems unnecessary. I actually ran into the "dist/include/b2g.h missing" problem in another bug, and was able to fix it by tweaking the order of the build directories in toolkit/toolkit-tiers.mk:
https://mxr.mozilla.org/mozilla-central/source/toolkit/toolkit-tiers.mk#167

Try adding this above |tier_platform_dirs += ..|:

ifeq (gonk,$(MOZ_WIDGET_TOOLKIT))
# b2g headers need to be built before dom headers
tier_platform_dirs += b2g
endif

Comment 55

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

@Marshall: Wow, cool! Thanks for the advise. However, would it cause dependency loop if we have native code in b2g folder some day? It seems like a interface design flaw but I have no idea how to fix it.

Comment 56

[Marshall Culpepper [:marshall_law]]

(In reply to Shih-Chiang Chien from comment #55)
> @Marshall: Wow, cool! Thanks for the advise. However, would it cause
> dependency loop if we have native code in b2g folder some day? It seems like
> a interface design flaw but I have no idea how to fix it.

Hrm, good point. Can you file a follow up to separate out each interface in b2g.idl, and move it into dom/system/gonk? That might be the best place for these interfaces if we ever run into more DOM dependency problems in the future. 

Chris / Fabrice, do you guys have an opinion?

Comment 57

[[:fabrice] Fabrice Desré]

Will this be b2g only? if so, you can put your xpcom component in the b2g/ directory.

Comment 58

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 667874 [details] [diff] [review]
Expose RecoveryService API via MozPower.reboot()

I agree with Marshall, let's fix the underlying bug here.

Comment 59

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Shih-Chiang Chien from comment #53)
> Created attachment 667874 [details] [diff] [review]
> Expose RecoveryService API via MozPower.reboot()
> 
> A second thought: Should we use mozContentEvent for factory reset instead?
> We'll have a consistent interface for gaia to do reset and FOTA. Suppose
> FOTA and reset should have the same level of permission protection, right?

The UX spec has factory reset initiated from the settings app.  The settings app can't send those special events, and we don't want to allow it to.

Comment 60

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Expose RecoveryService API via MozPower.reboot(), v2

1. Update according to comment 54.
2. File a follow-up bug 798180 for refactory b2g.idl according to comment 54.

Comment 61

[Marshall Culpepper [:marshall_law]]

Comment on attachment 668308 [details] [diff] [review]
Expose RecoveryService API via MozPower.reboot(), v2

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

Looks good! Did you verify that this builds after a clobber too?

Comment 62

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Marshall Culpepper [:marshall_law] from comment #61)
> Comment on attachment 668308 [details] [diff] [review]
> Expose RecoveryService API via MozPower.reboot(), v2
> 
> Review of attachment 668308 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good! Did you verify that this builds after a clobber too?
I've verified this patch on both otoro and desktop build. Works fine! :-)

Comment 63

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 668308 [details] [diff] [review]
Expose RecoveryService API via MozPower.reboot(), v2

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

It does feel very scary that a simple 'true' completely clears all the data. I'd be somewhat partial to having an explicit factoryReset function. Sorry for not thinking about this earlier.

But I'm totally fine with this as-is too. So sr=me, but I'd also be fine with adding a separate MozPower.factoryReset() function.

Comment 64

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Expose RecoveryService API via MozPower.factoryReset(), v3

Update patch according to comment 63.
I think there is no significant semantic difference between |reboot(needReset)| and |factoryReset()|, but using an isolated |factoryReset()| function will make the code more readable.
I keep the sr+ according to Jonas's comment in comment 63.

Comment 65

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

You should update the iid on the MozPower interface. No need to ask for new reviews for that though.

Comment 66

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Expose RecoveryService API via MozPower.factoryReset(), v4

Update iid according to comment 65.
Now we are ready to check in this patch, many thanks to Chris, Jonas, Vivien, and Marshall!

Comment 67

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fc318454f2b

Any way to test this?

Comment 68

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Ryan VanderMeulen from comment #67)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/0fc318454f2b
> 
> Any way to test this?

I'm going to add test case for factory reset under librecovery, but I couldn't find a proper way to test a delegation function such as MozPower.factoryReset(). Do you guys have any suggestion?

Comment 69

[Daniel Holbert [:dholbert]]

philor backed this out, because it broke b2g builds:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/c2e44ae3a09d

The failure looks like this:
xpidl.IDLError: error: File 'domstubs.idl' not found, /builds/slave/m-in-ics-armv7a-gecko/build/b2g/components/b2g.idl line 5:0
https://tbpl.mozilla.org/php/getParsedLog.php?id=16181001&tree=Mozilla-Inbound

... which I suspect is from this chunk:
>+# b2g headers need to be built before dom headers
>+ifeq (gonk,$(MOZ_WIDGET_TOOLKIT))
>+tier_platform_dirs += b2g
>+endif

Comment 70

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Sorry I didn't find out the cyclic dependency earlier, which means I misunderstood comment 61. :(
I'm going to move the declaration of nsIRecoveryService to dom/power/nsIRecoveryService.idl and test more thoroughly.

Comment 71

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Previously, I tested mozPower.factoryReset() with system app and it works fine.
However, I encounter a process permission issue while I invoke factoryReset() in settings app.
  E/librecovery(  385): librecovery factoryReset()
  E/librecovery(  385): Unable to create recovery directory "/cache/recovery": Permission denied
  I/Gecko   (  385): -*- RecoveryService: Error: Factory reset failed
Looks like RecoveryService can only work in main thread. After discussing with @StevenLee and @RudyL, we might need to pass the function call all the way to Hal, just like what we did for mozPower.reboot(). 

Sorry about not finding out this problem earlierly.

Comment 72

[Mozilla RelEng Bot]

Try run for 668d9a2e879f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=668d9a2e879f
Results (out of 16 total builds):
    success: 16
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-668d9a2e879f

Comment 73

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Expose RecoveryService API via MozPower.factoryReset(), v5

update patch according to inbound build error and permission error described in comment 71
1. move the declaration of nsIRecoveryService to hal/gonk/nsIRecoveryService.idl
2. delegate MozPower.factoryReset() to hal::FactoryReset()
3. use ipc to invoke RecoveryService.factoryReset() in main thread.

note 1: keep sr+ since no modification on DOM interface.
note 2: I've push this patch to try server. https://tbpl.mozilla.org/?tree=Try&rev=379baec328c2

Comment 74

[Mozilla RelEng Bot]

Try run for 379baec328c2 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=379baec328c2
Results (out of 16 total builds):
    success: 15
    warnings: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-379baec328c2

Comment 75

[Marshall Culpepper [:marshall_law]]

Comment on attachment 672669 [details] [diff] [review]
Expose RecoveryService API via MozPower.factoryReset(), v5

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

Hrm, what was the thinking behind moving nsIRecoveryService into HAL? The update prompt and update service code also depends on nsIRecoveryService, so we should be really careful about moving it.

Comment 76

[Marshall Culpepper [:marshall_law]]

Comment on attachment 672669 [details] [diff] [review]
Expose RecoveryService API via MozPower.factoryReset(), v5

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

(In reply to Marshall Culpepper [:marshall_law] from comment #75)
> Hrm, what was the thinking behind moving nsIRecoveryService into HAL? 

Silly me, I didn't read the previous comments before providing feedback :)

I think the new location for nsIRecoveryService is probably OK as long as we make sure toolkit/mozapps/update/nsUpdateService.js and b2g/components/UpdatePrompt.js can access them as expected. I can help manually test this if needed.

::: dom/power/nsIDOMPowerManager.idl
@@ +9,5 @@
>  
>  /**
>   * This interface implements navigator.mozPower
>   */
> +[scriptable, uuid(7b181fef-2757-4198-89a0-8c426b8439ea)]

Will we need any special permission to update these UUIDs for aurora?

Comment 77

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 672669 [details] [diff] [review]
Expose RecoveryService API via MozPower.factoryReset(), v5


>diff --git a/dom/power/PowerManager.cpp b/dom/power/PowerManager.cpp

> NS_IMETHODIMP
>+PowerManager::FactoryReset()
>+{
>+#ifdef MOZ_WIDGET_GONK
>+  hal::FactoryReset();

No need to |ifdef| this.  That's the point of the hal:: API ;).

Comment 78

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Expose RecoveryService API via MozPower.factoryReset(), v6

update patch according to comment 77.
keep r+ and sr+ since no logic change on this new patch.
pass the test build on try server. https://tbpl.mozilla.org/?tree=Try&rev=c9ba6adfc4a4
I'm ready to raise the "check-in needed" flag again!

Comment 80

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ae8f8f7028f

Chris, can you provide some guidance on how to test this?

Comment 81

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

A test has already been written.

Comment 82

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9ae8f8f7028f

Comment 83

[Ryan VanderMeulen [:RyanVM]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #81)
> A test has already been written.

Great! Where is it?

https://hg.mozilla.org/releases/mozilla-aurora/rev/a1a85eefb329

Comment 84

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://github.com/mozilla-b2g/librecovery/pull/1