1216736 - Write some tests to ensure fetch is safe to use from chrome script

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Mark Goodwin [:mgoodwin]]

The security state client (for OneCRL, HSTS preloads, etc) makes use of kinto.js - to keep variations between the different versions of kinto.js to a minimum, it's useful if the APIs it can use are the same, where possible, across platforms.

kinto.js uses fetch to sync with a kinto server. We need a fetch-like way of making this work.

Comment 1

[Mark Goodwin [:mgoodwin]]

Attachment: Bug1216736.patch

This creates a (thin) wrapper around XHR to provide a (minimal) fetch-like interface.

Comment 2

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=72f63377af1e

Comment 3

[Dave Townsend [:mossop]]

Perhaps a silly question, but why can't we just use Fetch?

Comment 4

[Mark Goodwin [:mgoodwin]]

(In reply to Dave Townsend [:mossop] from comment #3)
> Perhaps a silly question, but why can't we just use Fetch?

Not a silly question; I didn't think we could.

It took me a while to figure out, but...

Cu.importGlobalProperties(['fetch']);

...does the trick.  I have questions, though:

1) How does this work? A peek at the fetch webidl seems to suggest that this API isn't exported to System - what does 'Export' mean in this context
2) I see no-one else doing this outside of tests; should I check this is OK to use in this context, or do I just go ahead?

Comment 5

[Dave Townsend [:mossop]]

(In reply to Mark Goodwin [:mgoodwin] from comment #4)
> (In reply to Dave Townsend [:mossop] from comment #3)
> > Perhaps a silly question, but why can't we just use Fetch?
> 
> Not a silly question; I didn't think we could.
> 
> It took me a while to figure out, but...
> 
> Cu.importGlobalProperties(['fetch']);
> 
> ...does the trick.  I have questions, though:
> 
> 1) How does this work? A peek at the fetch webidl seems to suggest that this
> API isn't exported to System - what does 'Export' mean in this context

I'm not sure what "Export" you're referring to here. Cu.importGlobalProperties allows us to use things normally only exposed to webcontent in jsms and other sandboxes.

> 2) I see no-one else doing this outside of tests; should I check this is OK
> to use in this context, or do I just go ahead?

Andrea, is it ok for us to use Fetch like this?

Comment 6

[Mark Goodwin [:mgoodwin]]

(In reply to Dave Townsend [:mossop] from comment #5)
> I'm not sure what "Export" you're referring to here.

I meant 'expose'.

Comment 7

[Mark Goodwin [:mgoodwin]]

09:56 <mgoodwin> Hi, I was wondering if it's OK for me to use fetch from chrome JS
09:57 <baku> it's not exposed to system, so no.
09:57 <mgoodwin> Reason I ask; I see some places doing fetch-like things (e.g. in devtools) and rolling their own instead of using 
                 it
09:57 <baku> we could expose it... I don't see any reason why it's not.
09:57 <mgoodwin> Well it's not exposed but you can Cu.importGlobalProperties(['fetch']);
09:57 <mgoodwin> which works fine
09:58 <mgoodwin> I take it that exposing just add the magic glue so you don't have to do the importGlobalProperties step?
10:00 <baku> back
10:00 <baku> yes, we dont have tests about it.
10:00 <baku> so I cannot tell you if it's fully OK or not.
10:00 <baku> it should work, but yes, would be nice to test it properly.
10:00 <mgoodwin> OK. so I have some options.
10:00 <mgoodwin> 1) Just use it
10:01 <mgoodwin> 2) Write some tests, use it without exposing
10:01 <mgoodwin> 3) Write some tests *and* expose to system
10:02 <baku> I would start with 1, if it works-ish... do 2.
10:02 <mgoodwin> What's your preferred approach? Who should make the call on this?
10:02 <mgoodwin> Oh, it works fine.
10:03 <baku> thanks!
10:03 <mgoodwin> I wrote some tests for bug 1216736 (Where I wrote an alternative impl - you're ni? in that by the way)
10:04 <mgoodwin> I can include just the tests for the bits I need and document in the place where I importGlobal... that if others 
                 do the same they might want to add tests?
10:05 <baku> mgoodwin: sorry if I didn't answer yet, I'm a bit overloaded of NI :/
10:05 <mgoodwin> Yeah, I know people are busy. 
10:05 <baku> but yes, the best would be to have some basic tests
10:05 <mgoodwin> I'll paste this chat in IRC and clear the ni? for you :)
10:05 <baku> and import them in mochitests and chrome/browser tests
10:06 <mgoodwin> where should the test live? With the code that uses it or in some other component?
10:06 <baku> so far we have tests in dom/tests/fetch/
10:06 <mgoodwin> ok, thanks

Comment 8

[Mark Goodwin [:mgoodwin]]

Attachment: Bug1216736.patch

As a starting point, here are the tests I wrote before moved to dom/tests/unit and reworked to use Fetch via importGlobalProperties.

Comment 9

[Mark Goodwin [:mgoodwin]]

As per comment 8, this probably doesn't need to block bug 1197703

Comment 10

[Mark Goodwin [:mgoodwin]]

Attachment: MozReview Request: Bug 1216736 - Provide xpcshell tests for basic Fetch usage from chrome js r?baku

Bug 1216736 - Provide xpcshell tests for basic Fetch usage from chrome js r?baku

Comment 11

[Mark Goodwin [:mgoodwin]]

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

Comment 12

[Mark Goodwin [:mgoodwin]]

I've marked this is as blocking Bug 1197707 since I want to ensure we have test coverage before we're relying on this.

Comment 13

[Andrea Marchesini [:baku]]

Comment on attachment 8684199 [details]
MozReview Request: Bug 1216736 - Provide xpcshell tests for basic Fetch usage from chrome js r?baku

https://reviewboard.mozilla.org/r/24497/#review23243

::: dom/tests/unit/xpcshell.ini:3
(Diff revision 1)
>  tail = 

can you fix these 2 extra spaces?

Comment 14

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=31fd6e7aeab6

Comment 15

[Mark Goodwin [:mgoodwin]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae2ec0e9bc9ffa8417ab2e6f5672f955eba60e1c
Bug 1216736 - Provide xpcshell tests for basic Fetch usage from chrome js r=baku

Comment 16

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

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