1155898 - Expose fetch on JS sandbox

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Martin Thomson [:mt:]]

It's unfair that we only get to use the crappy XMLHttpRequest API in JS sandboxes.  Enabling fetch doesn't seem to be too intrusive.

I only wish that it were easier to test using xpcshell.  I need to work up a more rigorous test than what I have currently.  At least the WebRTC stuff works with the changes I've made.

Comment 1

[Martin Thomson [:mt:]]

Attachment: MozReview Request: bz://1155898/mt

/r/7229 - Bug 1155898 - Fetch support for running outside of window/worker
/r/7231 - Bug 1155898 - Expose fetch on JS sandbox
/r/7233 - Bug 1155898 - Use fetch instead of XHR for IdP

Pull down these commits:

hg pull -r fc9821779ff0b8de8d3274a0df8da278945fb6f9 https://reviewboard-hg.mozilla.org/gecko/

Comment 2

[Martin Thomson [:mt:]]

This should be fun:

https://hg.mozilla.org/try/pushloghtml?changeset=723bcd6fff76

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8594219 [details]
MozReview Request: bz://1155898/mt

Do you think that I need anyone else to look at this more carefully?

Comment 4

[Bobby Holley (:bholley)]

Can't this be done with Exposed= in the WebIDL?

Comment 5

[Martin Thomson [:mt:]]

If you know how to do that, I'd be happy to learn.  Is it as simple as [Exposed=System] ?

Comment 6

[Bobby Holley (:bholley)]

(In reply to Martin Thomson [:mt] from comment #5)
> If you know how to do that, I'd be happy to learn.  Is it as simple as
> [Exposed=System] ?

Oh hm, I guess we'd talked about it, but that would require bug 1137772 first. So nevermind, I guess.

Comment 7

[Gabor Krizsanits (INACTIVE)]

https://reviewboard.mozilla.org/r/7229/#review6031

Patches looks great! I have only a few nits, and I would like someone who is an expert in webidl to take a look at the second patch before we ship them...

::: dom/fetch/Fetch.cpp
(Diff revision 1)
> +    // and the principal is not null or the system principal.

or nsExpandedPrincipal...
basically if it has a meaningfull URI, note that system princiapl also returns null for the URI, but nullprincipal does not. So it's enough checking if it's not nullprincipal and has a non-null URI.

Comment 8

[Gabor Krizsanits (INACTIVE)]

https://reviewboard.mozilla.org/r/7231/#review6025

::: js/xpconnect/tests/unit/test_sandbox_fetch.js
(Diff revision 1)
> +    dump('(' + testFunc.toSource() + ')(' + propertyName + ');');

I don't think this dump should stay...

::: js/xpconnect/src/Sandbox.cpp
(Diff revision 1)
> +                       const MutableHandleValue& arg0)

Arg0 does not make much sence here as the name.

::: js/xpconnect/src/Sandbox.cpp
(Diff revision 1)
> +        return false;

In the false case what error does TrySetToRequest return?

