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

RESOLVED FIXED in Firefox 45

Status

()

Toolkit
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mgoodwin, Assigned: mgoodwin)

Tracking

unspecified
mozilla45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

2 years ago
Created attachment 8676473 [details] [diff] [review]
Bug1216736.patch

This creates a (thin) wrapper around XHR to provide a (minimal) fetch-like interface.
Attachment #8676473 - Flags: review?(rnewman)
Perhaps a silly question, but why can't we just use Fetch?
Flags: needinfo?(mgoodwin)
(Assignee)

Comment 4

2 years ago
(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?
Flags: needinfo?(mgoodwin) → needinfo?(dtownsend)
(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?
Flags: needinfo?(dtownsend) → needinfo?(amarchesini)
(Assignee)

Comment 6

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

I meant 'expose'.
(Assignee)

Comment 7

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

Updated

2 years ago
Summary: Create a fetch-like wrapper for XHR for kinto.js (and other things) to use → Write some tests to ensure fetch is safe to use from chrome script
(Assignee)

Comment 8

2 years ago
Created attachment 8679421 [details] [diff] [review]
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.
Attachment #8676473 - Attachment is obsolete: true
Attachment #8676473 - Flags: review?(rnewman)
Attachment #8679421 - Flags: feedback?(amarchesini)
(Assignee)

Comment 9

2 years ago
As per comment 8, this probably doesn't need to block bug 1197703
No longer blocks: 1197703
Attachment #8679421 - Flags: feedback?(amarchesini) → feedback+
(Assignee)

Comment 10

2 years ago
Created attachment 8684199 [details]
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
Attachment #8684199 - Flags: review?(amarchesini)
(Assignee)

Comment 12

2 years ago
I've marked this is as blocking Bug 1197707 since I want to ensure we have test coverage before we're relying on this.
Blocks: 1197707
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?
Attachment #8684199 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 15

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

Updated

2 years ago
Attachment #8679421 - Attachment is obsolete: true

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ae2ec0e9bc9f
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.