931243 - XMLHttpRequest should be disabled on ServiceWorkers

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

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

See https://github.com/slightlyoff/ServiceWorker/blob/master/advanced_topics.md

Comment 1

[Jefry Lagrange]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #0)
> See
> https://github.com/slightlyoff/ServiceWorker/blob/master/advanced_topics.md


That link is broken.

Comment 2

[Jefry Lagrange]

Attachment: disable Synchronous XMLHttpRequest in ServiceWorkers

I made some changes. The when open is called from the proxy object that glues workers XMLHttpRequest to the main thread XMLHttpRequest, it sets the async option to true if it is a serviceWorker. Am I on the right track here? 


Also, I don't know how to test this directly. I was thinking of registering a serviceWorker with the synchronous option to true and the test whether it blocked a timeout or not. But that seems to me a roundabout way of doing it. Let me know what you think.

Comment 3

[Masatoshi Kimura [:emk]]

Why is the sync option being removed from workers (i.e. non-main threads)? Is FileReaderSync also deprecated in ServiceWorkers?
Where is the spec about this? The link is broken as you said.

Comment 4

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

Please file a spec issue at https://github.com/slightlyoff/ServiceWorker/issues about whether this topic that was discussed several months ago is something we want to enforce.

Comment 5

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

Comment on attachment 8517806 [details] [diff] [review]
disable Synchronous XMLHttpRequest in ServiceWorkers

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

::: dom/workers/XMLHttpRequest.cpp
@@ +1913,5 @@
>      return;
>    }
>  
> +  if (!aAsync && mWorkerPrivate->IsServiceWorker()){
> +    aAsync = true;

We don't want to do this, doing something unexpected. The spec issue is where discussion should happen, but my suggestion (and feel free to put this in the issue) is that we throw a AbortError here, along with a devtools warning.

Comment 6

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

XHR is not exposed on ServiceWorkers at all - https://github.com/whatwg/xhr/issues/19

Comment 9

[(no longer active)]

Attachment: Remove the XMLHttpRequest APIs from ServiceWorkerGlobalScope

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8676249 [details] [diff] [review]
Remove the XMLHttpRequest APIs from ServiceWorkerGlobalScope

>+            assert_equals(e.data.xhr, false);

assert_false(e.data.xhr) is more idiomatic, right?

r=me, though we'll see if people end up crying out for this in service workers...

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Boris Zbarsky [:bz] from comment #10)
> r=me, though we'll see if people end up crying out for this in service
> workers...

XHR ergonomics are heavily penalized in service workers compared to fetch().  An XHR would have to read into an arraybuffer and then do new Response() in order to intercept.  Its much easier just to use fetch() directly.

Comment 12

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #11)
> (In reply to Boris Zbarsky [:bz] from comment #10)
> > r=me, though we'll see if people end up crying out for this in service
> > workers...
> 
> XHR ergonomics are heavily penalized in service workers compared to fetch().
> An XHR would have to read into an arraybuffer and then do new Response() in
> order to intercept.  Its much easier just to use fetch() directly.

Well, for the record, while that is true, removing XHR has a huge downside of making all of the existing libraries that use XHR useless for using inside service workers.  I wouldn't be surprised at all that we would have to add it back to service workers at some point because of this reason, but I guess it's easier to add it later than take it away.

Comment 13

[Pulsebot]

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

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/xmlhttprequest-is-no-longer-available-in-service-workers/