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

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

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.

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.

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

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

> @@ +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.

Pushed by Jonas then backed out because of orange on Windows Mochitest-1. See: http://tbpl.mozilla.org/?tree=Firefox&rev=0acfb7c8331b

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

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