791039 - Add support for 3rd party preinstalled apps to build system

Status: RESOLVED FIXED

(Firefox OS Graveyard :: GonkIntegration, defect, P1)

Description

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

Right now our buildsystem can only create preinstalled apps that are uninstallable and which are stored in the /system directory.

We need to enable it to create preinstalled 3rd party apps which can be uninstalled by the user. Ideally these apps would be built to the /data directory, though technically that isn't required as long as they can be uninstalled, and they don't reappear on system update.

I suspect that we don't need a whole lot of platform work here, mostly just support for the buildsystem to preinstall these apps at build time.

Comment 1

[[:fabrice] Fabrice Desré]

The platform already has support for loading apps from /system and from /data. All we need here is on the gaia build system side.

Comment 2

[Andrew Overholt [:overholt]]

Fabrice says we should close this and open a Gaia bug instead.  I did that with https://github.com/mozilla-b2g/gaia/issues/4748.

Comment 3

[[:fabrice] Fabrice Desré]

We have the following requirements for preloaded 3rd party apps:

- These apps come preloaded, optionally with an app-cache. They are
considered as hosted apps.
- They can be upgraded independently of gecko+gaia updates.
- On a factory reset, they should survive (and ideally their app-cache
also).
- On a gaia+gecko update, these apps may be downgraded if the version in
the gecko+gaia update is not the latest one.

We must first change the build system to flash these apps in
/system/b2g/external_apps, with a directory structure such as:

/system/b2g/external_apps/webapps.json (includes the origin, etc.)
                         /marketplace/manifest.webapp
                         /marketplace/cache.manifest
                         /marketplace/cache/index.html
                         /marketplace/cache/index.css

At first run (including a factory reset) or after gaia+gecko update, we
run the following steps for each 3rd party app:
- copy $APP/manifest.webapp to /data/local/webapps/$APP/manifest.webapp
- add the entry for this app from webapps.json to the app registry.
- set the permissions for this app.
- load the app-cache if any, using the ressources in the cache
subdirectory (eg. reusing the code currently used in the gaia build system).

Comment 4

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

