Masatoshi Kimura [:emk] Reporter
	Description • 13 years ago

At present, XHR2 response/responseType is not implemented in Web Workers.

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 13 years ago

It's be really nice to get this working for Fx7...

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 2 • 13 years ago

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?

	Masatoshi Kimura [:emk] Reporter
	Comment 3 • 13 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?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 4 • 13 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)

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 5 • 13 years ago

What's the status of this?

	Shawn Gong
	Comment 7 • 13 years ago

Still working on more tests; meanwhile please take a look to see if the implementation is correct

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 8 • 13 years ago

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.

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 10 • 13 years ago

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.

	Shawn Gong
	Comment 11 • 13 years ago

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)

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 12 • 13 years ago

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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 13 • 13 years ago

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)

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 14 • 13 years ago

Fixed the chunked types, and fixed the threadstack thing.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 15 • 13 years ago

Comment on attachment 572628 [details] [diff] [review]
Patch, v1.1

r=me on JSVAL_IS_UNIVERSAL.

	Johnny Stenback (:jst)
	Comment 16 • 13 years ago

Comment on attachment 572628 [details] [diff] [review]
Patch, v1.1

r=jst for the nsContentUtils changes.

	Ben Turner (not reading bugmail, use the needinfo flag!) Assignee
	Comment 17 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/29a1638f2e62

	Jean-Yves Perrier [:teoli]
	Comment 18 • 13 years ago

For documentation purpose: this is intended to be in Firefox 11, right?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 19 • 13 years ago

It'll be in FF10

	Jean-Yves Perrier [:teoli]
	Comment 20 • 13 years ago

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