[Browser API] getWebManifest()

RESOLVED FIXED in Firefox 45

Status

()

Core
DOM
P2
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: benfrancis, Assigned: marcosc)

Tracking

(Blocks: 3 bugs, {dev-doc-complete, feature})

unspecified
mozilla45
dev-doc-complete, feature
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

User Story

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).

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
Marcos, what do you think needs to be done to hook the Browser API up to your manifest implementation like this?
Flags: needinfo?(mcaceres)
(Assignee)

Comment 2

3 years ago
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.
Flags: needinfo?(mcaceres)
(Reporter)

Comment 3

3 years ago
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
Flags: needinfo?(tclancy)
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
This is something we'll need anyway in order to actually do updates of icons.
(Reporter)

Comment 6

3 years ago
> 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
Flags: needinfo?(mcaceres)
Flags: needinfo?(jonas)
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.
Flags: needinfo?(jonas)
(Assignee)

Comment 9

3 years ago
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.
Flags: needinfo?(mcaceres)
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?
Flags: needinfo?(bfrancis)
(Assignee)

Comment 11

3 years ago
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?
Flags: needinfo?(jonas)
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?
Flags: needinfo?(jonas)
(Assignee)

Comment 13

3 years ago
(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.
I can help with the implementation if we are clear how to go forward.
(Reporter)

Comment 15

3 years ago
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.
Flags: needinfo?(bfrancis)
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.
(Assignee)

Comment 17

3 years ago
(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.
(Assignee)

Updated

3 years ago
Summary: [Browser API] getManifest() → [Browser API] getWebManifest()
(Assignee)

Updated

3 years ago
Assignee: nobody → mcaceres
(Assignee)

Comment 18

3 years ago
Created attachment 8648244 [details] [diff] [review]
0001-Bug-1169633-Browser-API-getWebManifest.patch
Flags: needinfo?(tclancy)
(Assignee)

Comment 19

3 years ago
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.
Attachment #8648244 - Flags: feedback?(tclancy)
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.

Updated

2 years ago
Attachment #8648244 - Flags: feedback?(tclancy)
(Assignee)

Comment 21

2 years ago
Created attachment 8658423 [details] [diff] [review]
Updated UUID

Was busted because I forgot to update the IDL's UUID.
Attachment #8648244 - Attachment is obsolete: true
Attachment #8658423 - Flags: review?(kchen)
(Assignee)

Comment 22

2 years ago
Created attachment 8658438 [details] [diff] [review]
Fixed typo

Sorry, had a mistake in the last patch. FYI, writing tests in a separate patch :)
Attachment #8658423 - Attachment is obsolete: true
Attachment #8658423 - Flags: review?(kchen)
Attachment #8658438 - Flags: review?(kchen)
(Assignee)

Comment 23

2 years ago
Created attachment 8659656 [details] [diff] [review]
0001-Bug-1169633-Browser-API-getWebManifest-.-r-kanru.patch

Updated patch with tests
Attachment #8658438 - Attachment is obsolete: true
Attachment #8658438 - Flags: review?(kchen)
Attachment #8659656 - Flags: review?(kchen)
(Assignee)

Comment 25

2 years ago
ping
(Assignee)

Comment 26

2 years ago
Created attachment 8661029 [details] [diff] [review]
Moved oop tests to right place

Moved the oop tests to the right mochitest ini file.
Attachment #8659656 - Attachment is obsolete: true
Attachment #8659656 - Flags: review?(kchen)
Attachment #8661029 - Flags: review?(kchen)
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.
Attachment #8661029 - Flags: review?(kchen) → review+
(Assignee)

Comment 28

2 years ago
(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!
(Assignee)

Comment 29

2 years ago
Created attachment 8661570 [details] [diff] [review]
Final patch...
Attachment #8661029 - Attachment is obsolete: true
(Assignee)

Comment 30

2 years ago
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?
Attachment #8661570 - Flags: review?(bobbyholley)
Attachment #8661570 - Flags: review?(bobbyholley) → review+
(Assignee)

Comment 31

2 years ago
Created attachment 8661613 [details] [diff] [review]
Adds bholly to r list.
(Assignee)

Comment 32

2 years ago
checkin-needed on attachment 8661613 [details] [diff] [review].
Keywords: checkin-needed
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 ?
Flags: needinfo?(mcaceres)
Keywords: checkin-needed
(Assignee)

Comment 35

2 years ago
(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.
Flags: needinfo?(mcaceres)
(Assignee)

Comment 36

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

Ted, take note above for you patch! :)
(Assignee)

Comment 37

2 years ago
Created attachment 8661778 [details] [diff] [review]
Updated both UUIDs
Attachment #8661570 - Attachment is obsolete: true
Attachment #8661613 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(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 :-)

Updated

2 years ago
Flags: needinfo?(mcaceres)
Keywords: dev-doc-needed

Comment 44

2 years ago
Kan-Run, I could use your help now please. I'm not sure how to proceed wrt the test failures above :(
Flags: needinfo?(kchen)
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.
(Assignee)

Comment 46

2 years ago
(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.
Priority: -- → P2
(Assignee)

Comment 47

2 years ago
ping
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.
Flags: needinfo?(kchen)
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
Flags: needinfo?(mcaceres)
(Assignee)

Comment 51

2 years ago
(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.
Flags: needinfo?(mcaceres)
(Assignee)

Comment 52

2 years ago
Created attachment 8682443 [details] [diff] [review]
Adds test fix by Kan-Ru
Attachment #8661778 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 55

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/29b8246feea7
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Reporter)

Updated

2 years ago
Blocks: 1222157
(Reporter)

Comment 56

2 years ago
\o/
You need to log in before you can comment on or make changes to this bug.