::: js/xpconnect/src/Sandbox.cpp
(Diff revision 1)
> +    if (arg0.isObject() && !requestHolder.TrySetToRequest(cx, arg0, noMatch, false)) {

RequestOrUSVStringArgument seems like something only the code generator should use... I would really like some webidl expert to take a look at this.

::: js/xpconnect/src/Sandbox.cpp
(Diff revision 1)
> +        JS_ReportError(cx, "fetch requires a string or Request in argument 1");

I would prefer this function to just return false in case of any error and then its consumer to report the error. The 1st argument makes more sense in that context anyway then here. We could then also rename this function to something more generic, like SetRequestFromValue or SetFetchRequestFromValue.

::: js/xpconnect/tests/unit/test_sandbox_fetch.js
(Diff revision 1)
> +  var interfaceExists = o => ok(o);

Since these patches not just exposing the fetch to sandboxes but also enabling them to be used without a window/document, it would be great if we could add some more meaningfull test than just checking the existance of these properties.
(So we could guard against someone breaking the sandboxed version in the future)

Could you add some trivial test that checks functionality? Like adding a chrome test that creates a sandbox and runs http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/fetch/test_fetch_basic.js so basically adding a new driver function to fetch_test_framework.js and use it from a chrome mochi test?

Comment 9

[Gabor Krizsanits (INACTIVE)]

https://reviewboard.mozilla.org/r/7233/#review6029

This looks good to me but please remove the old code instead of commenting it out.

::: dom/media/tests/mochitest/identity/idp.js
(Diff revision 1)
> +/*      var xhr = new XMLHttpRequest();

Is there a reason not to remove this code?

Comment 10

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8594219 [details]
MozReview Request: bz://1155898/mt

(In reply to Martin Thomson [:mt] from comment #3)
> Do you think that I need anyone else to look at this more carefully?

I guess that someone is Bobby, who already did that :) For the second patch I would like either him or Boris or someone else who is an expert in webidl to take a closer look at. I'm not sure if we should use some of these types/methods you are using outside of the codegen... so better ask than sorry :)

Comment 11

[Martin Thomson [:mt:]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #8)
> > +    if (arg0.isObject() && !requestHolder.TrySetToRequest(cx, arg0, noMatch, false)) {
> 
> RequestOrUSVStringArgument seems like something only the code generator
> should use... I would really like some webidl expert to take a look at this.

I will ask.  I'm really only cargo-culting some of this, so actual expert input will be needed.  Honestly, I'm surprised that something this hacktastic even worked.
 
> ::: js/xpconnect/src/Sandbox.cpp
> (Diff revision 1)
> > +        JS_ReportError(cx, "fetch requires a string or Request in argument 1");
> 
> I would prefer this function to just return false in case of any error and
> then its consumer to report the error. The 1st argument makes more sense in
> that context anyway then here. We could then also rename this function to
> something more generic, like SetRequestFromValue or SetFetchRequestFromValue.

Yes, I have moved that to the calling scope; and the name suggestion is great, thanks.

> ::: js/xpconnect/tests/unit/test_sandbox_fetch.js
> (Diff revision 1)
> > +  var interfaceExists = o => ok(o);
> 
> Since these patches not just exposing the fetch to sandboxes but also
> enabling them to be used without a window/document, it would be great if we
> could add some more meaningfull test than just checking the existance of
> these properties.
> (So we could guard against someone breaking the sandboxed version in the
> future)

Yes, I have that test, and will upload it.  It's a regular mochitest.  I'll probably remove the stubbed test at the same time.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #9)
> https://reviewboard.mozilla.org/r/7233/#review6029
> 
> This looks good to me but please remove the old code instead of commenting
> it out.

Yeah, that's just a mistake; I was switching back and forth, looking at how XHR and fetch differ in practice.

> (In reply to Martin Thomson [:mt] from comment #3)
> > Do you think that I need anyone else to look at this more carefully?
> 
> I guess that someone is Bobby, who already did that :) For the second patch
> I would like either him or Boris or someone else who is an expert in webidl
> to take a closer look at. I'm not sure if we should use some of these
> types/methods you are using outside of the codegen... so better ask than
> sorry :)

I will do.

(BTW, I notice that your RB comments only ever reference a single line.  Did you know that you can select more lines of context by dragging the mouse over the lines, rather than clicking?)

Comment 12

[Martin Thomson [:mt:]]

Comment on attachment 8594219 [details]
MozReview Request: bz://1155898/mt

/r/7229 - Bug 1155898 - Fetch support for running outside of window/worker
/r/7231 - Bug 1155898 - Expose fetch on JS sandbox
/r/7233 - Bug 1155898 - Use fetch instead of XHR for IdP

Pull down these commits:

hg pull -r 602ffb301e2fe3870f403ddb2d61306049ca4061 https://reviewboard-hg.mozilla.org/gecko/

Comment 13

[Martin Thomson [:mt:]]

nsm, do you think that this is worth opening a separate bug for?

I've commented out the line in test_sandbox_fetch.html that triggers this dump, it's the blob test here:
https://dxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/fetch/test_fetch_basic.js#38

     |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
                                              Per-Inst   Leaked    Total      Rem
   0 TOTAL                                          30      768  1074118       16
 136 DataOwner                                      48       48        1        1
 175 FileImpl                                       24       48        2        2
 176 FileImplMemory                                120      120        1        1
 290 MultipartFileImpl                             128      128        1        1
 493 WeakReference<MessageListener>                 16       16      265        1
 597 nsAuthURLParser                                24       24        2        1
 607 nsBasePrincipal                                24       24      311        1
 950 nsPrincipal                                    64       64      311        1
1012 nsStandardURL                                 248      248     7108        1
1022 nsStringBuffer                                  8       24    64550        3
1064 nsTArray_base                                   8       24   204039        3

Adding a call to Cu.nukeSandbox() doesn't fix the problem, sadly.

I see the leak of a weak reference to a MessageListener all the time, so you can probably ignore that one.

Comment 14

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://reviewboard.mozilla.org/r/7229/#review6091

::: dom/fetch/Fetch.cpp:239
(Diff revision 2)
> +      loadGroup = do_CreateInstance(NS_LOADGROUP_CONTRACTID);

The loadgroup should ideally be associated with the principal. You can obtain one by using https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsNetUtil.h#830

::: dom/fetch/FetchDriver.cpp
(Diff revision 2)
> -  MOZ_ASSERT(mLoadGroup);

why was this assertion removed?

Comment 15

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://reviewboard.mozilla.org/r/7229/#review6093

Ship It!

Comment 16

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://reviewboard.mozilla.org/r/7229/#review6095

This should be 'fix it, then ship it'

Comment 17

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8594219 [details]
MozReview Request: bz://1155898/mt

it seems like reviewboard does not integrate with feedback? flag yet.

Comment 18

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Inside the fetch handler, can you try calling URL.revokeObjectURL(url) and see if it stops leaking? That way we'll know where the problem is. In windows+documents, the document is aware of such urls backed by blobs and releases them https://dxr.mozilla.org/mozilla-central/source/dom/base/URL.cpp#175. You'd have to add similar functionality to sandbox, or prohibit the sandbox from using createObjectURL for now.

Comment 19

[Gabor Krizsanits (INACTIVE)]

(In reply to Martin Thomson [:mt] from comment #11) 
> (BTW, I notice that your RB comments only ever reference a single line.  Did
> you know that you can select more lines of context by dragging the mouse
> over the lines, rather than clicking?)

Nope, I was not aware of it, in fact it kind of bothered me, so thanks a lot! :)

Comment 20

[Martin Thomson [:mt:]]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #18)
> Inside the fetch handler, can you try calling URL.revokeObjectURL(url) and
> see if it stops leaking? That way we'll know where the problem is. In
> windows+documents, the document is aware of such urls backed by blobs and
> releases them
> https://dxr.mozilla.org/mozilla-central/source/dom/base/URL.cpp#175. You'd
> have to add similar functionality to sandbox, or prohibit the sandbox from
> using createObjectURL for now.

It looks like that is the problem.  I'm going to open a second bug for that.  It should be easy enough to attach something like that to SandboxPrivate, but I'm concerned that we're just punting the problem if we do that: won't workers have the same problem?

Comment 21

[Martin Thomson [:mt:]]

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

Comment 22

[Martin Thomson [:mt:]]

Comment on attachment 8594219 [details]
MozReview Request: bz://1155898/mt

/r/7229 - Bug 1155898 - Fetch support for running outside of window/worker
/r/7231 - Bug 1155898 - Expose fetch on JS sandbox
/r/7233 - Bug 1155898 - Use fetch instead of XHR for IdP

Pull down these commits:

hg pull -r 5ebab881fbc2a387c633c9b0177a290cb4c377c8 https://reviewboard-hg.mozilla.org/gecko/

Comment 23

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/7233/#review6139

Sweet! (I got the easy review)

Comment 24

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Martin Thomson [:mt] from comment #20)
> (In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #18)
> > Inside the fetch handler, can you try calling URL.revokeObjectURL(url) and
> > see if it stops leaking? That way we'll know where the problem is. In
> > windows+documents, the document is aware of such urls backed by blobs and
> > releases them
> > https://dxr.mozilla.org/mozilla-central/source/dom/base/URL.cpp#175. You'd
> > have to add similar functionality to sandbox, or prohibit the sandbox from
> > using createObjectURL for now.
> 
> It looks like that is the problem.  I'm going to open a second bug for that.
> It should be easy enough to attach something like that to SandboxPrivate,
> but I'm concerned that we're just punting the problem if we do that: won't
> workers have the same problem?

workers also seem to have hooks like documents - https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#4110

Comment 25

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://reviewboard.mozilla.org/r/7229/#review6167

Ship It!

Comment 26

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8594219 [details]
MozReview Request: bz://1155898/mt

https://reviewboard.mozilla.org/r/7227/#review6169

Ship It!

Comment 27

[Gabor Krizsanits (INACTIVE)]

https://reviewboard.mozilla.org/r/7231/#review6247

::: js/xpconnect/tests/mochitest/test_sandbox_fetch.html:51
(Diff revision 3)
> +    SpecialPowers.Cu.nukeSandbox(sb);

Why do we need to nuke the sandbox here?

::: js/xpconnect/src/Sandbox.cpp:275
(Diff revision 3)
> +        FetchRequest(global, Constify(request), Constify(options), rv);
> +    rv.WouldReportJSException();
> +    if (rv.Failed()) {
> +        return ThrowMethodFailedWithDetails(cx, rv, "Sandbox", "fetch");
> +    }

Not sure if we should use ThrowMethodFailedWithDetails here... Probably not... but then again I let Peter sort out what API of these binging internals should or should not be used externally. I wish there were an easier way to export functions to sandboxes like there is for ctors (::GetConstructorObject).

Comment 28

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8594219 [details]
MozReview Request: bz://1155898/mt

https://reviewboard.mozilla.org/r/7227/#review6249

Ship It!

Comment 29

[Martin Thomson [:mt:]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #27)
> https://reviewboard.mozilla.org/r/7231/#review6247
> 
> ::: js/xpconnect/tests/mochitest/test_sandbox_fetch.html:51
> (Diff revision 3)
> > +    SpecialPowers.Cu.nukeSandbox(sb);
> 
> Why do we need to nuke the sandbox here?

I added this when investigating the leak that this test exposed.  It's not really needed.  I'll remove it.

> ::: js/xpconnect/src/Sandbox.cpp:275
> (Diff revision 3)
> > +        FetchRequest(global, Constify(request), Constify(options), rv);
> > +    rv.WouldReportJSException();
> > +    if (rv.Failed()) {
> > +        return ThrowMethodFailedWithDetails(cx, rv, "Sandbox", "fetch");
> > +    }
> 
> Not sure if we should use ThrowMethodFailedWithDetails here... Probably
> not... but then again I let Peter sort out what API of these binging
> internals should or should not be used externally. I wish there were an
> easier way to export functions to sandboxes like there is for ctors
> (::GetConstructorObject).

I couldn't find one.  But that's why I'm asking Peter for review.  I really don't know this code well enough.

Comment 30

[Peter Van der Beken [:peterv]]

https://reviewboard.mozilla.org/r/7231/#review7579

::: js/xpconnect/src/Sandbox.cpp:233
(Diff revision 3)
> +                                const MutableHandleValue& requestOrUrl)

Fix up whitespace.

Comment 31

[Peter Van der Beken [:peterv]]

https://reviewboard.mozilla.org/r/7231/#review7585

Ship It!

Comment 32

[Martin Thomson [:mt:]]

Comment on attachment 8594219 [details]
MozReview Request: bz://1155898/mt

/r/7229 - Bug 1155898 - Fetch support for running outside of window/worker, r=nsm
/r/7231 - Bug 1155898 - Expose fetch on JS sandbox, r=gabor,peterv
/r/7233 - Bug 1155898 - Use fetch instead of XHR for IdP, r=jib

Pull down these commits:

hg pull -r 65c1b06068e7e1a2c4bdd27a521459247bbff83b https://reviewboard-hg.mozilla.org/gecko/

Comment 33

[Martin Thomson [:mt:]]

Comment on attachment 8594219 [details]
MozReview Request: bz://1155898/mt

Carrying r+

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ef9cce1a775
https://hg.mozilla.org/integration/mozilla-inbound/rev/e42c9f4794d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/21e041962894

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Backed out for test_sandbox_fetch.html failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=10002262&repo=mozilla-inbound

This looks relevant:
08:30:34 INFO - JavaScript error: http://mochi.test:8888/tests/js/xpconnect/tests/mochitest/test_sandbox_fetch.html, line 28: ReferenceError: resolvePromise is not defined

Comment 36

[Pulsebot]

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

Comment 37

[Martin Thomson [:mt:]]

Comment on attachment 8594219 [details]
MozReview Request: bz://1155898/mt

/r/7229 - Bug 1155898 - Fetch support for running outside of window/worker, r=nsm
/r/7231 - Bug 1155898 - Expose fetch on JS sandbox, r=gabor,peterv
/r/7233 - Bug 1155898 - Use fetch instead of XHR for IdP, r=jib

Pull down these commits:

hg pull -r cfdc83a79f6ad2c0b389385a3eabc4a3d4a0e6e3 https://reviewboard-hg.mozilla.org/gecko/

Comment 38

[Martin Thomson [:mt:]]

Comment on attachment 8594219 [details]
MozReview Request: bz://1155898/mt

https://reviewboard.mozilla.org/r/7227/#review7789

Ship It!

Comment 39

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/72346a679254
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccb70668319c
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c600e4bc66

Comment 40

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

https://hg.mozilla.org/mozilla-central/rev/72346a679254
https://hg.mozilla.org/mozilla-central/rev/ccb70668319c
https://hg.mozilla.org/mozilla-central/rev/d7c600e4bc66

Comment 42

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1155898 - Use fetch instead of XHR for IdP, r=jib

Comment 43

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1155898 - Fetch support for running outside of window/worker, r=nsm

Comment 44

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1155898 - Expose fetch on JS sandbox, r=gabor,peterv

Comment 45

[Wladimir Palant]

I added this to https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Language_Bindings/Components.utils.Sandbox#options, that single line in the table should be sufficient.