Closed Bug 678432 Opened 13 years ago Closed 13 years ago

Sending arraybuffers using XMLHttpRequest.send

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla9

People

(Reporter: hippopotamus.gong, Assigned: hippopotamus.gong)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 2 obsolete files)

Patch 2.
5.93 KB, patch
sicking
: review+
Details | Diff | Splinter Review
Changed to call JS_GetArrayBufferDataLength() which is an exposed API.
5.38 KB, patch
Details | Diff | Splinter Review 
Implement the ability for XMLHttpRequest.send to take in arraybuffer data.
Assignee: nobody → shawn
Attached patch Patch 1. Implemented the feature (obsolete) — — Splinter Review 

        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.

    
    

    
  
(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-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch 2.
        — 
          — Splinter Review
      

    


  

        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+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch 3. (obsolete)
        — 
          — Splinter Review
      

    


  
Fixed the issues mentioned by Jonas

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/8e143ea435d9
Status: NEW → RESOLVED
Closed: 13 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.

    
    

    
  
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!
Component: DOM → DOM: Core & HTML

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: