The default bug view has changed. See this FAQ.

Sending arraybuffers using XMLHttpRequest.send

RESOLVED FIXED in mozilla9

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Shawn Gong, Assigned: Shawn Gong)

Tracking

({dev-doc-complete})

unspecified
mozilla9
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Implement the ability for XMLHttpRequest.send to take in arraybuffer data.
(Assignee)

Updated

6 years ago
Assignee: nobody → shawn
(Assignee)

Comment 1

6 years ago
Created attachment 552589 [details] [diff] [review]
Patch 1. Implemented the feature
Attachment #552589 - Flags: review?(jonas)
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?
Roping in bent to think about how to do this in workers.
(Assignee)

Comment 4

6 years ago
(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 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.
Attachment #552589 - Flags: review?(jonas) → review-
(Assignee)

Comment 6

6 years ago
Created attachment 553980 [details] [diff] [review]
Patch 2.
Attachment #552589 - Attachment is obsolete: true
Attachment #553980 - Flags: review?(jonas)
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.
Attachment #553980 - Flags: review?(jonas) → review+
(Assignee)

Comment 8

6 years ago
Created attachment 554188 [details] [diff] [review]
Patch 3.

Fixed the issues mentioned by Jonas
(Assignee)

Comment 9

6 years ago
Created attachment 554986 [details] [diff] [review]
Changed to call JS_GetArrayBufferDataLength() which is an exposed API.
Attachment #554188 - Attachment is obsolete: true
Checked in to inbound:

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

Thanks for the patch! Awesome job!
Keywords: dev-doc-needed
http://hg.mozilla.org/mozilla-central/rev/8e143ea435d9
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
So, does this work off the main thread?
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.
I tried to test this on the train but I ran into two separate worker XHR bugs ...

I might try this again next week.
(Assignee)

Comment 15

6 years ago
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!
Documented:

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

And mentioned on Firefox 9 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.