(In reply to Fabrice Desré [:fabrice] from comment #3)
> We have the following requirements for preloaded 3rd party apps:
> 
> - These apps come preloaded, optionally with an app-cache. They are
> considered as hosted apps.
> - They can be upgraded independently of gecko+gaia updates.
> - On a factory reset, they should survive (and ideally their app-cache
> also).
> - On a gaia+gecko update, these apps may be downgraded if the version in
> the gecko+gaia update is not the latest one.
>

I believe this is a strong requirement? This create the needs for the external-apps directory in system which is not really elegant.
 
> We must first change the build system to flash these apps in
> /system/b2g/external_apps, with a directory structure such as:
> 
> /system/b2g/external_apps/webapps.json (includes the origin, etc.)
>                          /marketplace/manifest.webapp
>                          /marketplace/cache.manifest
>                          /marketplace/cache/index.html
>                          /marketplace/cache/index.css
> 
> At first run (including a factory reset) or after gaia+gecko update, we
> run the following steps for each 3rd party app:
> - copy $APP/manifest.webapp to /data/local/webapps/$APP/manifest.webapp

This step can be done by Gaia. The manifest.webapp file will live in the userdata.img and will be by default on firstrun or every factory reset.

> - add the entry for this app from webapps.json to the app registry.
> - set the permissions for this app.
> - load the app-cache if any, using the ressources in the cache
> subdirectory (eg. reusing the code currently used in the gaia build system).

I wish those files could have live in /data/local/webapps/$APP/ instead of /system/b2g/externall-apps/ and break the ability to update them with Gecko (or downgrade them). But this is not an option right?

Comment 5

[[:fabrice] Fabrice Desré]

We can't flash these apps to /data/local since this partition is wiped when doing a factory reset. We should not flash ANYTHING on /data actually.

Comment 6

[Alexandre Poirot [:ochameau]]

What vivien meant is to put them in userdata.img. That's what is currently done when not using PRODUCTION flag. This image file is used during factory reset in order to reset /data partition with its content.

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: Part 1 - Factorize code executed after system app install. (`onAppsLoaded` function)

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: Part 2 - Fill webapp appcache from its local `cache` folder.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 5885

So. In this Gaia pull request, I ship the `cache` folder into the webapp device directory, next to its manifest.webapp file.
And in the two previous platform patches, I read this cache folder, search for the /cache/manifest.appcache file, parse it and automatically inject all cache entry into the appcache service.

It is slightly different from what has been described in comment 3.
The cache folder can contain cache entry for multiple domains.
So that the layout will look like this:
../apps/webapps.json (includes the origin, etc.)
       /marketplace/manifest.webapp
       /marketplace/cache/manifest.appcache
       /marketplace/cache/marketplace.mozilla.org/index.html
       /marketplace/cache/marketplace.mozilla.org/index.css
       /marketplace/cache/persona.mozilla.org/include.js

The other sensible different is that I haven't created a dedicated folder for external apps on device (yet). If it really is usefull, I'd prefer doing it in a followup patch.

Having said that I only have weak opinion on what we should do. I'm focused on getting stuff done. So suggestions are welcomed.

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: Part 0 - fix typo in webapps.jsm

Carry over r+ from bug Bug 803071.

Comment 11

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

Comment on attachment 672840 [details] [diff] [review]
Part 1 - Factorize code executed after system app install. (`onAppsLoaded` function)

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

::: dom/apps/src/Webapps.jsm
@@ +177,5 @@
> +    try {
> +      file = FileUtils.getFile("coreAppsDir", ["webapps", "webapps.json"], false);
> +    } catch(e) { }
> +
> +    if (file && file.exists) {

Don't forget to merge that with you typo fix.

Comment 12

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

Comment on attachment 672841 [details] [diff] [review]
Part 2 - Fill webapp appcache from its local `cache` folder.

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

R+ with nits fixed.

::: dom/apps/src/OfflineCacheInstaller.jsm
@@ +6,5 @@
> +
> +const Cu = Components.utils;
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cr = Components.results;

nit: Cr is unused.

@@ +98,5 @@
> +const OfflineCacheInstaller = {
> +  installCache: function installCache(app) {
> +    let cacheDir = Cc["@mozilla.org/file/local;1"]
> +                     .createInstance(Ci.nsIFile);
> +    cacheDir.initWithPath(app.basePath);

You may want to create a new File constructor here with CC.

@@ +102,5 @@
> +    cacheDir.initWithPath(app.basePath);
> +    cacheDir.append(app.appId);
> +    cacheDir.append("cache");
> +    if (!cacheDir.exists())
> +      return;

nit: add a line break

@@ +118,5 @@
> +		// a browser element, but a mozbrowser iframe.
> +	  // See netwerk/cache/nsDiskCacheDeviceSQL.cpp: AppendJARIdentifier
> +		let groupID = appcacheURL + '#' + app.localId+ '+f';
> +		let applicationCache = applicationCacheService.createApplicationCache(groupID);
> +		applicationCache.activate();

nit: indets seems broken here :)

@@ +146,5 @@
> +        else if (line.substr(0, 4) == 'http')
> +          urls.push(line);
> +        else
> +          throw new Error('Invalid line in appcache manifest:\n' + line +
> +                          '\nFrom: ' + cacheManifest.path);

Use brackets for if/else if/else. Mozilla guidelines are to not use them only if this is a if one liner.

Comment 13

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

Comment on attachment 672847 [details]
Pull request 5885

r+ with nits fixed.

Comment 14

[[:fabrice] Fabrice Desré]

Comment on attachment 672841 [details] [diff] [review]
Part 2 - Fill webapp appcache from its local `cache` folder.

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

::: dom/apps/src/OfflineCacheInstaller.jsm
@@ +35,5 @@
> +  let originURI = Services.io.newURI(origin, null, null);
> +  let principal = Services.scriptSecurityManager.getAppCodebasePrincipal(
> +                    originURI, appId, false);
> +  Services.perms.addFromPrincipal(principal, 'offline-app',
> +                                  Ci.nsIPermissionManager.ALLOW_ACTION);

You also need to add the "pin-app" permission to prevent these caches to be evicted.

@@ +47,5 @@
> +    onCacheEntryAvailable: function (cacheEntry, accessGranted, status) {
> +      cacheEntry.setMetaDataElement('request-method', 'GET');
> +      cacheEntry.setMetaDataElement('response-head', 'HTTP/1.1 200 OK\r\n');
> +      // Force an update. the default expiration time is way too far in the future:
> +      //cacheEntry.setExpirationTime(0);

Will this line stay commented?

Comment 15

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

(In reply to Fabrice Desré [:fabrice] from comment #14)
> Comment on attachment 672841 [details] [diff] [review]
> Part 2 - Fill webapp appcache from its local `cache` folder.
> 
> Review of attachment 672841 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/src/OfflineCacheInstaller.jsm
> @@ +35,5 @@
> > +  let originURI = Services.io.newURI(origin, null, null);
> > +  let principal = Services.scriptSecurityManager.getAppCodebasePrincipal(
> > +                    originURI, appId, false);
> > +  Services.perms.addFromPrincipal(principal, 'offline-app',
> > +                                  Ci.nsIPermissionManager.ALLOW_ACTION);
> 
> You also need to add the "pin-app" permission to prevent these caches to be
> evicted.

Oups. I forgot that. Thanks for pointing it out.

Comment 16

[Alexandre Poirot [:ochameau]]

Attachment: Bug 791039 part 0: Fix typo in Webapps.jsm

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: Bug 791039 part 1: Factorize code executed after system app install. (`onAppsLoaded` function)

Comment 18

[Alexandre Poirot [:ochameau]]

Attachment: Bug 791039 part 2: Fill webapp appcache from its local `cache` folder.

Comment 19

[Alexandre Poirot [:ochameau]]

https://tbpl.mozilla.org/?tree=Try&rev=b20838be942a

Comment 20

[Alexandre Poirot [:ochameau]]

Try seems mostly green, I don't think oranges are related to this change?

Comment 21

[[:fabrice] Fabrice Desré]

(In reply to Alexandre Poirot (:ochameau) from comment #20)
> Try seems mostly green, I don't think oranges are related to this change?

Yep. Note that you can retrigger failing tests if you want to be extra sure (not needed here, let's save some IT resources :) ).

Comment 22

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb90e14a89f7
https://hg.mozilla.org/integration/mozilla-inbound/rev/04176aaf8ee4
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cce74c60214

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fb90e14a89f7
https://hg.mozilla.org/mozilla-central/rev/04176aaf8ee4
https://hg.mozilla.org/mozilla-central/rev/5cce74c60214

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/30d6183f4dea
https://hg.mozilla.org/releases/mozilla-aurora/rev/c210cb087be3
https://hg.mozilla.org/releases/mozilla-aurora/rev/13a355eeeff8