1061116 - The criterion of valid |src| for mozwidget

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Junior [inactive]]

A |src| for mozwidget is valid only if the |src| is equal to one of |widgetPages| specified by manifest. Here we want to define the equivalence. The equivalence is perfect coincidence in bug 1005818, and we need to find out if appropriate equivalence is up to path/query/hash fragment.

Comment 1

[John Hu [:johnhu][:johu][:醬糊小弟]]

I think there may be no problem that we make they perfect match on scheme, domain, and path. There are two parts: query and hash needing to defined. In some Forum/CMS/Blog apps, they may use ?p=99 to reference to another page. So, I would suggest that we keep the perfect match on the "query".

IMHO, I would suggest to discard the hash for flexibility because hash may be a link among the page.

Comment 2

[Yadani]

I think it is useful for widget developer if widget API ignores hash tag or query string when gecko compares |widgetPages| and |src|.
Because some developer will want to use hash tag or query string to send information to widget.

Comment 3

[Junior [inactive]]

Here's a scenario:
There's a device with several third-party homescreens.
Each homescreen is bound to a user profile.
Assume here's an homescreen for Andy.

While the homescreen launches an widget for Andy,
the homescreen needs a way to talk to the widget to load Andy's profile.

Since the homescreen might be an privileged app, IAC is not a general solution.

One way is using hash/query like |src=/index.html#AndysHash|, which needs to ease
comparison criterion to hash/query-insensitive instead of perfect coincidence.

Thoughts?

Comment 4

[Paul Theriault [:pauljt] (no longer reading bugmail)]

The manifest declaration is a whitelist only. Does it matter to us if "different" pages are mapped to the same entry in the whitelist? IE I don't think we need to match anything except the path? For exmaple the developer specifies |/user| in the manifest they they can still use <iframe mozwidget=manifestURL src="/user?id=steve#mobileview"> since the path matches. 

Whitelisting on the value of query or hash sounds fragile to me? (what if the query parameters are the same but in a different order?)

Comment 5

[Junior [inactive]]

Attachment: startsWith Comparison

Use startsWith comparison for hasWidgetPage check.
btw, I remove some useless dependency to datastore

without permission: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=addddb83695d
with permission: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ab2b1d3d8072

Comment 6

[[:fabrice] Fabrice Desré]

Comment on attachment 8511894 [details] [diff] [review]
startsWith Comparison

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

