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.

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

Fabulous! Thanks for the save. Gene, Shian-Yow --- ignore our discussion earlier. This is ready for you to take.

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!

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.

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

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?

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.

(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

@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?

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.

template for librecovery.

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 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 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 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 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 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?

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.

(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.

(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?

(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.

(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 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 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.

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

Comment on attachment 662100 [details] [diff] [review] Factory reset Well, fair enough then. This looks fine to me.

(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.

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

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.

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?

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.

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 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 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 :).

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 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 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

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

Why can't the alarm API do this itself?

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.

Updated to match new API for librecovery, and only enables librecovery functions for Gonk. Graciously adopted from Shih-Chiang Chien's original code :)

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.

(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?

(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!

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 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 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 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?

(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?

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.

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

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].

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 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

@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.

(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?

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

Comment on attachment 667874 [details] [diff] [review] Expose RecoveryService API via MozPower.reboot() I agree with Marshall, let's fix the underlying bug here.

(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.

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

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?

(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 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.

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.

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

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fc318454f2b Any way to test this?

(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?

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

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.

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.

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/schien@mozilla.com-668d9a2e879f

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

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/schien@mozilla.com-379baec328c2

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 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 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 ;).

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!

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ae8f8f7028f Chris, can you provide some guidance on how to test this?

A test has already been written.

(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