1049308 - Figure out a way for certified/core apps to uninstall themselves

Status: RESOLVED INVALID

(Core Graveyard :: DOM: Apps, defect)

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

[Blocking Requested - why for this release]: Should be marked as feature.

Right now, core apps cannot be removed in any way due to a block in gecko (https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#592). However, the browser app will need to remove itself from the system once it is done moving data as part of bug 938171.

Best solution I've got right now is to allow certified/core apps to remove themselves. This would mean that we check the AppID of the app currently requesting the uninstall, and if it matches the app it's uninstalling, and that app is core, we ignore the "removable" check and go on with the uninstall.

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Talked to fabrice. The problem with letting core apps uninstall themselves is that on upgrade, they'll come right back, do their upgrade step, and uninstall themselves again. We'll need to add an app URI blacklist to Webapps.jsm that will stop the apps from reinstalling themselves on upgrade. Jank, but it should work. 

This does unfortunately mean that we will be zombie-ing along no longer used core apps until the end of time.

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Allow core apps to uninstall themselves

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v1) - Implement core apps reinstall blacklist

Comment 4

[[:fabrice] Fabrice Desré]

Comment on attachment 8470408 [details] [diff] [review]
Patch 1 (v1) - Allow core apps to uninstall themselves

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

::: dom/apps/src/Webapps.jsm
@@ +3661,4 @@
>      );
>    },
>  
> +  uninstall: function(aTargetPrincipal, aManifestURL, aOnSuccess, aOnFailure) {

that will break https://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webapps.js#714

@@ +3671,2 @@
>      let id = app.id;
> +    let target_manifest = this.getManifestURLByLocalId(aTargetPrincipal.appId);

nit: s/target_manifest/targetManifest

Comment 5

[[:fabrice] Fabrice Desré]

Comment on attachment 8470409 [details] [diff] [review]
Patch 2 (v1) - Implement core apps reinstall blacklist

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

I'd rather not hardcode that, but update a pref when uninstalling a core app, and here check that the manifestURL of the app is part of the "dom.mozApps.core-uninstalled" pref.

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

So what type should that pref be since we can't store lists in prefs? Just a space delimited string or something?

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v2) - Allow core apps to uninstall themselves

Fixed up devtools uninstall call, fixed nit

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v2) - Implement core apps reinstall blacklist

Taking a shot as using a space delimited string of manifest URLs for the blacklisting pref. Just r- if there's a better way to do this I'm not aware of.

Comment 9

[[:fabrice] Fabrice Desré]

Comment on attachment 8470460 [details] [diff] [review]
Patch 1 (v2) - Allow core apps to uninstall themselves

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

lgtm, but I'm not sure we're not dying in bad ways on device when we'll try to remove files from a read only partition. Did you check that? eg, are we calling the onuninstall handlers as expected?

Comment 10

[[:fabrice] Fabrice Desré]

Comment on attachment 8470461 [details] [diff] [review]
Patch 2 (v2) - Implement core apps reinstall blacklist

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

let's use a comma delimiter instead of space, that's what we do for other prefs (look at https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#3440 for instance).

::: dom/apps/src/Webapps.jsm
@@ +578,4 @@
>        let appDir = FileUtils.getDir("coreAppsDir", ["webapps"], false);
>        // c
>        for (let id in data) {
> +        let coreUninstalledPref = Services.prefs.getCharPref("dom.mozApps.core-uninstalled");

that will throw if the pref is not set.

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Fabrice Desré [:fabrice] from comment #9)
> Did you check that? eg, are we
> calling the onuninstall handlers as expected?

Everything seemed ok on device, but I'll keep a closer eye on the debug messages just in case.

Comment 12

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v3) - Implement core apps reinstall blacklist

Changed space delimiter to comma, remembered to actually add change to prefs all.js file this time.

Comment 13

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v3, Final) - Allow core apps to uninstall themselves

Had argument order in doUninstall wrong, meaning nothing ever would uninstall. And of course forgot to rebuild with the jsm in. >.<

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Since we're moving to doing IDB trickery, we no longer need this.