1171200 - Add means of checking if a document links to a manifest

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Marcos Caceres [:marcosc]]

It would be useful if WebManifest.jsm exposed some way to check if a browser has a linked manifest (as opposed to trying to obtain the manifest first).

Comment 1

[Marcos Caceres [:marcosc]]

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

Comment 2

[Marcos Caceres [:marcosc]]

Attachment: Adds an e10s friendly way of checking for a web manifest in a content doc

Comment 3

[Marcos Caceres [:marcosc]]

Comment on attachment 8615443 [details] [diff] [review]
Adds an e10s friendly way of checking for a web manifest in a content doc

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

Refactoring this... will send again.

Comment 4

[Marcos Caceres [:marcosc]]

Attachment: Part 1 - base class, manifest finder code, and tests.

Follow on from Bug 1172586, PromiseMessagePasser uses promises together with message manager. This code doesn't rely on Error being cloned (I have other code in dom/ipc/manifestMessages.js that does!), but you can see where I am going with this hopefully.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8625382 [details] [diff] [review]
Part 1 - base class, manifest finder code, and tests.

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

Sorry, this seems like a lot of complexity to accomplish something pretty simple. As far as I can tell the parent process wants to be able to call _checkForManifest in the child and get the result. I can see that there's a little complexity in generating the ID and creating a promise, but you don't need all this code here to do that.

I also don't see why the exceptions are an issue. _checkForManifest doesn't throw as far as I can tell. If it does, it just seems like a bug that we should fix.

I'm happy to look at a patch that has a small JSM that just generates an ID, sends a message on a particular message manager, and resolves a promise when a result with the same ID is received.

