1169633 - [Browser API] getWebManifest()

Status: RESOLVED FIXED

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

Description

[Ben Francis [:benfrancis]]

As a developer I'd like to request a web manifest for the current page and have it fetched using the context of the page (e.g. for authenticated resources).

I propose adding a getManifest() method to the Browser API which returns a Promise that resolves with a sanitised version of the manifest JSON data.

Needs hooking up to manifest fetcher implemented under bug 1083410.

Comment 1

[Ben Francis [:benfrancis]]

Marcos, what do you think needs to be done to hook the Browser API up to your manifest implementation like this?

Comment 2

[Marcos Caceres [:marcosc]]

Not sure, tbh. Probably just need to add a frame-script to the content process used by b2g. Who could we ask about that? If you guys are already using browser.js, then in theory it should just work... otherwise, we just need to add dom/ipc/manifestMessages.js somewhere B2G can get to it. Then all you need to do is Cu.import(WebManifest.jsm) wherever you want to fetch manifest from and you are good to go. 

BTW, I should have a fancy new version of WebManifest.jsm hopefully landed by next week - but can again try to help with integration.

Comment 3

[Ben Francis [:benfrancis]]

Ted, are you able to work this out?

I think it probably needs to start with a new getManifest() method in http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.js which maybe gets handled in http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChildPreload.js and hooked up to https://dxr.mozilla.org/mozilla-central/source/dom/manifest/ManifestObtainer.js

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually, the easier solution here is likely to enable any page that has access to the browser API to create an XHR object which uses that browser's cookie-jar.

Basically something like:

new XMLHttpRequest({ mozbrowser: true });

Doing this would require access to the browser API. If the page doesn't have permission to use the browser API the above would throw a security exception.

Any request made by the object created above would use the same cookie-jar as pages inside a <iframe mozbrowser> created by the same page.

Implementing this would basically involve creating a fake loadgroup which has a loadcontext with the appropriate appid/browserflag, and then use that when creating the channel here
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsXMLHttpRequest.cpp#1773

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This is something we'll need anyway in order to actually do updates of icons.

Comment 6

[Ben Francis [:benfrancis]]

> Actually, the easier solution here is likely to enable any page that has access to the browser API to create an XHR object which uses that browser's cookie-jar.

To be clear, this wouldn't use Marcos' manifest fetcher implementation at all? Would this be compliant with the spec in terms of CSP etc?

I'm guessing this would also require the mozSystem [1] feature of XHR in order to bypass any CORS headers set on the manifest? Marcos, would that be enough?

https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest#Non-standard_properties

Comment 7

[Ben Francis [:benfrancis]]

For reference: http://www.w3.org/TR/appmanifest/#content-security-policy

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, we'd need to set something in order to allow fetching cross-origin. I'm not sure if mozSystem is the most appropriate given that normally that makes the request go out without any cookies at all.

I don't know how Marcos manifest-fetcher is implemented, so can't speak to if that's reusable. If it's implemented in JS, then maybe the system app could run that code?

Honoring CSP would indeed be a concern.

Comment 9

[Marcos Caceres [:marcosc]]

ManifestObtainer.jsm is implemented in JS, supports manifest-src CSP rules + conforms to other CSP requirements, is able to send cookies cross-origin if needed (as it respects <link crossorigin>), and integrates appropriately with service workers (by running in the correct CSP/fetch context: "manifest"). 

It seems trivial to hook this up. If we could find like half a day with someone that knows the FxOS JS architecture well, it would be trivial to wire up. I am more than happy to help, but just need a FxOS/JS person to work with.  

I don't have time to do this exploration myself - but if we find someone and we can get it integrated quickly (I don't know people on the FxOS team, so I can't suggest anyone - maybe there is even someone in Toronto?). Then I can also provide the code that picks the right icon for the display context. I'm working on that ATM.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ben, Marcos, do we have an implementation strategy for how to implement updates of the homescreen icon once the user has pinned the app? Is that something that we're planning on doing for 2.5? Is getManifest() enough to get updates working, or will other components (such as the XHR API in comment 4) needed as well?

Comment 11

[Marcos Caceres [:marcosc]]

Just FYI, I'm on PTO till the 11th of Aug.... 

(In reply to Jonas Sicking (:sicking) from comment #10)
> Ben, Marcos, do we have an implementation strategy for how to implement
> updates of the homescreen icon once the user has pinned the app?

I would like to work with someone one that - which would also allow us to fix the spec. However, I don't have time to learn b2g architecture, so would really prefer to work with someone that knows b2g well and I can just make changes to the web manifest code as needed + to the spec. 

> Is that
> something that we're planning on doing for 2.5? Is getManifest() enough to
> get updates working, or will other components (such as the XHR API in
> comment 4) needed as well?

We will probably discover a bunch of fun issues when we implement. We can't use XHR because of the CSP dependencies and general integration with service workers. However, fetching a manifest is implemented already - we just need to prototype how it would work to discover what the issues are. Again, I'm happy to collaborate, but really need you or Ben to find me an engineer to collaborate with. That could be Ted or someone that's already worked with the homescreen code?

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm not sure what the question to me is?

It seems pretty clear how we need to do updates, no? We'll have to re-download the HTML URL that was pinned, and then update the icon based on any <meta> or <link rel=manifest> that exists in there? At the very least that must be the default strategy. I guess we could allow developers to opt into something different, but I don't know if there are use cases.

I still think that we need to have an implementation strategy for how to implement the above before we move forward.

I don't know that implementing the CSP policy is needed for the initial release here? It can easily be worked around by an attacker using <meta> tags, no?

Comment 13

[Marcos Caceres [:marcosc]]

(In reply to Jonas Sicking (:sicking) from comment #12)
> I'm not sure what the question to me is?

Sorry, the questions is: can you or Ben please find me someone to work with on this? I need someone who knows B2G really well, specially the JS side of things. As much as I would love to go it alone, I can't do it: it will take me too long and don't want to hold things up. 

> It seems pretty clear how we need to do updates, no? We'll have to
> re-download the HTML URL that was pinned, and then update the icon based on
> any <meta> or <link rel=manifest> that exists in there? At the very least
> that must be the default strategy. I guess we could allow developers to opt
> into something different, but I don't know if there are use cases.
> 
> I still think that we need to have an implementation strategy for how to
> implement the above before we move forward.

The strategy can be the one above. Elsewhere, we discussed only updating apps when the user launches them (i.e., after relaunch of the start URL). As there is no certainty about the location of the manifest, it's the only sure way to get the right manifest that corresponds to that user's credentials. 
 
> I don't know that implementing the CSP policy is needed for the initial
> release here?

Don't worry. I already implemented that. 

> It can easily be worked around by an attacker using <meta>
> tags, no?

No? Why would it? Meta tags are ignored if there is a manifest and things like link@rel=icon are governed by "image" and "default" policies.

Comment 14

[Kan-Ru Chen [:kanru] (UTC+9)]

I can help with the implementation if we are clear how to go forward.

Comment 15

[Ben Francis [:benfrancis]]

Firstly, Kan-Ru is the perfect person to work with Marcos on this bug and Michael and I already talked with him about it. Thank you Kan-Ru!

I think if we had getManifest() we could implement manifest updates in Gaia, I've filed bug 1190388 for that. My proposal is to just check for a new manifest whenever the user loads the start_url. I know Jonas has suggested periodically checking for manifest updates outside of user interactions in addition to this, but I haven't yet figured out how that could work. I think bug 1190388 is a good first step.

I think we should target app manifest updates for 2.5, but we could ship 2.5 without it if we really had to (Chrome did). I've added the user story to the Product Backlog.

Marcos, we're currently implementing the icon fetching in Gaia (like we do for icon link relations), let me know if you think there's a reason that should happen in Gecko instead, but we'd likely have to expose an API for that functionality too if that's the case.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think long term it's not an acceptable solution to only update the icon when the user opens the app. Mainly because a rebranded icon is how I think some developers will want to attract users to open an app which they haven't used for some time.

But obviously we don't need to do that in the 2.5 timeframe.

Comment 17

[Marcos Caceres [:marcosc]]

(In reply to Jonas Sicking (:sicking) from comment #16)
> I think long term it's not an acceptable solution to only update the icon
> when the user opens the app. Mainly because a rebranded icon is how I think
> some developers will want to attract users to open an app which they haven't
> used for some time.

Maybe use push and SW background-sync instead? the icons can be pulled from a SW's cache. 

> But obviously we don't need to do that in the 2.5 timeframe.

Comment 18

[Marcos Caceres [:marcosc]]

Attachment: 0001-Bug-1169633-Browser-API-getWebManifest.patch

Comment 19

[Marcos Caceres [:marcosc]]

Comment on attachment 8648244 [details] [diff] [review]
0001-Bug-1169633-Browser-API-getWebManifest.patch

Ted, just mirrored your other patch. However, I don't have a b2g env. set up, so I can't test this :( Could use your help with that. Should be trivial to test.

Comment 20

[Ted Clancy [:tedders1]]

Hi Marcos,

I'm afraid this doesn't seem to be working for me. When I call iframe.getWebManifest(), I get an exception that says "InvalidStateError: An attempt was made to use an object that is not, or is no longer, usable ".

But I also can't see anything wrong with the code. It looks like it should work.

Let's talk about this offline.

Comment 21

[Marcos Caceres [:marcosc]]

Attachment: Updated UUID

Was busted because I forgot to update the IDL's UUID.

Comment 22

[Marcos Caceres [:marcosc]]

Attachment: Fixed typo

Sorry, had a mistake in the last patch. FYI, writing tests in a separate patch :)

Comment 23

[Marcos Caceres [:marcosc]]

Attachment: 0001-Bug-1169633-Browser-API-getWebManifest-.-r-kanru.patch

Updated patch with tests

Comment 24

[Marcos Caceres [:marcosc]]

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

Comment 25

[Marcos Caceres [:marcosc]]

ping

Comment 26

[Marcos Caceres [:marcosc]]

Attachment: Moved oop tests to right place

Moved the oop tests to the right mochitest ini file.

Comment 27

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8661029 [details] [diff] [review]
Moved oop tests to right place

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

Looks good to me.

::: dom/browser-element/BrowserElementChildPreload.js
@@ +1492,5 @@
> +    if (hasManifest) {
> +      try {
> +        manifest = yield ManifestObtainer.contentObtainManifest(content);
> +      } catch(e) {
> +        debug(`Error fetching web manifest: ${e}.`);

I think the DOMRequest should fire onerror in this case.

Comment 28

[Marcos Caceres [:marcosc]]

(In reply to Kan-Ru Chen [:kanru] from comment #27)
> Comment on attachment 8661029 [details] [diff] [review]
> Moved oop tests to right place
> 
> Review of attachment 8661029 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me.

Awesome! Thank you!!! ^_^

> ::: dom/browser-element/BrowserElementChildPreload.js
> @@ +1492,5 @@
> > +    if (hasManifest) {
> > +      try {
> > +        manifest = yield ManifestObtainer.contentObtainManifest(content);
> > +      } catch(e) {
> > +        debug(`Error fetching web manifest: ${e}.`);
> 
> I think the DOMRequest should fire onerror in this case.

Agree. Good catch!

Comment 29

[Marcos Caceres [:marcosc]]

Attachment: Final patch...

Comment 30

[Marcos Caceres [:marcosc]]

Comment on attachment 8661570 [details] [diff] [review]
Final patch...

> WebIDL file dom/webidl/BrowserElement.webidl altered in changeset 77878a277ea3 without DOM peer review

Bobby, can you please check if the idl is ok?

Comment 31

[Marcos Caceres [:marcosc]]

Attachment: Adds bholly to r list.

Comment 32

[Marcos Caceres [:marcosc]]

checkin-needed on attachment 8661613 [details] [diff] [review].

Comment 33

[Marcos Caceres [:marcosc]]

Updated try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c9836524a86

Comment 34

[Carsten Book [:Tomcat]]

Hi, when i tried to push this to inbound i got:

remote: Push rejected because the following IDL interfaces were
remote: modified without changing the UUID:
remote:   - nsIBrowserElementAPI in changeset 3659d7fc6e61
remote: 
remote: To update the UUID for all of the above interfaces and their
remote: descendants, run:
remote:   ./mach update-uuids nsIBrowserElementAPI
remote: 
remote: If you intentionally want to keep the current UUID, include
remote: 'IGNORE IDL' in the commit message.
remote: *************************************************************

is this expected ?

Comment 35

[Marcos Caceres [:marcosc]]

(In reply to Carsten Book [:Tomcat] from comment #34)
> Hi, when i tried to push this to inbound i got:
> 
> remote: Push rejected because the following IDL interfaces were
> remote: modified without changing the UUID:
> remote:   - nsIBrowserElementAPI in changeset 3659d7fc6e61
> remote: 
> remote: To update the UUID for all of the above interfaces and their
> remote: descendants, run:
> remote:   ./mach update-uuids nsIBrowserElementAPI
> remote: 
> remote: If you intentionally want to keep the current UUID, include
> remote: 'IGNORE IDL' in the commit message.
> remote: *************************************************************
> 
> is this expected ?

Strange, thought I'd done that in (comment 21). Will do it again.

Comment 36

[Marcos Caceres [:marcosc]]

Ah, I see what happened. There was two IDs to update, not one. 

Ted, take note above for you patch! :)

Comment 37

[Marcos Caceres [:marcosc]]

Attachment: Updated both UUIDs

Comment 38

[Ted Clancy [:tedders1]]

(In reply to Marcos Caceres [:marcosc] from comment #36)
> Ah, I see what happened. There was two IDs to update, not one. 
> 
> Ted, take note above for you patch! :)

Oh. Thanks, Marcos :-)

Comment 39

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47ccf6689101

Comment 40

[Pulsebot]

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

Comment 41

[Pulsebot]

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

Comment 42

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

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/9c45ef119f96 for bustage:

https://treeherder.mozilla.org/logviewer.html#?job_id=14275181&repo=mozilla-inbound

Comment 43

[Marcos Caceres [:marcosc]]

Sent to try again. Can't reproduce locally:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=073fccb62ea0
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2caea771a2a6

Comment 44

[marcos]

Kan-Run, I could use your help now please. I'm not sure how to proceed wrt the test failures above :(

Comment 45

[Kan-Ru Chen [:kanru] (UTC+9)]

Sorry, I'm still not sure how to reproduce the failures. I'm in a work week so probably won't be able to look into this until next week or so.

Comment 46

[Marcos Caceres [:marcosc]]

(In reply to Kan-Ru Chen [:kanru] (UTC-7) from comment #45)
> Sorry, I'm still not sure how to reproduce the failures. I'm in a work week
> so probably won't be able to look into this until next week or so.

Ok, chat next week.

Comment 47

[Marcos Caceres [:marcosc]]

ping

Comment 48

[Kan-Ru Chen [:kanru] (UTC+9)]

I can't reproduce locally so I pushed to try again.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=10c7e5328bc2

Sorry for the long delay.

Comment 49

[Kan-Ru Chen [:kanru] (UTC+9)]

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

Comment 50

[Kan-Ru Chen [:kanru] (UTC+9)]

It's a shame that I didn't catch this in review. Marcos, you need to wait the "testready" event before running any test code. It guarantees all permissions are set.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d34b56c5f67

Comment 51

[Marcos Caceres [:marcosc]]

(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #50)
> It's a shame that I didn't catch this in review. Marcos, you need to wait
> the "testready" event before running any test code. It guarantees all
> permissions are set.

Thank you sooooo much Kan-Ru... I'm so sorry you had to manually test every single line. I was imagining is was something in the C++ or something.

Comment 52

[Marcos Caceres [:marcosc]]

Attachment: Adds test fix by Kan-Ru

Comment 53

[Marcos Caceres [:marcosc]]

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4443e29aa656

Comment 54

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/29b8246feea7

Comment 55

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/29b8246feea7

Comment 56

[Ben Francis [:benfrancis]]

\o/

Comment 57

[Chris Mills [:cmills]]

Documented:

https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement/getManifest

I've added a note to the release notes too:

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Releases/2.5#Web_API_changes