Closed Bug 791039 Opened 7 years ago Closed 7 years ago

Add support for 3rd party preinstalled apps to build system

Categories

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

All
Gonk (Firefox OS)
defect

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

RESOLVED FIXED
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: sicking, Assigned: ochameau)

References

Details

Attachments

(4 files, 3 obsolete files)

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.
The platform already has support for loading apps from /system and from /data. All we need here is on the gaia build system side.
OS: Mac OS X → Gonk
Hardware: x86 → ARM
Fabrice says we should close this and open a Gaia bug instead.  I did that with https://github.com/mozilla-b2g/gaia/issues/4748.
Status: NEW → RESOLVED
Closed: 7 years ago
OS: Gonk → Mac OS X
Hardware: ARM → x86
Resolution: --- → INVALID
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).
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Assignee: nobody → fabrice
OS: Mac OS X → Gonk
Hardware: x86 → All
Depends on: 796649
Assignee: fabrice → poirot.alex
(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?
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.
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.
Priority: -- → P1
Attached file 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.
Attachment #672840 - Flags: review?(21)
Attachment #672841 - Flags: review?(21)
Attachment #672847 - Flags: review?(21)
Attached patch Part 0 - fix typo in webapps.jsm (obsolete) — Splinter Review
Carry over r+ from bug Bug 803071.
Attachment #673018 - Flags: review+
Attachment #672840 - Attachment description: Factorize code executed after system app install. (`onAppsLoaded` function) → Part 1 - Factorize code executed after system app install. (`onAppsLoaded` function)
Attachment #672841 - Attachment description: Bug 791039: Fill webapp appcache from its local `cache` folder. → Part 2 - Fill webapp appcache from its local `cache` folder.
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.
Attachment #672840 - Flags: review?(21) → review+
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.
Attachment #672841 - Flags: review?(21) → review+
Comment on attachment 672847 [details]
Pull request 5885

r+ with nits fixed.
Attachment #672847 - Flags: review?(21) → review+
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?
(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.
Try seems mostly green, I don't think oranges are related to this change?
Keywords: checkin-needed
(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 :) ).
Duplicate of this bug: 804183
Blocks: 824949
Depends on: 1009531
You need to log in before you can comment on or make changes to this bug.