Closed Bug 1171200 Opened 5 years ago Closed 4 years ago

Add means of checking if a document links to a manifest

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox41 --- affected
firefox42 --- fixed

People

(Reporter: marcosc, Assigned: marcosc)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 11 obsolete files)

36.31 KB, patch
Details | Diff | Splinter Review
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).
Blocks: webmanifest
Assignee: nobody → mcaceres
Summary: Means of checking if a browser links to a manifest via WebManifest.jsm → Add means of checking if a browser links to a manifest via WebManifest.jsm
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.
Attachment #8615443 - Flags: review?(mconley)
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.
Attachment #8615443 - Attachment is obsolete: true
Attachment #8625382 - Flags: feedback?(wmccloskey)
Summary: Add means of checking if a browser links to a manifest via WebManifest.jsm → Add means of checking if a document links to a manifest via WebManifest.jsm
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?
Attachment #8625382 - Flags: feedback?(wmccloskey) → feedback-
(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?
Blocks: 1178175
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?
Flags: needinfo?(wmccloskey)
Attached patch PromiseMessage module (obsolete) — Splinter Review
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.
Flags: needinfo?(wmccloskey)
Attachment #8629160 - Flags: feedback?(mcaceres)
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 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.
Attachment #8629160 - Flags: feedback?(mcaceres) → feedback+
Attached patch Part 1 - Adds PromiseMessage.jsm (obsolete) — Splinter Review
Small bug fixes to Bill's proposal.
Attachment #8625382 - Attachment is obsolete: true
Attachment #8629160 - Attachment is obsolete: true
Attachment #8629907 - Flags: review?(wmccloskey)
Attachment #8629908 - Flags: review?(wmccloskey)
Attached patch Part 3 - Add hasManifest checker (obsolete) — Splinter Review
Implementation using PromiseMessage.jsm
Attachment #8629909 - Flags: review?(wmccloskey)
Please note that this includes ManifestObtainer's refactored message listener. I did both a the same time.
Attachment #8629914 - Flags: review?(wmccloskey)
Attachment #8629918 - Flags: review?(wmccloskey)
(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.
Attachment #8629907 - Flags: review?(wmccloskey) → review+
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.
Attachment #8629909 - Flags: review?(wmccloskey)
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.
Attachment #8629914 - Flags: review?(wmccloskey)
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?
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.
Summary: Add means of checking if a document links to a manifest via WebManifest.jsm → Add means of checking if a document links to a manifest
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.
Flags: needinfo?(mcaceres)
> 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.
Flags: needinfo?(mcaceres)
Attached patch Addressed Bill's feedback. (obsolete) — Splinter Review
Attachment #8629907 - Attachment is obsolete: true
Attachment #8629908 - Attachment is obsolete: true
Attachment #8629909 - Attachment is obsolete: true
Attachment #8629914 - Attachment is obsolete: true
Attachment #8629918 - Attachment is obsolete: true
Attachment #8629908 - Flags: review?(wmccloskey)
Attachment #8629918 - Flags: review?(wmccloskey)
Attachment #8634742 - Flags: review?(wmccloskey)
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.
Flags: needinfo?(mcaceres)
(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.
Flags: needinfo?(mcaceres)
Attached patch Consolidated changes (obsolete) — Splinter Review
Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee3784c2e74d
Attachment #8634742 - Attachment is obsolete: true
Attachment #8635446 - Flags: review?(wmccloskey)
Attached patch Consolidated changes (obsolete) — Splinter Review
Fixed test that was reporting false positive.
Attachment #8635446 - Attachment is obsolete: true
Attachment #8635446 - Flags: review?(wmccloskey)
Attachment #8635964 - Flags: review?(wmccloskey)
Blocks: 1187027
No longer blocks: 1187027
Sorry I've been so slow on this. I promise to finish the review today or tomorrow.
@billm, appreciate the update. Sorry this grew bigger than than it should have been.
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?
Attachment #8635964 - Flags: review?(wmccloskey) → review+
(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.
Tree is closed, so putting this here.
Attachment #8635964 - Attachment is obsolete: true
sorry had to back this out for asan m1 test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=12297516&repo=mozilla-inbound
Flags: needinfo?(mcaceres)
Flags: needinfo?(mcaceres)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7692b5a9d0c5
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Duplicate of this bug: 1178175
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.