778079 - Support loading app packages from multiple locations

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

To minimize risk, we're going to update gecko and gaia atomically for v1.  That's very easy wrt gecko updater; the gaia packages are just more blobs of bits to be moved and patched.

What we don't have support for currently is loading apps from multiple locations (/data/$profile/webapps and, let's call it /system/b2g/systemapps).  These seems like it should be pretty straightforward, we need an extra bit or two of metadata.

The packages in /system/b2g/systemapps will be read-only as well, not uninstallable, so maybe we should expose this bit to the app manager as well.  I think currently we have a hard-coded list.

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: Patch

This patch adds support for two apps sources:
- /system/b2g/webapps for "core" unremovable apps (only on b2g)
- the usual apps directory

The DOM Application object has now a new boolean property ("removable") which is false for all core apps.

New installed apps will be installed in the usual apps directory since they are forced to be removable.

Comment 2

[[:fabrice] Fabrice Desré]

The matching PR for gaia is at https://github.com/mozilla-b2g/gaia/pull/2957

Comment 3

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

Comment on attachment 647157 [details] [diff] [review]
Patch

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

::: b2g/components/DirectoryProvider.js
@@ -26,2 @@
>      if (localProps.indexOf(prop) != -1) {
> -      prop.persistent = true;

Why are you removing prop.persistent = true; ?

::: dom/apps/src/Webapps.js
@@ +231,4 @@
>    * mozIDOMApplication object
>    */
>  
> +function createApplicationObject(aWindow, aApp) {

Thanks!

::: dom/apps/src/Webapps.jsm
@@ +88,5 @@
> +            if (!this.webapps[id]) {
> +              this.webapps[id] = aData[id];
> +            }
> +          }
> +          //this.webapps = aData;

leftover ?

::: netwerk/protocol/app/AppProtocolHandler.js
@@ +40,3 @@
>      }
>  
> +    return this._basePath[aId];

basePath -> getBasePath and add a name to the anonymous function.

Comment 4

[[:fabrice] Fabrice Desré]

(In reply to Vivien Nicolas (:vingtetun) from comment #3)
> Comment on attachment 647157 [details] [diff] [review]
> Patch
> 
> Review of attachment 647157 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/components/DirectoryProvider.js
> @@ -26,2 @@
> >      if (localProps.indexOf(prop) != -1) {
> > -      prop.persistent = true;
> 
> Why are you removing prop.persistent = true; ?

Because it doesn't do anything? prop is just a string, no?


> ::: dom/apps/src/Webapps.jsm
> @@ +88,5 @@
> > +            if (!this.webapps[id]) {
> > +              this.webapps[id] = aData[id];
> > +            }
> > +          }
> > +          //this.webapps = aData;
> 
> leftover ?

yep!

> ::: netwerk/protocol/app/AppProtocolHandler.js
> @@ +40,3 @@
> >      }
> >  
> > +    return this._basePath[aId];
> 
> basePath -> getBasePath and add a name to the anonymous function.

sure

Comment 5

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