::: dom/manifest/PromiseMessagePasser.jsm
@@ +69,5 @@
> +      browserMap.get(aWindow).set(msgId, {
> +        resolve: resolve,
> +        reject: reject
> +      });
> +      mm.sendAsyncMessage(aMsgKey, {

There's no sendAsyncMessage method defined on a XUL window. You would need to use broadcastAsyncMessage (which would broadcast to every tab in the window). However, I suspect that's not what you want. Do you want to send the message to the currently selected tab?

Comment 6

[Marcos Caceres [:marcosc]]

(In reply to Bill McCloskey (:billm) from comment #5)

Thanks for the feedback... 

> Comment on attachment 8625382 [details] [diff] [review]
> Part 1 - base class, manifest finder code, and tests.
> 
> Review of attachment 8625382 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry, this seems like a lot of complexity to accomplish something pretty
> simple. As far as I can tell the parent process wants to be able to call
> _checkForManifest in the child and get the result. I can see that there's a
> little complexity in generating the ID and creating a promise, but you don't
> need all this code here to do that.

Unless I've misunderstood something, you need a way of associating the browser (content) being queried and message manager along with the promises that need to be resolved/rejected (without disconnecting the message manager from listening for messages in case multiple calls are made by one caller). This was the simplest design I could come up with to cope with that.

I could simplify this a little bit by not de-registering the message listener, but then it would be keeping a connection to the frame script around unnecessarily. 

If you have suggestions of how to make it simpler, while retaining the ability to use promises, I'm all ears. 

> I also don't see why the exceptions are an issue. _checkForManifest doesn't
> throw as far as I can tell. If it does, it just seems like a bug that we
> should fix.

Yes, of course this would totally be overkill if it was only for _checkForManifest(), but I want to use PromiseMessagePasser generically for other projects.

For example, the ManifestObtainer (see dom/manifest/ManifestObtainer.jsm), which I've also refactored to use PromiseMessagePasser, can throw/reject for a variety of reasons: 

* network error,
* CSP violation,
* parsing error,
* etc.  

Having PromiseMessagePasser handling all the message passing avoids a bunch of code repetition and gives me a generic set of tools/protocols that I can use to simplify IPC (+ works with async-await thanks to promises). This makes async code easier to read/write.

> I'm happy to look at a patch that has a small JSM that just generates an ID,
> sends a message on a particular message manager, and resolves a promise when
> a result with the same ID is received.
> 
> ::: dom/manifest/PromiseMessagePasser.jsm
> @@ +69,5 @@
> > +      browserMap.get(aWindow).set(msgId, {
> > +        resolve: resolve,
> > +        reject: reject
> > +      });
> > +      mm.sendAsyncMessage(aMsgKey, {
> 
> There's no sendAsyncMessage method defined on a XUL window. You would need
> to use broadcastAsyncMessage (which would broadcast to every tab in the
> window). However, I suspect that's not what you want. Do you want to send
> the message to the currently selected tab?

More or less (but not necessarily the currently selected tab, any tab). Checks for manifests would generally be performed in the background. The above works fine tho in all my tests. What am I missing?

Comment 7

[Marcos Caceres [:marcosc]]

Hi Bill, does comment 6 address your concerns? I would like to land this ASAP as it's blocking other projects. I can try to refactor/simplify the PromiseMessagePasser later to be less complex, but right now it's good enough to do what I need it to do. 

Also, are you ok to review it or can you recommend someone else?

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: PromiseMessage module

Hi Marcos,
Sorry for the delay. I took a closer look at the code in your try push and I think I understand it better. (By the way, the patch you posted didn't include changes to manifestMessages.js or browser_hasManifestLink.js; those two files cleared a lot up for me.)

One thing that confused me is that aWindow is not a XUL window--it's a XUL <browser> element. We don't normally call those windows. Your use of sendAsyncMessage now makes more sense.

I'm attaching a patch that's more along the lines I was asking for. Let me know if it will work for you. If it doesn't suit your needs, perhaps we can figure out how to fix it up.

Comment 9

[Marcos Caceres [:marcosc]]

Comment on attachment 8629160 [details] [diff] [review]
PromiseMessage module

This is nice :) However, it pushes some of the things I was trying to centralize up the stack again. I think that's fine tho, because most of what I'm doing is pretty simple. 

I'm going to plug this in and see how well it works (requires a bit of refactoring, obviously). Will get back to you in after the weekend with any additional feedback.

Comment 10

[Marcos Caceres [:marcosc]]

Comment on attachment 8629160 [details] [diff] [review]
PromiseMessage module

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

Plugged this and made a made it so it works with `reply.data.id` instead. Works well.

::: toolkit/modules/PromiseMessage.jsm
@@ +24,5 @@
> +
> +    // Return a promise that resolves when we get a reply (a message of the same name).
> +    return new Promise(resolve => {
> +      messageManager.addMessageListener(name, function listener(reply) {
> +        if (reply.id != id) {

This should be reply.data.id, no?

This bit still worries me tho... if you get a strange ID, hasn't something gone pretty wrong? I'm worried about just returning without at least warning.

Comment 11

[Marcos Caceres [:marcosc]]

Attachment: Part 1 - Adds PromiseMessage.jsm

Small bug fixes to Bill's proposal.

Comment 12

[Marcos Caceres [:marcosc]]

Attachment: Part 2 - Adds tests for hasManifest

Comment 13

[Marcos Caceres [:marcosc]]

Attachment: Part 3 - Add hasManifest checker

Implementation using PromiseMessage.jsm

Comment 14

[Marcos Caceres [:marcosc]]

Attachment: Part 4 - Update how messages are handled on the content side

Please note that this includes ManifestObtainer's refactored message listener. I did both a the same time.

Comment 15

[Marcos Caceres [:marcosc]]

Attachment: Part 5 - Expose hasManifest through WebManifest.jsm

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Marcos Caceres [:marcosc] from comment #10)
> Comment on attachment 8629160 [details] [diff] [review]
> PromiseMessage module
> 
> Review of attachment 8629160 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Plugged this and made a made it so it works with `reply.data.id` instead.
> Works well.
> 
> ::: toolkit/modules/PromiseMessage.jsm
> @@ +24,5 @@
> > +
> > +    // Return a promise that resolves when we get a reply (a message of the same name).
> > +    return new Promise(resolve => {
> > +      messageManager.addMessageListener(name, function listener(reply) {
> > +        if (reply.id != id) {
> 
> This should be reply.data.id, no?

Oops, sorry. I wrote the code without testing it.

> This bit still worries me tho... if you get a strange ID, hasn't something
> gone pretty wrong? I'm worried about just returning without at least warning.

An incorrect ID will happen if there are two callers with outstanding PromiseMessage.send calls. Both will be notified when the first reply comes in, and one of them needs to ignore the reply.

An alternate way to do this would be to make the message name for the reply be `${name}-${id}`. That would be more efficient.

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8629909 [details] [diff] [review]
Part 3 - Add hasManifest checker

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

::: dom/manifest/hasManifestLink.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

This filename should end in .jsm and start with a capital letter since it's imported with Cu.import.

@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
> +/* globals Components */
> +'use strict';

We try to use double-quotes everywhere in Firefox JS code. There's probably a lot of exceptions, but please use double quotes in these patches. It makes it easier to search for string constants without using a regular expression.

@@ +7,5 @@
> +  utils: Cu
> +} = Components;
> +const {
> +  PromiseMessage
> +} = Cu.import('resource://gre/modules/PromiseMessage.jsm', {});

We try to stick to a style where a module exports one symbol which has the same name as the module (PromiseMessage for PromiseMessage.jsm for example). Then you can just do Cu.import("resource://gre/modules/PromiseMessage.jsm"); without any destructuring.

@@ +12,5 @@
> +const {
> +  Task: {
> +    async
> +  }
> +} = Components.utils.import('resource://gre/modules/Task.jsm', {});

Same: no need for destructuring.

@@ +15,5 @@
> +  }
> +} = Components.utils.import('resource://gre/modules/Task.jsm', {});
> +const {
> +  MANIFEST_SELECTOR
> +} = Cu.import('resource://gre/modules/WebManifest.jsm', this);

The destructuring makes sense here because you're importing a symbol that has a different name from the module.

However, the only use of this symbol is from this module. It might be better to move it here.

@@ +22,5 @@
> + * Singleton used to check if a window's document has
> + * a manifest. It is exposed as hasManifestLink.
> + */
> +function ManifestFinder() {}
> +ManifestFinder.prototype = Object.create(Object.prototype, {

Why not just use a normal object literal?

let ManifestFinder = {
  hasManifestLink: ...
};

@@ +33,5 @@
> +   * @param aWindowOrBrowser the XUL browser or window to check.
> +   * @return {Promise}
> +   */
> +  hasManifestLink: {
> +    value: async(function* (aWindowOrBrowser) {

Is there any reason this couldn't be split into one function that operates on <browser> elements and one function that operates on content windows? We generally try to keep code that operates in the content process completely separate from code in the parent process.

Given how simple the code for _checkForManifest is, could you just put the code directly in the message handler in manifestMessages.js?

@@ +48,5 @@
> +    })
> +  }
> +});
> +
> +function _isXULBrowser(aBrowser) {

There's no need for the underscore since names are automatically hidden if they're not in EXPORTED_SYMBOLS.

@@ +53,5 @@
> +  const XUL = 'http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul';
> +  return (aBrowser.namespaceURI && aBrowser.namespaceURI === XUL);
> +}
> +
> +function _checkForManifest(aWindow) {

Same here.

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8629914 [details] [diff] [review]
Part 4 - Update how messages are handled on the content side

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

::: dom/ipc/manifestMessages.js
@@ +27,2 @@
>    }
> +} = Cu.import('resource://gre/modules/Task.jsm', {});

Same stuff about the destructuring.

@@ +37,5 @@
> +      'DOM:ManifestObtainer:Obtain',
> +      this._obtainManifest.bind(this)
> +    );
> +  },
> +  /**

Please put a blank line between methods.

@@ +57,5 @@
> +    const obtainer = new ManifestObtainer();
> +    const response = this._makeMsgResponse(id);
> +    try {
> +      response.success = true;
> +      response.result = yield obtainer.obtainManifest(content);

This doesn't seem right unless there are more patches coming. obtainManifest operates on a <browser> element and you're giving it a content window.

@@ +77,5 @@
> +   * FIX ME: https://bugzilla.mozilla.org/show_bug.cgi?id=1172586
> +   * @param  {Error} aError The error to serialize.
> +   * @return {Object} The serialized object.
> +   */
> +  _serializeError(aError) {

I think this should be eliminated.

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8629918 [details] [diff] [review]
Part 5 - Expose hasManifest through WebManifest.jsm

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

Does this file need to exist at all? Why not just have each consumer import the JSM they need?

Comment 20

[Marcos Caceres [:marcosc]]

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

Comment 21

[Marcos Caceres [:marcosc]]

Hi Bill, thanks for your help. I made the following changes as you suggested:

* renamed files .jsm with capitalized names.
* made sure only one symbol exported per JSM
* Dropped destructoring on Cu.imports where not needed.
* Used object literal in ManifestFinder (renamed file also ManifestFinder and made it a constructor)
* Dropped EXPORTED_SYMBOLS, just hard coded it where used.
* Added blank lines between functions where missing.
* Deleted WebManifest.jsm. 

Didn't change:

 * single quotes to double quotes - sorry, that horse has bolted. I'd also like to see Mozilla move to JSCS, because the inconsistency across our JS code is sad pandas.
 * retained single function for content and browser windows, so to keep API symmetrical.
 * checkForManifest() code remains in ManifestFinder.jsm. I prefer to keep all the code in the JSM and just let manifestMessages.js just do the message passing. 
 * serializeError() is still needed by ManifestObtainer, so kept it for now. Happy to discuss in another bug.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/01d03b6be047

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/01d03b6be047

Comment 24

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I think maybe you meant to upload new patches here?

(In reply to Marcos Caceres [:marcosc] from comment #21)
> Hi Bill, thanks for your help. I made the following changes as you suggested:
> 
> * renamed files .jsm with capitalized names.
> * made sure only one symbol exported per JSM
> * Dropped destructoring on Cu.imports where not needed.
> * Used object literal in ManifestFinder (renamed file also ManifestFinder
> and made it a constructor)
> * Dropped EXPORTED_SYMBOLS, just hard coded it where used.
> * Added blank lines between functions where missing.
> * Deleted WebManifest.jsm. 

Thanks!

> Didn't change:
> 
>  * single quotes to double quotes - sorry, that horse has bolted. I'd also
> like to see Mozilla move to JSCS, because the inconsistency across our JS
> code is sad pandas.

Our coding style states that "Double-quoted strings (e.g. "foo") are preferred to single-quoted strings (e.g. 'foo') in JavaScript, except to avoid escaping of embedded double quotes or when assigning inline event handlers."

If we move to JSCS, we'll have to convert all our code to match our own style, which will mean switching this patch to use double quotes. So please just do that :-).

>  * retained single function for content and browser windows, so to keep API
> symmetrical.

I don't agree with this. In converting things to e10s, it's been really helpful to separate chrome-touching code (which uses <browser> elements) from content-touching code (which uses content windows) as much as possible. Otherwise things get really confusing, especially since JS doesn't have types. So please keep them separate.

>  * checkForManifest() code remains in ManifestFinder.jsm. I prefer to keep
> all the code in the JSM and just let manifestMessages.js just do the message
> passing. 
>  * serializeError() is still needed by ManifestObtainer, so kept it for now.
> Happy to discuss in another bug.

OK, I don't feel that strongly about these two, although I'd like to see how things end up in a new patch.

Comment 25

[Marcos Caceres [:marcosc]]

> If we move to JSCS, we'll have to convert all our code to match our own style, which will mean switching this patch to use double quotes. So please just do that :-).

Checkmate :( Ok. 

> I don't agree with this. In converting things to e10s, it's been really helpful to separate chrome-touching code (which uses <browser> elements) from content-touching code (which uses content windows) as much as possible. Otherwise things get really confusing, especially since JS doesn't have types. So please keep them separate.

Ok, yeah... my abuse of types was kinda nasty in hindsight. I added contentHasManifest() and browserHasManifest() to ManifestFinder.jsm - and similarly named methods for ManifestObtainer.jsm. 

> OK, I don't feel that strongly about these two, although I'd like to see how things end up in a new patch.

Building now... will send patch in the next hour.

Comment 26

[Marcos Caceres [:marcosc]]

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

Comment 27

[Marcos Caceres [:marcosc]]

Attachment: Addressed Bill's feedback.

Comment 28

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Marcos, I just noticed that you landed a bunch of patches here already. The only patch that I reviewed was for PromiseMessage.jsm. However, commit 01d03b6be047 includes changes to all sorts of other files that I did not r+. Please back all this stuff out immediately. Landing stuff without review is a pretty serious breach of our policies.

Once everything is backed out, please post a complete patch or patches for review.

Comment 29

[Marcos Caceres [:marcosc]]

(In reply to Bill McCloskey (:billm) from comment #28)
> Marcos, I just noticed that you landed a bunch of patches here already. The
> only patch that I reviewed was for PromiseMessage.jsm. However, commit
> 01d03b6be047 includes changes to all sorts of other files that I did not r+.

Ah, sorry. I saw the r+ and no r-'s and thought it applied to the whole thing. 

> Please back all this stuff out immediately. Landing stuff without review is
> a pretty serious breach of our policies.
> 
> Once everything is backed out, please post a complete patch or patches for
> review.

Will do.

Comment 30

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6922e4bb78b

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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

Comment 32

[Marcos Caceres [:marcosc]]

Attachment: Consolidated changes

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

Comment 33

[Marcos Caceres [:marcosc]]

Attachment: Consolidated changes

Fixed test that was reporting false positive.

Comment 34

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Sorry I've been so slow on this. I promise to finish the review today or tomorrow.

Comment 35

[Marcos Caceres [:marcosc]]

@billm, appreciate the update. Sorry this grew bigger than than it should have been.

Comment 36

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8635964 [details] [diff] [review]
Consolidated changes

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

Thanks for making the changes.

::: dom/manifest/ManifestFinder.jsm
@@ +11,5 @@
> +
> +/**
> + * @constructor
> + */
> +function ManifestFinder() {}

Given that there isn't any state stored here, an object literal would be more typical:

const ManifestFinder = {
  contentHasManifestLink() {...},
  browserHasManifestLink() {...}
};

Then the callers don't need to construct anything.

::: dom/manifest/ManifestObtainer.jsm
@@ +92,5 @@
> +    reqInit.credentials = "include";
> +  }
> +  const req = new aWindow.Request(manifestURL, reqInit);
> +  req.setContentPolicyType(Ci.nsIContentPolicy.TYPE_WEB_MANIFEST);
> +  const response = yield aWindow.fetch(req);

I think someone else should review this who understands how manifests actually work. Is there someone who could do that?

Comment 37

[Marcos Caceres [:marcosc]]

(In reply to Bill McCloskey (:billm) from comment #36)
> Comment on attachment 8635964 [details] [diff] [review]
> Consolidated changes
> 
> Review of attachment 8635964 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for making the changes.
> 
> ::: dom/manifest/ManifestFinder.jsm
> @@ +11,5 @@
> > +
> > +/**
> > + * @constructor
> > + */
> > +function ManifestFinder() {}
> 
> Given that there isn't any state stored here, an object literal would be
> more typical:
> 
> const ManifestFinder = {
>   contentHasManifestLink() {...},
>   browserHasManifestLink() {...}
> };
> 
> Then the callers don't need to construct anything.

Good point. Someone else had told me to make these constructors previously - but I'm going to change them back. 

> ::: dom/manifest/ManifestObtainer.jsm
> @@ +92,5 @@
> > +    reqInit.credentials = "include";
> > +  }
> > +  const req = new aWindow.Request(manifestURL, reqInit);
> > +  req.setContentPolicyType(Ci.nsIContentPolicy.TYPE_WEB_MANIFEST);
> > +  const response = yield aWindow.fetch(req);
> 
> I think someone else should review this who understands how manifests
> actually work. Is there someone who could do that?

This bit of code was already reviewed during the CSP implementation part of things (by :ehsan and a few people familiar with CSP). I also wrote the w3c spec - both web manifest and the CSP part relating to manifest-src.

Comment 38

[Marcos Caceres [:marcosc]]

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

Comment 39

[Marcos Caceres [:marcosc]]

Attachment: Addressed final changes from billm.

Tree is closed, so putting this here.

Comment 40

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b328a6f7448

Comment 41

[Carsten Book [:Tomcat]]

sorry had to back this out for asan m1 test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=12297516&repo=mozilla-inbound

Comment 42

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d61ae75fd242

Comment 43

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7692b5a9d0c5

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7692b5a9d0c5