Make XHR2 response/responseType work in Web Workers

RESOLVED FIXED in mozilla10

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: emk, Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

({dev-doc-complete})

unspecified
mozilla10
x86
Windows XP
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
At present, XHR2 response/responseType is not implemented in Web Workers.
It's be really nice to get this working for Fx7...
As things stand, this is very tricky to implement in workers. The problem is that the actual XHR object lives on the main thread, while the worker runs on a separate thread.

The problem with this is that the main thread will start receiving data before we have a chance to fire the readystatechange event for HEADERS_RECEIVED on the worker thread. And we don't want to block the main thread while running the worker code.

We could do some sort of worker specific data storing until the event has had a chance to fire in the worker, but even that is tricky to get exactly right.

However we do have a alternative plan which lets us run the XHR code on the worker thread. This should make this and a lot of other things a lot easier to implement, so I suggest that we wait until that is done.

I don't know if there's a bug for that part on file yet?
(Reporter)

Comment 3

6 years ago
(In reply to comment #2)
> However we do have a alternative plan which lets us run the XHR code on the
> worker thread. This should make this and a lot of other things a lot easier
> to implement, so I suggest that we wait until that is done.
> 
> I don't know if there's a bug for that part on file yet?
Bug 649537?

Comment 4

6 years ago
(In reply to comment #2)
> However we do have a alternative plan which lets us run the XHR code on the
> worker thread.
Just curious, what is that plan? Would we have to separate implementations of
XHR (I sure hope not) or would we handle cycle collector issue in some other way?
I'm asking this since the design could affect to other APIs which we should
implement for workers (websocket, indexeddb, eventsource, etc)
What's the status of this?
Duplicate of this bug: 675504

Comment 7

6 years ago
Created attachment 556899 [details] [diff] [review]
Initial implementation for responseType support

Still working on more tests; meanwhile please take a look to see if the implementation is correct
Attachment #556899 - Flags: review?(bent.mozilla)
Comment on attachment 556899 [details] [diff] [review]
Initial implementation for responseType support

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

::: dom/workers/XMLHttpRequestPrivate.cpp
@@ +874,5 @@
>    nsString mPassword;
>    bool mMultipart;
>    bool mBackgroundRequest;
>    bool mWithCredentials;
> +  nsString mResponseType;

Nit: move this up before the bools, after mPassword.

@@ +886,4 @@
>    : WorkerThreadProxySyncRunnable(aWorkerPrivate, aProxy), mMethod(aMethod),
>      mURL(aURL), mUser(aUser), mPassword(aPassword), mMultipart(aMultipart),
> +    mBackgroundRequest(aBackgroundRequest),
> +    mWithCredentials(aWithCredentials), mResponseType(aResponseType)

These changes should be reverted.

@@ +1396,5 @@
> +    return false;
> +  }
> +
> +  JSString* str = JS_ValueToString(aCx, *aVp);
> +  NS_ENSURE_TRUE(str, NS_ERROR_XPC_BAD_CONVERT_JS);

In worker-land we don't use nsresults really, and this functions is supposed to return bool. JS_ValueToString will set an exception on aCx if it returns false so just return false again.

@@ +1400,5 @@
> +  NS_ENSURE_TRUE(str, NS_ERROR_XPC_BAD_CONVERT_JS);
> +
> +  nsDependentJSString responseType;
> +  if (!responseType.init(aCx, str)) {
> +    return NS_ERROR_XPC_BAD_CONVERT_JS;

Same here, return false.

@@ +1406,5 @@
> +
> +  if (!mProxy) {
> +    mResponseType = responseType;
> +    return true;
> +  }

As discussed earlier we should throw if Open hasn't been called.

::: dom/workers/XMLHttpRequestPrivate.h
@@ +66,5 @@
>    bool mMultipart;
>    bool mBackgroundRequest;
>    bool mWithCredentials;
>    bool mCanceled;
> +  nsString mResponseType;

No need for this.

Comment 9

6 years ago
Created attachment 557257 [details] [diff] [review]
Patch 2 for responseType support
Attachment #556899 - Attachment is obsolete: true
Attachment #556899 - Flags: review?(bent.mozilla)
Attachment #557257 - Flags: review?(bent.mozilla)
Comment on attachment 557257 [details] [diff] [review]
Patch 2 for responseType support

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

This looks ok other than this:

::: dom/workers/XMLHttpRequestPrivate.cpp
@@ +1392,5 @@
> +    return false;
> +  }
> +
> +  JSString* str = JS_ValueToString(aCx, *aVp);
> +  NS_ENSURE_TRUE(str, NS_ERROR_XPC_BAD_CONVERT_JS);

See previous comments about using nsresults in worker code.

Updated

6 years ago
Assignee: nobody → shawn

Comment 11

6 years ago
Created attachment 562277 [details] [diff] [review]
*Unfinished* Implementation for 'response' in workers. Please see detailed description in comment 11

It looks like the XMLHttpRequestPrivate.cpp->EventRunnable->PreDispatch always get JSContext* aCx passed in with a null value. As a result, when we call mProxy->GetResponse() we are not able to get the actual response back from the main thread's nsXMLHttpRequest (GetResponseText doesn't require JSContext thus we can get the text value correctly). Please take a deeper look into that - thanks!! (I wish I could have more free time to dig into the problem, but unfortunately the current school work is very heavy)

