Closed Bug 1042881 Opened 5 years ago Closed 5 years ago

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

Categories

(Core Graveyard :: DOM: Apps, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla34

People

(Reporter: fabrice, Assigned: fabrice)

References

Details

Attachments

(1 file)

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
Attachment #8461053 - Flags: review?(myk)
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?
(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.
(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.
(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 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.
Attachment #8461053 - Flags: review?(myk) → review+
https://hg.mozilla.org/mozilla-central/rev/5a84a1907654
Assignee: nobody → fabrice
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
That was a backout for causing bug 1046928
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 1046928
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.
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/bdd901635243
Target Milestone: mozilla34 → ---
https://hg.mozilla.org/mozilla-central/rev/83532407ecf9
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Blocks: 1052722
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.