949242 - [inter-app-comm-api] Wrongly filters out messages when the page has a hash

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Kevin Grandon :kgrandon]

See: http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/InterAppMessagePort.js#192

When a page changes its hash value, it should still receive messages.

Comment 1

[Kevin Grandon :kgrandon]

Attachment: naive-patch.patch

There's probably a better way of doing this. Feel free to take this patch over if you'd like. Thanks.

Comment 2

[Gene Lian (I already quit Mozilla)]

Thanks Kevin for catching this. So, If we have two pages URL:

  app://pubApp.gaiamobile.org/handler.html?aaa
  app://pubApp.gaiamobile.org/handler.html?bbb

and each of them hold a port waiting to receive message. Under this circumstance, the page ?aaa will receive the message that is supposed to be sent to ?bbb. It doesn't sound reasonable to me.

Or these two pages ?aaa and ?bbb actually represent the *same* page in reality? If yes, I think I can then pass the review. :)

Comment 3

[Gene Lian (I already quit Mozilla)]

Hi Fabrice, I'd like to ask one quick question: do we have any existing utility functions to remove the suffix of |?...| or |#...|? For example,

  app://pubApp.gaiamobile.org/handler.html?aaa -> app://pubApp.gaiamobile.org/handler.html
  app://pubApp.gaiamobile.org/handler.html#aaa -> app://pubApp.gaiamobile.org/handler.html

Comment 4

[Kevin Grandon :kgrandon]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #2)
> Or these two pages ?aaa and ?bbb actually represent the *same* page in
> reality? If yes, I think I can then pass the review. :)

Honestly I suppose it would be up to each application as to whether or not it would consider them a "page". After thinking about it, I suppose my recommendation here would be to actually stop using a string and use a regex pattern instead. This patch would be a larger change, and I'm not sure how to go about proposing it or making it.

This is similar to how we filter in web activities, and may end up being more flexible. E.g.:

"connections": {
  "my-connection": {
    "pattern": "/(foo|bar)\.html.*",
    "rules": {}
  }
}

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #3)
> Hi Fabrice, I'd like to ask one quick question: do we have any existing
> utility functions to remove the suffix of |?...| or |#...|? For example,
> 
>   app://pubApp.gaiamobile.org/handler.html?aaa ->
> app://pubApp.gaiamobile.org/handler.html
>   app://pubApp.gaiamobile.org/handler.html#aaa ->
> app://pubApp.gaiamobile.org/handler.html

Try nsIURI.ref http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIURI.idl#225

Comment 6

[[:fabrice] Fabrice Desré]

Yes, you can use URI.specIgnoringRef for the #, and probably just string search for "?". Another way may be to resolve "." against the current url, that should strip query parameters and ref.

Comment 7

[Kevin Grandon :kgrandon]

Gene, Fabrice - 

What are your thoughts on using a pattern here instead (or in addition) to allow maximum flexibility for developers?

Comment 8

[[:fabrice] Fabrice Desré]

(In reply to Kevin Grandon :kgrandon from comment #7)
> Gene, Fabrice - 
> 
> What are your thoughts on using a pattern here instead (or in addition) to
> allow maximum flexibility for developers?

If you don't have a concrete use case that needs pattern, I'm very tempted to say "no"!

Comment 9

[Kevin Grandon :kgrandon]

(In reply to Fabrice Desré [:fabrice] from comment #8)
> (In reply to Kevin Grandon :kgrandon from comment #7)
> If you don't have a concrete use case that needs pattern, I'm very tempted
> to say "no"!

Ok, then I suppose I would be happy if we just strip querystrings and hashes for now.

Comment 10

[Kevin Grandon :kgrandon]

Comment on attachment 8346252 [details] [diff] [review]
naive-patch.patch

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

Hi Gene - Swapping this over to a review if you think this is good enough. I'm not really sure how to utilize specIgnoringRef, and I think that this is a pretty clean way of solving the problem. If you think that it would be better to explore using some other utility, we can certainly try.

Comment 11

[Gene Lian (I already quit Mozilla)]

Hi Kevin, could you please try uri.specIgnoringRef first to ignore the ref? I guess it's something like:

  uri = Services.io.newURI(pageURL, null, null);
  pageURL = uri.specIgnoringRef;

Comment 12

[Kevin Grandon :kgrandon]

Attachment: Bug-949242-inter-app-comm-api-Wrongly-filters-out-me.patch

Potential new patch, testing it.

Comment 13

[Kevin Grandon :kgrandon]

Comment on attachment 8355237 [details] [diff] [review]
Bug-949242-inter-app-comm-api-Wrongly-filters-out-me.patch

Fabrice - maybe you could also review this one since Gene is out till the 7th?

Comment 14

[[:fabrice] Fabrice Desré]

Comment on attachment 8355237 [details] [diff] [review]
Bug-949242-inter-app-comm-api-Wrongly-filters-out-me.patch

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

lgtm, but I'm not very familiar with this api design. Fernando, can you take over Gene review?

::: dom/apps/src/InterAppMessagePort.js
@@ +60,5 @@
>      this._manifestURL = appsService.getManifestURLByLocalId(principal.appId);
> +    this._pageURL = principal.URI.specIgnoringRef;
> +
> +    // Remove querystrings
> +    this._pageURL = this._pageURL.split('?')[0];

nit: strings use double quotes in this file.

Comment 15

[Kevin Grandon :kgrandon]

Attachment: Bug-949242-inter-app-comm-api-Wrongly-filters-out-me.patch

Updating quote style.

Comment 16

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8355450 [details] [diff] [review]
Bug-949242-inter-app-comm-api-Wrongly-filters-out-me.patch

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

Thanks Kevin for the update and Fabrice for the first review.

r=gene

::: dom/apps/src/InterAppMessagePort.js
@@ +59,5 @@
>      let principal = aWindow.document.nodePrincipal;
>      this._manifestURL = appsService.getManifestURLByLocalId(principal.appId);
> +    this._pageURL = principal.URI.specIgnoringRef;
> +
> +    // Remove querystrings

nit: comment format should be:

//[one_space][upper_case_letter]...[one_period]

so, this comment should be:

// Remove query string.

Note that I added one space between "query" and "string" as well because it's supposed to be separate in a formal definition [1].

[1] http://en.wikipedia.org/wiki/Query_string

Comment 17

[Kevin Grandon :kgrandon]

Attachment: Bug-949242-inter-app-comm-api-Wrongly-filters-out-me.patch

Comment 18

[Kevin Grandon :kgrandon]

On try: https://tbpl.mozilla.org/?tree=Try&rev=ab903d3a5c76

Comment 19

[Kevin Grandon :kgrandon]

Adding checkin-needed for: https://bugzilla.mozilla.org/attachment.cgi?id=8356084

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/add56cded78f

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/add56cded78f

Comment 22

[Kevin Grandon :kgrandon]

Gene - I also think it would be very important to follow-up with some unit test for this. I'm not too familiar with the best way to test gecko code - or what the best testing strategy here would be. Could you give me any pointers for looking at this?

Comment 23

[Gene Lian (I already quit Mozilla)]

(In reply to Kevin Grandon :kgrandon from comment #22)
> Gene - I also think it would be very important to follow-up with some unit
> test for this. I'm not too familiar with the best way to test gecko code -
> or what the best testing strategy here would be. Could you give me any
> pointers for looking at this?

Yes. Bug 915884, which is almost done. Sorry that I'm too busy to keep working on that. Hope I can finish that this week.