Last Comment Bug 658178 - Make XHR2 response/responseType work in Web Workers
: Make XHR2 response/responseType work in Web Workers
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86 Windows XP
: -- normal with 1 vote (vote)
: mozilla10
Assigned To: Ben Turner (not reading bugmail, use the needinfo flag!)
:
Mentors:
: 675504 (view as bug list)
Depends on:
Blocks: 649133
  Show dependency treegraph
 
Reported: 2011-05-18 22:35 PDT by Masatoshi Kimura [:emk]
Modified: 2011-11-19 23:13 PST (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Initial implementation for responseType support (10.50 KB, patch)
2011-08-30 10:23 PDT, Shawn Gong
no flags Details | Diff | Splinter Review
Patch 2 for responseType support (10.72 KB, patch)
2011-08-31 11:20 PDT, Shawn Gong
no flags Details | Diff | Splinter Review
*Unfinished* Implementation for 'response' in workers. Please see detailed description in comment 11 (13.43 KB, patch)
2011-09-24 23:10 PDT, Shawn Gong
no flags Details | Diff | Splinter Review
Patch, v1 (32.71 KB, patch)
2011-11-05 14:02 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
jonas: review-
Details | Diff | Splinter Review
Patch, v1.1 (35.91 KB, patch)
2011-11-07 14:45 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
jonas: review+
jst: review+
mrbkap: review+
Details | Diff | Splinter Review

Description Masatoshi Kimura [:emk] 2011-05-18 22:35:55 PDT
At present, XHR2 response/responseType is not implemented in Web Workers.
Comment 1 Boris Zbarsky [:bz] (TPAC) 2011-05-18 22:42:53 PDT
It's be really nice to get this working for Fx7...
Comment 2 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-18 22:46:33 PDT
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?
Comment 3 Masatoshi Kimura [:emk] 2011-05-19 01:06:42 PDT
(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 Olli Pettay [:smaug] (TPAC) 2011-05-19 02:48:45 PDT
(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)
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-23 01:37:25 PDT
What's the status of this?
Comment 6 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-07-31 11:44:09 PDT
*** Bug 675504 has been marked as a duplicate of this bug. ***
Comment 7 Shawn Gong 2011-08-30 10:23:57 PDT
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
Comment 8 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-08-31 11:19:07 PDT
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 Shawn Gong 2011-08-31 11:20:40 PDT
Created attachment 557257 [details] [diff] [review]
Patch 2 for responseType support
Comment 10 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-08-31 11:29:47 PDT
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.
Comment 11 Shawn Gong 2011-09-24 23:10:11 PDT
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)
Comment 12 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-05 14:02:10 PDT
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.
Comment 13 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-11-06 17:37:51 PST
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)
Comment 14 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-07 14:45:54 PST
Created attachment 572628 [details] [diff] [review]
Patch, v1.1

Fixed the chunked types, and fixed the threadstack thing.
Comment 15 Blake Kaplan (:mrbkap) 2011-11-07 14:51:46 PST
Comment on attachment 572628 [details] [diff] [review]
Patch, v1.1

r=me on JSVAL_IS_UNIVERSAL.
Comment 16 Johnny Stenback (:jst, jst@mozilla.com) 2011-11-07 16:17:57 PST
Comment on attachment 572628 [details] [diff] [review]
Patch, v1.1

r=jst for the nsContentUtils changes.
Comment 17 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-07 17:10:40 PST
https://hg.mozilla.org/mozilla-central/rev/29a1638f2e62
Comment 18 Jean-Yves Perrier [:teoli] 2011-11-19 10:46:16 PST
For documentation purpose: this is intended to be in Firefox 11, right?
Comment 19 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-11-19 11:57:21 PST
It'll be in FF10

Note You need to log in before you can comment on or make changes to this bug.