Last Comment Bug 658683 - XMLHttpRequest copies data from ResponseBody every time GetResponse for ArrayBuffer is called
: XMLHttpRequest copies data from ResponseBody every time GetResponse for Array...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla6
Assigned To: Shawn Gong
:
: Andrew Overholt [:overholt]
Mentors:
: 639598 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-20 15:44 PDT by Shawn Gong
Modified: 2011-06-28 11:44 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Reimplemented the way how GetResponse for ArrayBuffer works. (10.27 KB, patch)
2011-05-20 15:45 PDT, Shawn Gong
jonas: review-
Details | Diff | Splinter Review
with the new TRACE (14.57 KB, patch)
2011-05-23 14:18 PDT, Shawn Gong
no flags Details | Diff | Splinter Review
check on the mResultArrayBuffer directly instead of the Rooted variable (14.56 KB, patch)
2011-05-23 14:27 PDT, Shawn Gong
no flags Details | Diff | Splinter Review
Changes according to Jonas and Peter's advices, and fixed the Rooting problem (14.20 KB, patch)
2011-05-23 15:15 PDT, Shawn Gong
jonas: review+
Details | Diff | Splinter Review
removed check on unrooting; restored blank spaces (13.76 KB, patch)
2011-05-23 17:38 PDT, Shawn Gong
jonas: review+
Details | Diff | Splinter Review
here you go :) (13.79 KB, patch)
2011-05-23 18:10 PDT, Shawn Gong
peterv: review-
Details | Diff | Splinter Review
latest attempt (13.44 KB, patch)
2011-05-24 04:22 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
jst: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Shawn Gong 2011-05-20 15:44:16 PDT
XMLHttpRequest copies data from ResponseBody every time GetResponse for ArrayBuffer is called, and return the newly copied result, which is inefficient. Should change the behavior thus we hold the result after first read, and return the result directly afterwards.
Comment 1 Shawn Gong 2011-05-20 15:45:52 PDT
Created attachment 534137 [details] [diff] [review]
Reimplemented the way how GetResponse for ArrayBuffer works.
Comment 2 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-21 00:27:01 PDT
Comment on attachment 534137 [details] [diff] [review]
Reimplemented the way how GetResponse for ArrayBuffer works.

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

Also add a test checking that the same buffer is returned for every call to GetResponse.

::: content/base/src/nsXMLHttpRequest.cpp
@@ +985,5 @@
>  
>    case XML_HTTP_RESPONSE_TYPE_ARRAYBUFFER:
> +    if (!mResultArrayBufferRooted) {
> +      rv = GetResponseArrayBuffer(aCx);
> +    }

Only call into GetResponseArrayBuffer if we're in the DONE state. Otherwise just return JSVAL_NULL.

That way you can also remove the state checks in the beginning of GetResponseArrayBuffer.

@@ +1968,5 @@
>      RequestCompleted();
>    } else {
>      ChangeState(XML_HTTP_REQUEST_STOPPED, PR_FALSE);
>    }
> + 

Why this change?

::: content/base/src/nsXMLHttpRequest.h
@@ +212,4 @@
>                                             nsXHREventTarget)
>  
>    PRBool AllowUploadProgress();
> +  

Don't add whitespace like this.

@@ +226,5 @@
>                  const char* fromRawSegment,
>                  PRUint32 toOffset,
>                  PRUint32 count,
>                  PRUint32 *writeCount);
> +  nsresult GetResponseArrayBuffer(JSContext* aCx);

Given that this function no longer returns a arraybuffer, probably better to rename it CreateResponseArrayBuffer or some such.
Comment 3 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-21 01:27:00 PDT
I pushed this and bug 632255 to tryserver and got one failure and one seemingly random crash:

http://tbpl.mozilla.org/?tree=Try&rev=e2514be69c2d
Comment 4 Peter Van der Beken [:peterv] 2011-05-23 03:53:12 PDT
Why do we have both mResultArrayBuffer and mResultArrayBufferRooted, seems like a non-null mResultArrayBuffer always means mResultArrayBufferRooted is true, so just checking for non-null mResultArrayBuffer instead should work. Also, you really should check for the result of NS_HOLD_JS_OBJECTS and null out mResultArrayBuffer if it fails, otherwise we'll end up with pointer to a potentially GC'ed object.
Comment 5 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-23 04:06:21 PDT
There seems to be bigger problems with this patch than that though. See comment 3.

