1042881 - Resolve manifest properties urls against the manifest url instead of the origin.

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[[:fabrice] Fabrice Desré]

Attachment: manifesthelper.patch

ManifestHelper() currently resolves paths against the app origin, but that won't work well with multiple apps from the same origin. We need these to be resolved against the manifest url.

Try run at https://tbpl.mozilla.org/?tree=Try&rev=8db0d4ada303

Comment 1

[Marco Castelluccio [:marco]]

Comment on attachment 8461053 [details] [diff] [review]
manifesthelper.patch

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

::: dom/apps/src/AppsUtils.jsm
@@ +594,5 @@
>  
>  /**
>   * Helper object to access manifest information with locale support
>   */
> +this.ManifestHelper = function(aManifest, aOrigin, aManifestURL) {

Instead of passing both the origin and the manifest URL, you could pass the
app object. This way in the following lines you can use the |kind| property
you're adding in another bug.
This would require callers to get the app object before creating a
ManifestHelper, but most of the callers already have an app object.

@@ +599,5 @@
> +  // If the app is packaged, we resolve uris against the origin.
> +  // If it's not, against the manifest url.
> +
> +  if (!aOrigin || !aManifestURL) {
> +    throw Error("ManifestHelper needs both origin and manifestURL");

It doesn't need both, right?

Comment 2

[[:fabrice] Fabrice Desré]

(In reply to Marco Castelluccio [:marco] from comment #1)

> Instead of passing both the origin and the manifest URL, you could pass the
> app object. This way in the following lines you can use the |kind| property
> you're adding in another bug.
> This would require callers to get the app object before creating a
> ManifestHelper, but most of the callers already have an app object.

most, but not all, and I don't want to incur the cost of retrieving a full app object in these cases. Which means that we'll end up building a fake app object with { origin: ..., manifestURL: ... } which is not great.

> @@ +599,5 @@
> > +  // If the app is packaged, we resolve uris against the origin.
> > +  // If it's not, against the manifest url.
> > +
> > +  if (!aOrigin || !aManifestURL) {
> > +    throw Error("ManifestHelper needs both origin and manifestURL");
> 
> It doesn't need both, right?

It does for packaged apps.

Comment 3

[Marco Castelluccio [:marco]]

(In reply to Fabrice Desré [:fabrice] from comment #2)
> (In reply to Marco Castelluccio [:marco] from comment #1)
> 
> > Instead of passing both the origin and the manifest URL, you could pass the
> > app object. This way in the following lines you can use the |kind| property
> > you're adding in another bug.
> > This would require callers to get the app object before creating a
> > ManifestHelper, but most of the callers already have an app object.
> 
> most, but not all, and I don't want to incur the cost of retrieving a full
> app object in these cases. Which means that we'll end up building a fake app
> object with { origin: ..., manifestURL: ... } which is not great.

Well, there are just two places where you don't already have an app object, and
they are:
1) http://mxr.mozilla.org/mozilla-central/source/dom/activities/src/ActivitiesService.jsm in |startActivity|
2) http://mxr.mozilla.org/mozilla-central/source/dom/messages/SystemMessagePermissionsChecker.jsm in |isSystemMessagePermittedToRegister|

In 1, we're calling _appIdForManifestURL, that is looping over all the objects in
|this.webapps|, so it would have almost the same cost as a getAppByManifestURL.
In 2, the callers of the function already have an app object and are passing
|app.origin| to the function (so we could just make them pass |app|).


> > @@ +599,5 @@
> > > +  // If the app is packaged, we resolve uris against the origin.
> > > +  // If it's not, against the manifest url.
> > > +
> > > +  if (!aOrigin || !aManifestURL) {
> > > +    throw Error("ManifestHelper needs both origin and manifestURL");
> > 
> > It doesn't need both, right?
> 
> It does for packaged apps.

Yeah, the comment was related to the case where we passed the app object.

Comment 4

[[:fabrice] Fabrice Desré]

(In reply to Marco Castelluccio [:marco] from comment #3)
> (In reply to Fabrice Desré [:fabrice] from comment #2)
> > (In reply to Marco Castelluccio [:marco] from comment #1)
> > 
> > > Instead of passing both the origin and the manifest URL, you could pass the
> > > app object. This way in the following lines you can use the |kind| property
> > > you're adding in another bug.
> > > This would require callers to get the app object before creating a
> > > ManifestHelper, but most of the callers already have an app object.
> > 
> > most, but not all, and I don't want to incur the cost of retrieving a full
> > app object in these cases. Which means that we'll end up building a fake app
> > object with { origin: ..., manifestURL: ... } which is not great.
> 
> Well, there are just two places where you don't already have an app object,
> and
> they are:
> 1)
> http://mxr.mozilla.org/mozilla-central/source/dom/activities/src/
> ActivitiesService.jsm in |startActivity|
> 2)
> http://mxr.mozilla.org/mozilla-central/source/dom/messages/
> SystemMessagePermissionsChecker.jsm in |isSystemMessagePermittedToRegister|
> 
> In 1, we're calling _appIdForManifestURL, that is looping over all the
> objects in
> |this.webapps|, so it would have almost the same cost as a
> getAppByManifestURL.

Don't you remember how costly it is to create these objects through the apps service? What is the benefit we would get here?

Overall, ManifestHelper doesn't need an app object, and I like that we are explicit with that. We can agree to disagree!

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8461053 [details] [diff] [review]
manifesthelper.patch

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

There are a few trivial conflicts, but otherwise this applies cleanly and works as expected.

::: dom/activities/src/ActivitiesService.jsm
@@ +294,5 @@
>            let results = [];
>            aManifests.forEach((aManifest, i) => {
>              let manifestURL = aResults.options[i].manifest;
> +            // Not passing the origin is fine here since we only need
> +            // helper.name which doesn't rely on url resolution.

Thanks for this (and the other similar) clarifying comment!

::: dom/apps/src/AppsUtils.jsm
@@ +742,5 @@
>  
>      return null;
>    },
>  
> +  resolve: function(aURI) {

Nit: I would call this resolveURL to make its function clearer.

::: mobile/android/chrome/content/WebappRT.js
@@ +84,5 @@
>          let apps = request.result;
>          for (let i = 0; i < apps.length; i++) {
>            let app = apps[i];
> +          let manifest =
> +            new ManifestHelper(app.manifest, app.origin, app.manifestURL);

Nit: here and elsewhere in mobile/android/, code style is to prefer longer lines, up to about 120 columns.

Comment 6

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/5a84a1907654

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5a84a1907654

Comment 8

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/bdd901635243

Comment 9

[[:fabrice] Fabrice Desré]

That was a backout for causing bug 1046928

Comment 10

[Jason Duell]

Looks like this patch must somehow cause a null SerializedLoadContext to be passed into setCookie somehow:

Parent 709] WARNING: CookieServiceParent: GetAppInfoFromParams: FATAL error: SerializedLoadContext from child is null: KILLING CHILD PROCESS
I/Gecko   (  709): : file ../../../netwerk/cookie/CookieServiceParent.cpp, line 41
I/Gecko   (  709): IPDL protocol error: Handler for SetCookieString returned error code
I/Gecko   (  709):
I/Gecko   (  709): ###!!! [Parent][DispatchAsyncMessage] Error: Processing error: message was deserialized, but the handler returned false (indicating failure)

That means the the nsIChannel argument for SetCookie()  was either null, or did not have it's .notificationCallbacks set to something that can QI to a nsILoadContext.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/bdd901635243

Comment 12

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/83532407ecf9

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/83532407ecf9