(In reply to Fabrice Desré [:fabrice] from comment #4)
> (In reply to Vivien Nicolas (:vingtetun) from comment #3)
> > Comment on attachment 647157 [details] [diff] [review]
> > Patch
> > 
> > Review of attachment 647157 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: b2g/components/DirectoryProvider.js
> > @@ -26,2 @@
> > >      if (localProps.indexOf(prop) != -1) {
> > > -      prop.persistent = true;
> > 
> > Why are you removing prop.persistent = true; ?
> 

Sigh. I think this is a typo that should be |persistent.value = true;|

Comment 6

[[:fabrice] Fabrice Desré]

Attachment: patch v2

updated patch to address comments, and change the way we deal with local ids since they must be stable between restarts.

Comment 7

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

Comment on attachment 649404 [details] [diff] [review]
patch v2

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

::: dom/apps/src/Webapps.jsm
@@ +73,5 @@
> +#ifdef MOZ_WIDGET_GONK
> +    dirList.push("coreAppsDir");
> +#endif
> +
> +    dirList.push(DIRECTORY_NAME);

nit:

let dirList = [DIRECTORY_NAME];

#ifdef MOZ_WIDGET_GONK
  dirList.push("coreappsDir");
#endif

@@ +79,5 @@
> +    let currentId = 1;
> +    for (let i in dirList) {
> +      let dir = dirList[i];
> +      let curFile = FileUtils.getFile(dir, ["webapps", "webapps.json"], true);
> +      if (curFile.exists()) {

dirList.forEach(function(dir) {
  let curFile = FileUtils.getFile(dir, ["webapps", "webapps.json"], true);
  if (!curFile.exists())
    return;

  ...
});

@@ +87,5 @@
> +          for (let id in aData) {
> +            if (!this.webapps[id]) {
> +              this.webapps[id] = aData[id];
> +            }
> +          }

Is it possible for 2 installed applications to have the same ID? If not you don't need the if.

@@ +88,5 @@
> +            if (!this.webapps[id]) {
> +              this.webapps[id] = aData[id];
> +            }
> +          }
> +          //this.webapps = aData;

leftover ?

@@ +89,5 @@
> +              this.webapps[id] = aData[id];
> +            }
> +          }
> +          //this.webapps = aData;
> +          for (let id in this.webapps) {

do you need to have a second loop here, can't you do all the logic in the first loop over aData?

@@ +91,5 @@
> +          }
> +          //this.webapps = aData;
> +          for (let id in this.webapps) {
> +            this.webapps[id].basePath = appDir.path;
> +            this.webapps[id].removable = dir == DIRECTORY_NAME;

nit: add () surrounding your expression, it makes it hard to read otherwise

@@ +98,3 @@
>  #endif
> +            // local ids must be stable between restarts.
> +            // We partition the ids in two buckets: 

nit: extra whitespace after buckets:

::: netwerk/protocol/app/AppProtocolHandler.js
@@ +34,5 @@
>  
> +  getBasePath: function app_phGetBasePath(aId) {
> +
> +    if (!this._basePath[aId]) {
> +      this._basePath[aId] = cpmm.sendSyncMessage("Webapps:GetBasePath", 

nit: extra whitespace after the last ','

Comment 8

[[:fabrice] Fabrice Desré]

Attachment: patch v3

Addressing comments.

Try push at https://tbpl.mozilla.org/?tree=Try&rev=731276a76ec5

Comment 9

[[:fabrice] Fabrice Desré]

Attachment: rebased patch

No changes, just rebasing against current m-i

Comment 10

[[:fabrice] Fabrice Desré]

Attachment: rebased patch

Better with the last qref.

Comment 11

[[:fabrice] Fabrice Desré]

Attachment: rebased patch

fixing a typo

Comment 12

[[:fabrice] Fabrice Desré]

Attachment: rebased patch

Removing a forgotten debug log, and rebasing because Bug 768868 had some changes in Webapps.js

Comment 13

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2865d46e472d

Comment 14

[Jason Smith [:jsmith]]

Is there anything testable here from an end-user perspective? I'm seeing new exposed properties in the patch.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

You know, when your Try push shows xpcshell failures on all platforms, maybe you should address those before pushing to inbound...

Backed out after it (shockingly) turned the xpcshell tests orange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd879b3ab5cd


https://tbpl.mozilla.org/php/getParsedLog.php?id=14819087&tree=Mozilla-Inbound

TEST-INFO | /Users/cltbld/talos-slave/test/build/xpcshell/tests/services/aitc/tests/unit/test_storage_registry.js | running test ...
TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/xpcshell/tests/services/aitc/tests/unit/test_storage_registry.js | test failed (with xpcshell return code: 0), see following log:
>>>>>>>

TEST-INFO | (xpcshell/head.js) | test 1 pending

TEST-INFO | (xpcshell/head.js) | test 2 pending

TEST-INFO | (xpcshell/head.js) | test 2 finished

TEST-INFO | (xpcshell/head.js) | running event loop

TEST-INFO | (xpcshell/head.js) | test 2 pending
TEST-INFO | /Users/cltbld/talos-slave/test/build/xpcshell/tests/services/aitc/tests/unit/test_storage_registry.js | Starting test_storage_install
Service.AITC.Storage	INFO	Server check got 2 apps
Service.AITC.Storage	INFO	Applying 2 installs to registry

TEST-INFO | (xpcshell/head.js) | test 2 finished
Services.Common.RESTRequest	DEBUG	GET http://localhost:8080/manifest.webapp 200
Services.Common.RESTRequest	DEBUG	GET http://localhost:8081/manifest.webapp 200
Service.AITC.Storage	WARN	_getManifest got invalid manifest: {"not":"a manifest","fullscreen":true}
Service.AITC.Storage	WARN	Could not fetch manifest for undefined
Service.AITC.Storage	DEBUG	1/2 apps processed
Service.AITC.Storage	DEBUG	2/2 apps processed

TEST-PASS | /Users/cltbld/talos-slave/test/build/xpcshell/tests/services/aitc/tests/unit/test_storage_registry.js | [null : 90] true == true

TEST-PASS | /Users/cltbld/talos-slave/test/build/xpcshell/tests/services/aitc/tests/unit/test_storage_registry.js | [null : 93] null == null
Service.AITC.Storage	INFO	Server check got 2 apps
Service.AITC.Storage	INFO	Applying 2 installs to registry
Services.Common.RESTRequest	DEBUG	GET http://localhost:8080/manifest.webapp 200
Services.Common.RESTRequest	DEBUG	GET http://localhost:8082/manifest.webapp 200
Service.AITC.Storage	DEBUG	1/2 apps processed
Service.AITC.Storage	DEBUG	2/2 apps processed

TEST-PASS | /Users/cltbld/talos-slave/test/build/xpcshell/tests/services/aitc/tests/unit/test_storage_registry.js | [null : 101] true == true

TEST-PASS | /Users/cltbld/talos-slave/test/build/xpcshell/tests/services/aitc/tests/unit/test_storage_registry.js | [null : 102] true == true

TEST-INFO | (xpcshell/head.js) | test 2 pending

TEST-INFO | (xpcshell/head.js) | test 2 finished

TEST-INFO | (xpcshell/head.js) | test 2 pending
TEST-INFO | /Users/cltbld/talos-slave/test/build/xpcshell/tests/services/aitc/tests/unit/test_storage_registry.js | Starting test_storage_uninstall
Ensure explicit uninstalls through hidden are honored.

TEST-PASS | /Users/cltbld/talos-slave/test/build/xpcshell/tests/services/aitc/tests/unit/test_storage_registry.js | [test_storage_uninstall : 110] {aea0e1bc-015d-2247-a6dd-1619dfe1abb5} != null
Service.AITC.Storage	INFO	Server check got 1 apps
Service.AITC.Storage	INFO	Uninstalling app: {aea0e1bc-015d-2247-a6dd-1619dfe1abb5}
Service.AITC.Storage	INFO	Uninstalling app: {e3b334d6-01c7-6044-a906-85ea74edfd6b}

TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/xpcshell/head.js | TypeError: this.webapps[aId] is undefined - See following stack:
JS frame :: /Users/cltbld/talos-slave/test/build/xpcshell/head.js :: do_throw :: line 451
JS frame :: /Users/cltbld/talos-slave/test/build/xpcshell/head.js :: _run_next_test :: line 891
JS frame :: /Users/cltbld/talos-slave/test/build/xpcshell/head.js :: <TOP_LEVEL> :: line 418

TEST-INFO | (xpcshell/head.js) | exiting test

TEST-INFO | (xpcshell/head.js) | test 2 finished
!!! error running onStopped callback: TypeError: callback is not a function
!!! error running onStopped callback: TypeError: callback is not a function
!!! error running onStopped callback: TypeError: callback is not a function
!!! error running onStopped callback: TypeError: callback is not a function
<<<<<<<


This was also causing mochitest-other failures.

https://tbpl.mozilla.org/php/getParsedLog.php?id=14819142&tree=Mozilla-Inbound

217 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | this should be an installed app - got 0, expected 1
222 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | this should be an installed app - got 0, expected 1
227 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | this should be an installed app - got 0, expected 1
232 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | this should be an installed app - got 0, expected 1
237 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | this should be an installed app - got 0, expected 1
282 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | this should be an installed app - got 0, expected 1
285 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | child should be an installed app - got 0, expected 1
291 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | this should be an installed app - got 0, expected 1
294 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | child should be an installed app - got 0, expected 1
300 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | this should be an installed app - got 0, expected 1
303 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | child should be an installed app - got 0, expected 1
313 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | child should be an installed app - got 0, expected 1
316 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | childPrincipal and parent principal should not have the same appStatus - didn't expect 0, but got it
319 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | this should be an installed app - got 0, expected 1
324 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | childPrincipal and parent principal should not have the same appStatus - didn't expect 0, but got it
331 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | child should be an installed app - got 0, expected 1
334 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | childPrincipal and parent principal should not have the same appStatus - didn't expect 0, but got it

Comment 16

[[:fabrice] Fabrice Desré]

I fixed the test failures and the try run looks green enough (I retriggered as many failing jobs I could).

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

Comment 17

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2cfd76ed711c

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2cfd76ed711c

Comment 19

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

Sorry for not chiming in earlier, I'm concerned that with how this currently stands there is no room for OEM differentiation in the core UI assuming they want to receive Mozilla's Gecko updates.  

At minimum we should consider adding support for a 2nd 'core app' location, say /system/vendor/b2g/webapps, for the non-Mozilla controlled non-removable apps.   Maybe the OEM wants to bundle another app or two.

Additionally I'm sure there will interest in a facility to optionally override the Mozilla controlled apps, say they want to do something a little different with the Homescreen/Lockscreen/Dialer/whatever.  Although maybe this safer as all-or-nothing.

As for updating the 2nd 'core app' location, the OEM could use FOTA although that's a pretty big hammer.  Maybe we can generalize the Gecko update downloader such that all the OEM needs to do is host their core app update package somewhere.

Most of this is probably out of scope for v1 but I see this being a rather serious limitation shortly thereafter.

Comment 20

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

Can you file a followup bug on that. I agree it's an important issue, but I'm not convinced that it's a basecamp blocker.

Comment 21

[Andreas Gal :gal]

It's a v1 shortcut. For v2+ we should not jointly update gecko and gaia and add OEM paths for this.