XMLHttpRequest should be disabled on ServiceWorkers

RESOLVED FIXED in Firefox 44

Status

()

RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: nsm, Assigned: Ehsan)

Tracking

(Blocks: 1 bug, {dev-doc-needed, site-compat})

unspecified
mozilla44
x86_64
Linux
dev-doc-needed, site-compat
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment 1

4 years ago
(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

4 years ago
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.
Attachment #8517806 - Flags: feedback?(nsm.nikhil)
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.
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 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.
Attachment #8517806 - Flags: feedback?(nsm.nikhil)
XHR is not exposed on ServiceWorkers at all - https://github.com/whatwg/xhr/issues/19
Summary: Sync XMLHttpRequest should be disabled on ServiceWorkers → XMLHttpRequest should be disabled on ServiceWorkers
(Assignee)

Updated

4 years ago
Assignee: nobody → ehsan
Keywords: dev-doc-needed
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1180746
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...
Attachment #8676249 - Flags: review?(bzbarsky) → review+
(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.
(Assignee)

Comment 12

4 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/d953a7e28421
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.