849988 - Implement support for a removable property for preinstalled apps to define if the app can be uninstalled or not

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Jason Smith [:jsmith]]

Currently, our preinstalled app framework does not support specifying a "removable" property in the metadata.json file to indicate if the app can be removed or not. There some back and forth debate on whether we should have the ability to support non-removable apps, but let's handle the actual definition of using it in a different bug. This bug focuses on implementing the support for a removable property to allow for an app to be uninstalled or not.

If no removable property is specified, then the default should be that the preinstalled app is removable.

Comment 2

[Joe Cheng [:jcheng] (please needinfo)]

partner is waiting on a patch for this. anyone can take this?

Comment 3

[Joe Cheng [:jcheng] (please needinfo)]

tef? as this is needed to make 3rd party apps removable by default

Comment 4

[Jason Smith [:jsmith]]

(In reply to Joe Cheng [:jcheng] from comment #2)
> partner is waiting on a patch for this. anyone can take this?

Probably someone who knows the build system piece for apps, potentially the core dom:apps pieces might a person to hunt for. Fabrice would be a good person to ask.

I was experimenting around with the source here:

https://github.com/mozilla-b2g/gaia/blob/master/build/webapp-manifests.js

Where I actually noticed the odd behavior that we:

1. Never read any property called removable from metadata.json
2. Default to removable to true on a packaged app and undefined on a hosted app

Comment 5

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

(tef- for now, as it's not clear why do we must support removing preinstalled apps?  how does that work with the device "Reset" functionality in settings for example?  For v1.0.1, I think we should treat all preloads as fixed just like the build-in core apps given that we are mostly out of time.)

Comment 6

[Jason Smith [:jsmith]]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #5)
> (tef- for now, as it's not clear why do we must support removing
> preinstalled apps? 

I believe this was part of the partner customization requirements for specific locale builds for any app that was not deemed core. For core apps, they would not be non-removable.

> how does that work with the device "Reset" functionality
> in settings for example?

Good question. I actually don't know this answer. Need info on product to clarify.

> For v1.0.1, I think we should treat all preloads
> as fixed just like the build-in core apps given that we are mostly out of
> time.)

Well, that's not the current behavior either. We currently do the following:

* If the app is a hosted preinstalled app, it's non-removable by default
* If the app is preinstalled packaged app, it's removable by default

Thoughts? The behavior is a tad awkward in it's current state, so we really do need to pick one or the other.

Comment 7

[[:fabrice] Fabrice Desré]

Attachment: gaia patch

Comment 8

[[:fabrice] Fabrice Desré]

Attachment: gecko patch

On the gecko side, we remove any use of the 'removable' property as an indicator of a non-system app. Instead, we check the basePath and compare it to the core apps path. I also removed the hardcoded "removable=true" when preinstalling 3rd party packaged apps. Gaia now sets the removable flag.

On the gaia side, the metadata.json now supports a "removable" field that defaults to true. I turned it to false for the maps app, and this will haunt me for the next 100 years.

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 725079 [details] [diff] [review]
gecko patch

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

Thanks Fabrice! Looks good. r=me

::: dom/apps/src/Webapps.jsm
@@ +1994,5 @@
>        return;
>      }
>  
>      // the manifest file used to be named manifest.json, so fallback on this.
> +    //let baseDir = (this.webapps[id].removable ? DIRECTORY_NAME : "coreAppsDir");

Remove the comment, please

Comment 10

[Joe Cheng [:jcheng] (please needinfo)]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #5)
> (tef- for now, as it's not clear why do we must support removing
> preinstalled apps?  how does that work with the device "Reset" functionality
> in settings for example?  For v1.0.1, I think we should treat all preloads
> as fixed just like the build-in core apps given that we are mostly out of
> time.)

from Bug 840891 comment 53 and on, you should see more background on it. Basically all preloaded 3rd party apps are expected to be removed by end users

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 725076 [details] [diff] [review]
gaia patch

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

::: build/webapp-manifests.js
@@ +133,5 @@
>      webapp.manifestFile.copyTo(webappTargetDir, 'manifest.webapp');
>      origin = webapp.metaData.origin;
> +    installOrigin = webapp.metaData.installOrigin || webapp.metaData.origin;
> +    manifestURL = webapp.metaData.manifestURL ||
> +                    (webapp.metaData.origin + 'manifest.webapp');

nit: alignment

Comment 12

[[:fabrice] Fabrice Desré]

Gaia part pushed to master: https://github.com/mozilla-b2g/gaia/commit/718b76a95d2bc8ecf928d56a308b36cd8b69b3bc

Don't mark as fixed until I land the gecko patch (inbound is closed).

Comment 13

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

(In reply to Joe Cheng [:jcheng] from comment #10)
> from Bug 840891 comment 53 and on, you should see more background on it.

I can't access this bug.

Comment 14

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/298a598dde74

Comment 15

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/16ac88f4d5b6 for timing out test_bug_779982.html.

Comment 16

[Phil Ringnalda (:philor)]

And also that bunch of localstorage tests in mochitest-chrome that always bust when webapp tests are busted.

Comment 17

[[:fabrice] Fabrice Desré]

Pushed to try with a small fix to make sure I cover all non-b2g platforms:
https://tbpl.mozilla.org/?tree=Try&rev=71e5fcee49cf

Comment 18

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a63b7a4c03f8

Comment 19

[Ben Francis [:benfrancis]]

This commit rendered the whole of Gaia unbootable for me. Can we back it out?

Comment 20

[Jason Smith [:jsmith]]

(In reply to Ben Francis [:benfrancis] from comment #19)
> This commit rendered the whole of Gaia unbootable for me. Can we back it out?

Yes, let's back out then.

Comment 21

[Jason Smith [:jsmith]]

Need info on fabrice to address comment 19.

Comment 22

[Ben Francis [:benfrancis]]

I have backed out the gaia part in:
https://github.com/mozilla-b2g/gaia/commit/532a60eabbf3a97f0df6a274614dec3d2d0d8ec7

because it caused the regression reported in https://groups.google.com/d/msg/mozilla.dev.gaia/C1Cqw4Z0CLE/9McijzGUgWAJ which causes gaia to fail to start up.

It seems that |make production| still worked, but a |make install-gaia| or |make reset-gaia| would result in a gaia which couldn't be started.

Comment 23

[[:fabrice] Fabrice Desré]

That broke the eng builds. I'm looking at what's happening for those. My bad for testing only with a production setting.

Comment 24

[Andreas Gal :gal]

Why was this landed without a test? Please back out the Gecko part and re-land with a test.

Comment 25

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

(semi-blind tef+ purely based on comment 3, as I can't see the full justification as per comment 13)

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a63b7a4c03f8

Comment 27

[Jason Smith [:jsmith]]

Reopening for the fact the Gaia part was backed out.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/b6be9a1ac001
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/65e03cab8c3d

Comment 29

[Jason Smith [:jsmith]]

Setting status-firefox22 back to affected - we've only landed the gecko changes here. The Gaia changes was backed out.

Comment 30

[Andrew Overholt [:overholt]]

Fabrice, the gaia patch was backed out.  Do you have time to investigate or should we find someone else to take it from here?

Comment 31

[[:fabrice] Fabrice Desré]

I need to re-test now that the gecko patch has been uplifted, and reland.

Comment 32

[[:fabrice] Fabrice Desré]

Re-landed: https://github.com/mozilla-b2g/gaia/commit/99267614620b8c09a82711d4ded14a87211d263b

Comment 33

[amiller]

Does not make sense to create a regression issue.

Comment 34

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted commit 99267614620b8c09a82711d4ded14a87211d263b as:
v1-train: deabd66c2c5b555d83795284bc372b1c77af413f
v1.0.1: 4b3b2b98ca0659dbc9883f3675215e0b0c6c4a0f

Comment 35

[Cody Roesch [:croesch]]

Can you please provide steps to verify this fix - as we will blackbox test from the UI?

Comment 36

[Jason Smith [:jsmith]]

(In reply to croesch from comment #35)
> Can you please provide steps to verify this fix - as we will blackbox test
> from the UI?

The maps app should be by default be non-removable as a result of this patch. Already checked and it is okay. That's one blackbox flow.

There's also more testing at the build system level, but that's on my end. I'd ignore this bug for your testing you guys are doing.

Comment 37

[Darren]

Being left to Jason Smith per Comment 36.

Comment 38

[Jason Smith [:jsmith]]

Verified sanity-wise by seeing the build generated for maps that is non-removable and not being able to uninstall it.