I'm guessing what is happening is that we're stepping on nsWrapperCache's toes by telling the cyclecollector to stop calling the trace hook on this object.

Peter: Does the wrapper cache always cause the XHR to be traced? If so, should we simply rely on that and just implement our own tracehook which traces the arraybuffer (if it's not null), and then calls the nsWrapperCache trace hook to give it a chance to trace the XHR wrapper.
Comment 6 Peter Van der Beken [:peterv] 2011-05-23 05:55:16 PDT
Ah, hmm, we don't have support for trace hooks in inheriting CC participants. Either we add support for that (just forward to the base CC pariticpant like the other _INHERITED macros), or you trace the nsWrapperCache yourself. I'd prefer the first solution I think.
Comment 7 Shawn Gong 2011-05-23 14:18:45 PDT
Created attachment 534565 [details] [diff] [review]
with the new TRACE
Comment 8 Shawn Gong 2011-05-23 14:27:27 PDT
Created attachment 534570 [details] [diff] [review]
check on the mResultArrayBuffer directly instead of the Rooted variable
Comment 9 Shawn Gong 2011-05-23 15:15:57 PDT
Created attachment 534587 [details] [diff] [review]
Changes according to Jonas and Peter's advices, and fixed the Rooting problem
Comment 10 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-23 17:07:29 PDT
Comment on attachment 534587 [details] [diff] [review]
Changes according to Jonas and Peter's advices, and fixed the Rooting problem

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

r=me with that fixed.

::: content/base/src/nsXMLHttpRequest.cpp
@@ +436,5 @@
>  nsXMLHttpRequest::~nsXMLHttpRequest()
>  {
> +  if (mResultArrayBuffer) {
> +    UnrootResultArrayBuffer();
> +  }

You need to always unroot. Same in the unlink code.

@@ +879,2 @@
>      return NS_OK;
>    }

This whole if-statement should not be needed. You're only getting in here if we're in the DONE state. Feel free to add an assert to that effect if you want though.

@@ -1931,5 @@
>      RequestCompleted();
>    } else {
>      ChangeState(XML_HTTP_REQUEST_STOPPED, PR_FALSE);
>    }
> -

This change seems unrelated.

::: content/base/src/nsXMLHttpRequest.h
@@ +213,2 @@
>    PRBool AllowUploadProgress();
> +  

Don't add this whitespace
Comment 11 Shawn Gong 2011-05-23 17:38:37 PDT
Created attachment 534636 [details] [diff] [review]
removed check on unrooting; restored blank spaces
Comment 12 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-23 18:02:56 PDT
Comment on attachment 534636 [details] [diff] [review]
removed check on unrooting; restored blank spaces

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

r=me with that fixed.

::: content/base/src/nsXMLHttpRequest.cpp
@@ +973,5 @@
>  
>    case XML_HTTP_RESPONSE_TYPE_ARRAYBUFFER:
>      if (mState & XML_HTTP_REQUEST_DONE) {
> +      if (!mResultArrayBuffer) {  
> +         rv = CreateResponseArrayBuffer(aCx);

Since you're using mResultArrayBuffer below, you should add an NS_ENSURE_SUCCESS(rv, rv); here to avoid using it unless it was successfully created.

::: content/base/src/nsXMLHttpRequest.h
@@ +212,2 @@
>                                             nsXHREventTarget)
> +  

Hmm.. the added spaces are still here.
Comment 13 Shawn Gong 2011-05-23 18:10:18 PDT
Created attachment 534646 [details] [diff] [review]
here you go :)
Comment 14 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-24 01:02:44 PDT
This is still not working, we end up with random crashes. I suspect it's related to the hold mechanism combined with the nsWrapperCache.

Peter, we really need your help to figure this out.
Comment 15 Peter Van der Beken [:peterv] 2011-05-24 02:23:15 PDT
Comment on attachment 534646 [details] [diff] [review]
here you go :)

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

::: content/base/src/nsXMLHttpRequest.cpp
@@ +434,5 @@
>  }
>  
>  nsXMLHttpRequest::~nsXMLHttpRequest()
>  {
> +  UnrootResultArrayBuffer();

You should only call DROP if you've called HOLD before, so the calls to UnrootResultArrayBuffer should be guarded with |if (mResultArrayBuffer)|, or you you can put the if inside UnrootResultArrayBuffer. The rule is you only call DROP once per call to HOLD, and you always call DROP between calls to HOLD.

@@ +455,5 @@
>  
> +void 	
> +nsXMLHttpRequest::RootResultArrayBuffer()
> +{
> +  NS_HOLD_JS_OBJECTS(this, nsXMLHttpRequest);

You still aren't checking the return value of HOLD/DROP.

@@ +870,3 @@
>      return NS_ERROR_FAILURE;
>  
> +  mResultArrayBuffer = nsnull;

This nulling out seems unnecessary? In fact, I'd argue you need to assert that mResultArrayBuffer is null here.

@@ +1095,5 @@
>    mResponseBody.Truncate();
>    mResponseBodyUnicode.SetIsVoid(PR_TRUE);
>    mResponseBlob = nsnull;
>    mState |= XML_HTTP_REQUEST_ABORTED;
> +  mResultArrayBuffer = nsnull;

Shouldn't this call UnrootResultArrayBuffer?
Comment 16 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-24 02:30:41 PDT
> @@ +434,5 @@
> >  }
> >  
> >  nsXMLHttpRequest::~nsXMLHttpRequest()
> >  {
> > +  UnrootResultArrayBuffer();
> 
> You should only call DROP if you've called HOLD before, so the calls to
> UnrootResultArrayBuffer should be guarded with |if (mResultArrayBuffer)|, or
> you you can put the if inside UnrootResultArrayBuffer. The rule is you only
> call DROP once per call to HOLD, and you always call DROP between calls to
> HOLD.

Is it safe to call DROP when we no longer need the ArrayBuffer, even if this is before the XHR is going away? Or will that conflict with nsWrapperCache potentially needing tracing to root the wrapper?

Note that we'll sometimes drop the ArrayBuffer well before the XHR goes away if you issue a second request with the same XHR.

Would be great if you can grab me on IRC.
Comment 17 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-24 04:22:43 PDT
Created attachment 534721 [details] [diff] [review]
latest attempt
Comment 18 Mounir Lamouri (:mounir) 2011-05-24 06:50:46 PDT
Pushed by Jonas then backed out because of orange on Windows Mochitest-1.
See:
http://tbpl.mozilla.org/?tree=Firefox&rev=0acfb7c8331b
Comment 19 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-25 00:48:04 PDT
So apparently *this* patch works fine. However doing the same thing in the FileReader does not.

Anyhow, I landed this part. Lets look at the FileReader over in bug 632255

http://hg.mozilla.org/mozilla-central/rev/6a66bc2687ca
Comment 20 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-31 17:46:54 PDT
Comment on attachment 534721 [details] [diff] [review]
latest attempt

I'd really like to get this into aurora.

This fixes a bug in a feature which was added in Firefox 6. This feature is XMLHttpRequest.response. Currently we copy a bunch of memory every time the property is accessed. This both leads to bad performance as well as unexpected results such as xhr.response == xhr.response evaluating to false.

We initially had some issues landing this patch, however it turned out that the problems weren't in this patch at all, but rather in one that was landed at the same time. This patch has been landed on trunk for over a week so far with no problems at all.

To be clear, I know we're past aurora branching, and I can live with not taking this patch because of that. But it would be a nice improvement of a new feature and so far data is indicating that the patch is safe.
Comment 21 Masatoshi Kimura [:emk] 2011-06-01 02:08:23 PDT
*** Bug 639598 has been marked as a duplicate of this bug. ***
Comment 22 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-06-28 11:44:07 PDT
This was checked in to aurora a long time ago.

https://hg.mozilla.org/releases/mozilla-aurora/rev/e47d70a92208

It was also checked in to trunk as noted in comment 19

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