Last Comment Bug 678432 - Sending arraybuffers using XMLHttpRequest.send
: Sending arraybuffers using XMLHttpRequest.send
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla9
Assigned To: Shawn Gong
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-11 20:07 PDT by Shawn Gong
Modified: 2011-11-15 09:40 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch 1. Implemented the feature (13.10 KB, patch)
2011-08-11 20:41 PDT, Shawn Gong
jonas: review-
Details | Diff | Splinter Review
Patch 2. (5.93 KB, patch)
2011-08-17 19:06 PDT, Shawn Gong
jonas: review+
Details | Diff | Splinter Review
Patch 3. (5.90 KB, patch)
2011-08-18 13:23 PDT, Shawn Gong
no flags Details | Diff | Splinter Review
Changed to call JS_GetArrayBufferDataLength() which is an exposed API. (5.38 KB, patch)
2011-08-22 16:29 PDT, Shawn Gong
no flags Details | Diff | Splinter Review

Description Shawn Gong 2011-08-11 20:07:39 PDT
Implement the ability for XMLHttpRequest.send to take in arraybuffer data.
Comment 1 Shawn Gong 2011-08-11 20:41:23 PDT
Created attachment 552589 [details] [diff] [review]
Patch 1. Implemented the feature
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-12 04:59:37 PDT
Comment on attachment 552589 [details] [diff] [review]
Patch 1. Implemented the feature

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

Some drive by comments.

I'm wary of only making this work on the main thread though.

::: content/base/public/nsIXMLHttpRequest.idl
@@ +262,1 @@
>    void   send([optional] in nsIVariant body);

You need to change the uuid.

::: content/base/src/nsXMLHttpRequest.cpp
@@ +2091,5 @@
> +      if (NS_SUCCEEDED(rv) && !JSVAL_IS_PRIMITIVE(realVal) &&
> +          JS_WrapValue(aCx, &realVal) && JSVAL_IS_OBJECT(realVal)) {
> +
> +        JSObject* obj = JSVAL_TO_OBJECT(realVal);
> +        if (js_IsArrayBuffer(obj)) {

Can you just return early here?

e.g.

if (!js_IsArrayBuffer(obj))
  return NS_ERROR_UNEXPECTED;

@@ +2093,5 @@
> +
> +        JSObject* obj = JSVAL_TO_OBJECT(realVal);
> +        if (js_IsArrayBuffer(obj)) {
> +          PRInt32 abLength = js::ArrayBuffer::getByteLength(obj);
> +          char* data = static_cast<char*>(NS_Alloc(abLength + 1));

The manual memory management here is more than a little ugly.  Why are you using NS_Alloc instead of just new, and can we use an nsAutoPtr instead of a raw ptr?

@@ +2100,5 @@
> +          }
> +          char* p = data;
> +          char* iter = (char*)JS_GetArrayBufferData(obj);
> +          for (PRInt32 i = 0; i < abLength; i++) {
> +            if (*iter & 0xFF00) {

Why are we picking out this one character in particular?  This needs comments if nothing else.

@@ +2105,5 @@
> +              NS_Free(data);
> +              return NS_ERROR_DOM_INVALID_CHARACTER_ERR;
> +            }
> +            *p++ = static_cast<char>(*iter++);
> +          }

Implementing our own memcpy kinda sucks.

@@ +2110,5 @@
> +          *p = '\0';
> +
> +          nsCOMPtr<nsIInputStream> stream;
> +          nsresult rv = NS_NewByteInputStream(getter_AddRefs(stream), data,
> +                                              abLength, NS_ASSIGNMENT_ADOPT);

Ok, are you using NS_Alloc so to get data to use the right allocator?

@@ +2117,5 @@
> +          }
> +          NS_ENSURE_SUCCESS(rv, rv);
> +
> +          if (stream) {
> +            *aResult = stream.forget().get();

stream.forget(aResult);

::: content/base/test/Makefile.in
@@ +69,5 @@
>  # Split files arbitrarily in two groups to not run into too-long command lines
>  # which break on Windows (see bug 563151)
> +_TEST_FILES1 = \
> +    handleArrayBuffer.sjs \
> +    test_bug5141.html \

As much as I hate them, use tabs to get the spacing right.

::: content/base/test/test_XHR.html
@@ +1,5 @@
>  <!DOCTYPE HTML>
>  <html>
>  <head>
>    <title>Test for XMLHttpRequest</title>
> +  <script type="text/javascript" src="handleArrayBuffer.sjs"></script>        

Why is this here?

::: dom/workers/XMLHttpRequestPrivate.cpp
@@ +978,5 @@
>          NS_ERROR("This should never fail!");
>        }
>      }
>  
> +    nsresult rv = mProxy->mXHR->Send(variant, nsnull);