Updated

6 years ago
Attachment #562277 - Attachment description: *Unfinished* Implementation for 'response' in workers. Please see detailed description in comments → *Unfinished* Implementation for 'response' in workers. Please see detailed description in comment 11
Created attachment 572231 [details] [diff] [review]
Patch, v1

This should finish up shawn's earlier work.

sicking can review most of the worker stuff.

peterv, please take a look at what I had to do in nsContentUtils.cpp and nsXMLHttpRequest.cpp. Basically we have the XHR object being used entirely in C++ (so, no JS wrapper is ever created), yet XHR calls PreserveWrapper to root some of its jsvals. I fixed it by making sure that nsContentUtils knows that a preserved wrapper can be null, and making sure that XHR removes itself from the root set when it dies. What do you think?

jorendorff, I added the JSVAL_IS_UNIVERSAL that you suggested, please only take a look at jsapi.h.
Assignee: hippopotamus.gong → bent.mozilla
Attachment #557257 - Attachment is obsolete: true
Attachment #562277 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #557257 - Flags: review?(bent.mozilla)
Attachment #572231 - Flags: review?(peterv)
Attachment #572231 - Flags: review?(jorendorff)
Attachment #572231 - Flags: review?(jonas)
Comment on attachment 572231 [details] [diff] [review]
Patch, v1

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

We also need to ensure that for "chunked" responsetypes we return null when the .response getter is called outside of a "progress" event handler. r- due to that.

::: dom/workers/XMLHttpRequestPrivate.cpp
@@ +1245,5 @@
>    }
>  
> +  nsIThreadJSContextStack* contextStack =
> +    nsContentUtils::ThreadJSContextStack();
> +  NS_ASSERTION(contextStack, "This should never be null!");

What does this change do? (i.e. the whole block)
Attachment #572231 - Flags: review?(jonas) → review-
Created attachment 572628 [details] [diff] [review]
Patch, v1.1

Fixed the chunked types, and fixed the threadstack thing.
Attachment #572231 - Attachment is obsolete: true
Attachment #572231 - Flags: review?(peterv)
Attachment #572231 - Flags: review?(jorendorff)
Attachment #572628 - Flags: review?(peterv)
Attachment #572628 - Flags: review?(jorendorff)
Attachment #572628 - Flags: review?(jonas)
Comment on attachment 572628 [details] [diff] [review]
Patch, v1.1

r=me on JSVAL_IS_UNIVERSAL.
Attachment #572628 - Flags: review?(jorendorff) → review+
Comment on attachment 572628 [details] [diff] [review]
Patch, v1.1

r=jst for the nsContentUtils changes.
Attachment #572628 - Flags: review?(peterv) → review+
Attachment #572628 - Flags: review?(jonas) → review+
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/29a1638f2e62
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
For documentation purpose: this is intended to be in Firefox 11, right?
It'll be in FF10
Target Milestone: --- → mozilla10
Thank you.

I've updated:
https://developer.mozilla.org/En/DOM/Worker/Functions_available_to_workers
and
https://developer.mozilla.org/en/Firefox_10_for_developers
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.