::: dom/apps/AppsUtils.jsm
@@ +59,5 @@
>    },
>  
>    hasWidgetPage: function(aPageURL) {
> +    var aPageURLHasPrefix = element => aPageURL.startsWith(element);
> +    return this.widgetPages.find( aPageURLHasPrefix ) !== undefined;

nits: s/var/let and no spaces around ( )

Also, this will not match potentially valid pages:
- lower/upper case mismatch in the scheme and host.
- default port.

eg, HTTP://test:80/widget should match http://test/widget

I know that doesn't work with the current version either, but let's try to improve there! (and add tests for that).

Comment 7

[Junior [inactive]]

Okay. That's a perfect chance to make it complete.
Let's investigate if there's any potential improvement.

Comment 8

[Junior [inactive]]

I have an idea to use |equalsExceptRef|[1] as a criterion since it's still a practical implementation and easy to implement. I wonder if it matters or not to make query sensitive.

[1] http://dxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIURI.idl#234

Is that feasible?

Comment 9

[[:fabrice] Fabrice Desré]

(In reply to Junior Hsu [:juniorhsu] from comment #8)
> I have an idea to use |equalsExceptRef|[1] as a criterion since it's still a
> practical implementation and easy to implement. I wonder if it matters or
> not to make query sensitive.
> 
> [1]
> http://dxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIURI.
> idl#234
> 
> Is that feasible?

I think we need to check without the query string. So it seems you'll have to rebuild uris from origin + path for each one and compare them.

Comment 10

[Junior [inactive]]

Attachment: WIP: hash/query insensitive comparison

WIP patch
1. Make hash/query insensitive.
2. And at least it passes the case in comment 6

Will modify tests.

Comment 11

[Junior [inactive]]

Attachment: WIP: hash/query insensitive comparisonprefix

Substitute for the wrong patch in comment 10

Comment 12

[[:fabrice] Fabrice Desré]

Comment on attachment 8524554 [details] [diff] [review]
WIP: hash/query insensitive comparisonprefix

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

That would be nice to add unit tests for the AppsUtils.jsm function.

Comment 13

[Junior [inactive]]

Attachment: hash/query insensitive comparison

Add test

Hello fabrice,
Sorry for a really long idling :(
Could I have your review?

Comment 14

[Junior [inactive]]

Attachment: hash/query insensitive comparison

add more test case
Hope the test could be an Q&A for checking criterion :)

Comment 15

[[:fabrice] Fabrice Desré]

Comment on attachment 8564954 [details] [diff] [review]
hash/query insensitive comparison

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

::: dom/apps/AppsUtils.jsm
@@ +79,1 @@
>    },

please factor that logic out to share it with webapps.jsm. We really don't want to duplicate this rather strange code ;)

Comment 16

[Junior [inactive]]

Attachment: hash/query insensitive comparison

refactor by comment 15

Comment 17

[Junior [inactive]]

Attachment: hash/query insensitive comparison

extract the logic in test

Comment 18

[[:fabrice] Fabrice Desré]

Comment on attachment 8568454 [details] [diff] [review]
hash/query insensitive comparison

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

Almost there, sorry to be picky.

::: dom/apps/AppsUtils.jsm
@@ +70,5 @@
> +    return this.widgetPages.find(equalCriterion) !== undefined;
> +  },
> +
> +  // Eliminate query and hash string.
> +  getFilePath: function(aPagePath) {

move that function out of the mozIApplication object.

@@ +75,5 @@
> +    let urlParser = Cc["@mozilla.org/network/url-parser;1?auth=no"]
> +                    .getService(Ci.nsIURLParser);
> +    let uriData = [aPagePath, aPagePath.length, {}, {}, {}, {}, {}, {}];
> +    urlParser.parsePath.apply(urlParser, uriData);
> +    let [{value: pathPos}, {value: pathLen}] = uriData.slice(2,4);

nit: space between 2, and 4

Comment 19

[Junior [inactive]]

(In reply to Fabrice Desré [:fabrice] from comment #18)
> Comment on attachment 8568454 [details] [diff] [review]
> hash/query insensitive comparison
> 
> Review of attachment 8568454 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Almost there, sorry to be picky.
> 
> ::: dom/apps/AppsUtils.jsm
> @@ +70,5 @@
> > +    return this.widgetPages.find(equalCriterion) !== undefined;
> > +  },
> > +
> > +  // Eliminate query and hash string.
> > +  getFilePath: function(aPagePath) {
> 
> move that function out of the mozIApplication object.

Any suggesstion place? Since this function needs to be shared in two files, we can not make it global in one file. I guess we can expose symbol here?
https://dxr.mozilla.org/mozilla-central/source/dom/apps/AppsUtils.jsm?from=AppsUtils.jsm#32

Comment 20

[[:fabrice] Fabrice Desré]

Junior, you can just add it on the AppsUtils object.

Comment 21

[Junior [inactive]]

Attachment: hash/query insensitive comparison

Modify by comment 18

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f7ebe7e3415

Comment 22

[Junior [inactive]]

Try server fails since we make query-insensitive, which disobey the origin test case. Will modify the test.

Comment 23

[Junior [inactive]]

Attachment: hash/query insensitive comparison

One line change in file_invalidWidget_app.template.webapp for test.
Carry r+
Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56952b355e02

Comment 24

[Junior [inactive]]

Still hit the jetpack package test fail. Will investigate.

Comment 25

[Junior [inactive]]

Attachment: hash/query insensitive comparison

Rebase, carry r+

The jetpack error disappears after rebase, seems irrelative to the patch.

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8810e975dfe

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/944d7d551bf3

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/944d7d551bf3