Support loading app packages from multiple locations

RESOLVED FIXED in mozilla18

Status

Core Graveyard
DOM: Apps
RESOLVED FIXED
5 years ago
27 days ago

People

(Reporter: cjones, Assigned: fabrice)

Tracking

Trunk
mozilla18
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 5 obsolete attachments)

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.
(Assignee)

Updated

5 years ago
Assignee: nobody → fabrice
(Assignee)

Updated

5 years ago
blocking-basecamp: --- → ?
(Assignee)

Comment 1

5 years ago
Created attachment 647157 [details] [diff] [review]
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.
Attachment #647157 - Flags: review?(21)
(Assignee)

Comment 2

5 years ago
The matching PR for gaia is at https://github.com/mozilla-b2g/gaia/pull/2957
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.
(Assignee)

Comment 4

5 years ago
(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
(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;|
(Assignee)

Comment 6

5 years ago
Created attachment 649404 [details] [diff] [review]
patch v2

updated patch to address comments, and change the way we deal with local ids since they must be stable between restarts.
Attachment #647157 - Attachment is obsolete: true
Attachment #647157 - Flags: review?(21)
Attachment #649404 - Flags: review?(21)
blocking-basecamp: ? → +
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 ','
Attachment #649404 - Flags: review?(21) → review-
(Assignee)

Comment 8

5 years ago
Created attachment 651575 [details] [diff] [review]
patch v3

Addressing comments.

Try push at https://tbpl.mozilla.org/?tree=Try&rev=731276a76ec5
Attachment #649404 - Attachment is obsolete: true
Attachment #651575 - Flags: review?(21)
Attachment #651575 - Flags: review?(21) → review+
(Assignee)

Comment 9

5 years ago
Created attachment 656502 [details] [diff] [review]
rebased patch

No changes, just rebasing against current m-i
(Assignee)

Comment 10

5 years ago
Created attachment 656514 [details] [diff] [review]
rebased patch

Better with the last qref.
Attachment #656502 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
Created attachment 656532 [details] [diff] [review]
rebased patch

fixing a typo
Attachment #656514 - Attachment is obsolete: true
(Assignee)

Comment 12

5 years ago
Created attachment 656579 [details] [diff] [review]
rebased patch

Removing a forgotten debug log, and rebasing because Bug 768868 had some changes in Webapps.js
Attachment #656532 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2865d46e472d
Is there anything testable here from an end-user perspective? I'm seeing new exposed properties in the patch.
QA Contact: jsmith
Whiteboard: [qa?]
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
(Assignee)

Comment 16

5 years ago
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
(Assignee)

Comment 17

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cfd76ed711c
https://hg.mozilla.org/mozilla-central/rev/2cfd76ed711c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
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.
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

5 years ago
It's a v1 shortcut. For v2+ we should not jointly update gecko and gaia and add OEM paths for this.
Depends on: 787564

Updated

5 years ago
Whiteboard: [qa?]

Updated

5 years ago
Whiteboard: [qa-]

Updated

27 days ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.