So this won't work in workers?
Comment 3 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-12 04:59:59 PDT
Roping in bent to think about how to do this in workers.
Comment 4 Shawn Gong 2011-08-16 16:05:01 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #2)

Long time no see Kyle (:

> I'm wary of only making this work on the main thread though.

Yes the plan is to first carry out the main thread functionality then consider the worker thread implementation.

> You need to change the uuid.

Good catch!

> ::: content/base/src/nsXMLHttpRequest.cpp
> @@ +2091,5 @@
> > +      if (NS_SUCCEEDED(rv) && !JSVAL_IS_PRIMITIVE(realVal) &&
> > +          JS_WrapValue(aCx, &realVal) && JSVAL_IS_OBJECT(realVal)) {
> > +
> > +        JSObject* obj = JSVAL_TO_OBJECT(realVal);
> > +        if (js_IsArrayBuffer(obj)) {
> 
> Can you just return early here?
> 
> e.g.
> 
> if (!js_IsArrayBuffer(obj))
>   return NS_ERROR_UNEXPECTED;

I think everything that does not fit the type checks goes into a plain string? That's why I didn't return. What do you think?

> @@ +2100,5 @@
> > +          }
> > +          char* p = data;
> > +          char* iter = (char*)JS_GetArrayBufferData(obj);
> > +          for (PRInt32 i = 0; i < abLength; i++) {
> > +            if (*iter & 0xFF00) {
> 
> Why are we picking out this one character in particular?  This needs
> comments if nothing else.

This one came from SendAsBinary; I think that's a mask to check if we have characters whose encoding is out of the range.

> @@ +2093,5 @@
> > +
> > +        JSObject* obj = JSVAL_TO_OBJECT(realVal);
> > +        if (js_IsArrayBuffer(obj)) {
> > +          PRInt32 abLength = js::ArrayBuffer::getByteLength(obj);
> > +          char* data = static_cast<char*>(NS_Alloc(abLength + 1));
> 
> The manual memory management here is more than a little ugly.  Why are you
> using NS_Alloc instead of just new, and can we use an nsAutoPtr instead of a
> raw ptr?
>
> @@ +2105,5 @@
> > +              NS_Free(data);
> > +              return NS_ERROR_DOM_INVALID_CHARACTER_ERR;
> > +            }
> > +            *p++ = static_cast<char>(*iter++);
> > +          }
> 
> Implementing our own memcpy kinda sucks.
> 
> @@ +2110,5 @@
> > +          *p = '\0';
> > +
> > +          nsCOMPtr<nsIInputStream> stream;
> > +          nsresult rv = NS_NewByteInputStream(getter_AddRefs(stream), data,
> > +                                              abLength, NS_ASSIGNMENT_ADOPT);
> 
> Ok, are you using NS_Alloc so to get data to use the right allocator?

Jonas said he will comment later on this issue.

> > +          if (stream) {
> > +            *aResult = stream.forget().get();
> 
> stream.forget(aResult);

Great.

> > +    handleArrayBuffer.sjs \
> > +    test_bug5141.html \
> As much as I hate them, use tabs to get the spacing right.

Right.

> ::: content/base/test/test_XHR.html
> > +  <script type="text/javascript" src="handleArrayBuffer.sjs"></script>        
> 
> Why is this here?

Because we need to send arraybuffer to this server-side script and check if we sent the correct data.

> ::: dom/workers/XMLHttpRequestPrivate.cpp
> > +    nsresult rv = mProxy->mXHR->Send(variant, nsnull);
> 
> So this won't work in workers?

Workers is the next step (:

Thanks a lot Kyle!
Comment 5 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-08-16 17:12:00 PDT
Comment on attachment 552589 [details] [diff] [review]
Patch 1. Implemented the feature

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

Rather than adding to test_XHR.html, add to test_XHRSendData.html, that is closer to what you want. Grab me on irc and I'll show how to do it.

::: content/base/src/nsXMLHttpRequest.cpp
@@ +2082,5 @@
>      if (sendable) {
>        return sendable->GetSendInfo(aResult, aContentType, aCharset);
>      }
> +
> +    if (aCx) {

Actually, I don't think you need to use a context here at all. You don't appear to be using it.

Also, put the arraybuffer checks after all the existing checks. That way we have all the tests for d

@@ +2088,5 @@
> +      jsval realVal;
> +      nsresult rv = aBody->GetAsJSVal(&realVal);
> +
> +      if (NS_SUCCEEDED(rv) && !JSVAL_IS_PRIMITIVE(realVal) &&
> +          JS_WrapValue(aCx, &realVal) && JSVAL_IS_OBJECT(realVal)) {

I don't think you need the JS_WrapValue here.

Also, you don't need to check both !JSVAL_IS_PRIMITIVE and JSVAL_IS_OBJECT. They basically check the same thing. Just do the !JSVAL_IS_PRIMITIVE check.

I'd also combine this with the js_IsArrayBuffer test as to create less nesting. You can write

if (NS_SUCCEEDED(rv) && !JSVAL_IS_PRIMITIVE(realVal) &&
    (obj = JSVAL_TO_OBJECT(realVal)) &&
    js_IsArrayBuffer(obj)) {

(And it's good that you're not early-returning here as we want to go through the catch-all code at the end of the function)

@@ +2105,5 @@
> +              NS_Free(data);
> +              return NS_ERROR_DOM_INVALID_CHARACTER_ERR;
> +            }
> +            *p++ = static_cast<char>(*iter++);
> +          }

No need to do the memory copying yourself at all, just pass NS_ASSIGNMENT_COPY to the NS_NewByteInputStream function and it will take care of copying the data.
Comment 6 Shawn Gong 2011-08-17 19:06:52 PDT
Created attachment 553980 [details] [diff] [review]
Patch 2.
Comment 7 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-08-18 01:37:12 PDT
Comment on attachment 553980 [details] [diff] [review]
Patch 2.

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

r=me with that fixed.

::: content/base/src/nsXMLHttpRequest.cpp
@@ +2089,5 @@
> +    if (NS_SUCCEEDED(rv) && !JSVAL_IS_PRIMITIVE(realVal) &&
> +        (obj = JSVAL_TO_OBJECT(realVal)) &&
> +        (js_IsArrayBuffer(obj))) {
> +
> +      aContentType.SetIsVoid(true);

SetIsVoid takes a PRBool, so change 'true' to 'PR_TRUE'. Also move this down to where you change the charset.

@@ +2097,5 @@
> +      nsCOMPtr<nsIInputStream> stream;
> +      nsresult rv = NS_NewByteInputStream(getter_AddRefs(stream), data,
> +                                          abLength, NS_ASSIGNMENT_COPY);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      if (stream) {

This nullcheck isn't needed. You've already checked that the function succeeded.
Comment 8 Shawn Gong 2011-08-18 13:23:26 PDT
Created attachment 554188 [details] [diff] [review]
Patch 3.

Fixed the issues mentioned by Jonas
Comment 9 Shawn Gong 2011-08-22 16:29:25 PDT
Created attachment 554986 [details] [diff] [review]
Changed to call JS_GetArrayBufferDataLength() which is an exposed API.
Comment 10 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-09-08 18:17:33 PDT
Checked in to inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/8e143ea435d9

Thanks for the patch! Awesome job!
Comment 12 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-16 16:48:07 PDT
So, does this work off the main thread?
Comment 13 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-09-16 17:02:52 PDT
If worker-XHR uses structured clones to get the body argument from the worker thread to the main thread, as has been claimed, then it should.

Would be great if someone wrote a test for this and for sending blobs.
Comment 14 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-16 22:29:35 PDT
I tried to test this on the train but I ran into two separate worker XHR bugs ...

I might try this again next week.
Comment 15 Shawn Gong 2011-09-16 22:48:26 PDT
Hey Kyle and Jonas! I have been super busy for the first week and probably the following week as well, but will definitely take a look when possible. Have a great weekend!
Comment 16 Eric Shepherd [:sheppy] 2011-11-15 09:40:54 PST
Documented:

https://developer.mozilla.org/en/DOM/XMLHttpRequest/Using_XMLHttpRequest#Sending_typed_arrays_as_binary_data

And mentioned on Firefox 9 for developers.

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