Last Comment Bug 680816 - Reusing an XHR2 object with responseType of "arraybuffer" does not work
: Reusing an XHR2 object with responseType of "arraybuffer" does not work
Status: RESOLVED FIXED
: testcase
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal with 1 vote (vote)
: mozilla7
Assigned To: Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-21 17:20 PDT by Bryan Rockwood
Modified: 2013-12-27 14:31 PST (History)
7 users (show)
khuey: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
fixed
-
fixed
fixed


Attachments
An example of the problem. (571 bytes, text/html)
2011-08-21 17:21 PDT, Bryan Rockwood
no flags Details
A binary file containing a five element float array where all values are 99998 (20 bytes, application/octet-stream)
2011-08-21 17:22 PDT, Bryan Rockwood
no flags Details
A binary file containing a five element float array where all values are 1 (20 bytes, application/octet-stream)
2011-08-21 17:22 PDT, Bryan Rockwood
no flags Details
Patch (1014 bytes, patch)
2011-08-23 15:29 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
jonas: review+
Details | Diff | Splinter Review
Patch as landed (1.12 KB, patch)
2011-08-25 07:05 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
khuey: review+
blizzard: approval‑mozilla‑aurora+
blizzard: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Bryan Rockwood 2011-08-21 17:20:00 PDT
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0) Gecko/20100101 Firefox/6.0
Build ID: 20110811165603

Steps to reproduce:

Attempting to reuse a XMLHttpRequest object where the responseType is set to "arraybuffer".


Actual results:

The first request runs without issue.  Subsequent requests do not update the response property so the first requested array buffer is always returned.


Expected results:

Each new request should update the response property with the data retrieved from the request.  Verified this works in Chrome (v13.0.782.112 m) and in Safari (5.1).
Comment 1 Bryan Rockwood 2011-08-21 17:21:11 PDT
Created attachment 554773 [details]
An example of the problem.
Comment 2 Bryan Rockwood 2011-08-21 17:22:24 PDT
Created attachment 554774 [details]
A binary file containing a five element float array where all values are 99998
Comment 3 Bryan Rockwood 2011-08-21 17:22:53 PDT
Created attachment 554775 [details]
A binary file containing a five element float array where all values are 1
Comment 4 Bryan Rockwood 2011-08-21 17:28:51 PDT
Recreating the XMLHttpRequest object before the next request allows the script to retrieve all the data properly so there is a workaround.
Comment 5 Boris Zbarsky [:bz] 2011-08-22 19:57:42 PDT
Jonas, can you take a look into this, please?  This sounds pretty bad....  Possibly bad enough that we need to fix this on 7.
Comment 6 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-23 15:07:01 PDT
We're nulling out mResultArrayBuffer in nsXMLHttpRequest::Abort, which is the obvious bug ...

Digging in here.
Comment 7 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-23 15:29:02 PDT
Created attachment 555226 [details] [diff] [review]
Patch

We need to clear mResultArrayBuffer even if we're not aborting an existing XHR send call.

/me makes a note to investigate FileReader for a similar bug.
Comment 8 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-08-23 16:59:05 PDT
Comment on attachment 555226 [details] [diff] [review]
Patch

we reset the other similar members here:

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#1728

Might be worth being consistent even though your patch releases the buffer slightly sooner.

(for what it's worth, state management in XHR is currently overly complicated. I'm slowly working on a set of patches to improve the situation).

r=me either way.
Comment 9 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-24 07:15:33 PDT
For those playing along at home, FileReader does not have the same bug, because FileReader calls abort unconditionally.

I'll make the change suggested.
Comment 10 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-25 06:56:24 PDT
http://hg.mozilla.org/mozilla-central/rev/bcadab745173
http://hg.mozilla.org/mozilla-central/rev/3a474d3aaa9a
Comment 11 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-25 07:05:54 PDT
Created attachment 555721 [details] [diff] [review]
Patch as landed
Comment 12 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-25 07:06:47 PDT
Comment on attachment 555721 [details] [diff] [review]
Patch as landed

I'm kind of undecided on what branches we should fix this on, but it's a pretty trivial patch, and bz seems to think it's important.
Comment 13 Christopher Blizzard (:blizzard) 2011-08-25 14:23:46 PDT
Comment on attachment 555721 [details] [diff] [review]
Patch as landed

Approved for Beta (Update 7) and Aurora (Update 8).  Please land as soon as possible.
Comment 14 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-25 17:05:48 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/a233176c3910
http://hg.mozilla.org/releases/mozilla-aurora/rev/82e79db5a5fc
Comment 15 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-25 19:08:31 PDT
http://hg.mozilla.org/releases/mozilla-beta/rev/ede728bc03d2
http://hg.mozilla.org/releases/mozilla-beta/rev/bf3712567861
Comment 16 Ioana (away) 2011-08-26 04:06:42 PDT
Verified Fixed on:
->Windows 7 32-bit
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 6.1; rv:8.0a2) Gecko/20110825 Firefox/8.0a2
Mozilla/5.0 (Windows NT 6.1; rv:9.0a1) Gecko/20110825 Firefox/9.0a1
->Windows 7 64-bit         
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a2) Gecko/20110825 Firefox/8.0a2
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:9.0a1) Gecko/20110825 Firefox/9.0a1

I used the example from comment #1 to verify this fix:
 1. Open the link provided in comment #1.
 2. Tap no the "do request" button.
Two alerts with the same number are displayed.
Comment 17 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-26 04:08:43 PDT
(In reply to Ioana Budnar from comment #16)
> Two alerts with the same number are displayed.

That's the *bug*.  The numbers are supposed to be different.
Comment 18 Ioana (away) 2011-08-26 04:19:42 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #17)
> (In reply to Ioana Budnar from comment #16)
> > Two alerts with the same number are displayed.
> 
> That's the *bug*.  The numbers are supposed to be different.

I ran the steps from my previous comment on Chrome and Safari 5.1, on which the bug description says the page works fine. The same two alerts with the same number were given on these browsers.

In this case, could you please give me another test case and/or some STRs?

Thank you
Comment 19 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-28 06:20:59 PDT
I didn't test Safari, but this works in Chrome.  You have to download all three test files though.  For convenience, I've put these files up at http://people.mozilla.org/~khuey/bugs/xhrarraybufferreuse.html.  If you click on that, you should see a dialog with '99998' and another dialog with '1'.  The bug is seeing two dialogs with '99998'.  If you see two dialogs with '3290512384' that means that it didn't find the files.
Comment 20 Ioana (away) 2011-08-28 23:54:02 PDT
I have verified this issue on the following builds:
->Windows 7 32-bit
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 6.1; rv:8.0a2) Gecko/20110825 Firefox/8.0a2
Mozilla/5.0 (Windows NT 6.1; rv:9.0a1) Gecko/20110825 Firefox/9.0a1
->Windows 7 64-bit         
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a2) Gecko/20110825 Firefox/8.0a2
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:9.0a1) Gecko/20110825 Firefox/9.0a1

The bug is still reproducing: two alerts with '99998' are displayed on all the builds. Should I reopen the bug in this case?
Comment 21 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-29 05:09:04 PDT
Testing builds made after the fix was checked in would help.
Comment 22 Ioana (away) 2011-09-02 01:21:12 PDT
Verified fixed on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0) Gecko/20100101 Firefox/7.0
Build ID: 20110830100616

STR: 
1. Open http://people.mozilla.org/~khuey/bugs/xhrarraybufferreuse.html.
2. Click the "do request" button.
Two alerts are displayed, the first one with "99998" and the second one with "1".

Will check on Fx8 and 9 when new builds appear.
Comment 23 Ioana (away) 2011-09-12 00:51:30 PDT
Verified fixed on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a2) Gecko/20110911 Firefox/8.0a2 
Build ID: 20110911042006
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:9.0a1) Gecko/20110911 Firefox/9.0a1
Build ID: 20110911030845

STR: 
1. Open http://people.mozilla.org/~khuey/bugs/xhrarraybufferreuse.html.
2. Click the "do request" button.
Two alerts are displayed, the first one with "99998" and the second one with "1".
Comment 24 Alex Vincent [:WeirdAl] 2011-12-17 21:12:59 PST
I think we may want to back this out - the XHR2 specification says this it should not work.

From http://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html#the-open-method, step 11:

"Let async be the value of the async argument or true if it was omitted.

If async is false, there is an associated XMLHttpRequest document and either the timeout attribute value is not zero, the withCredentials attribute value is true, or the responseType attribute value is not the empty string, throw an "InvalidAccessError" exception and terminate these steps."
Comment 25 Masatoshi Kimura [:emk] 2011-12-17 21:31:50 PST
Why backout? It's perfectly valid reusing XHR object with async=true.
You should rather rewrite the test so that it makes sure that .responseType is not settable for sync XHR.
Comment 26 Masatoshi Kimura [:emk] 2011-12-17 21:36:51 PST
See this interdiff about my attemot in bug 701787 (and separated out to the other bug later).
https://bugzilla.mozilla.org/attachment.cgi?oldid=578886&action=interdiff&newid=578887&headers=1
Comment 27 Boris Zbarsky [:bz] 2011-12-18 11:44:23 PST
> I think we may want to back this out

That should be a separate bug then.  Having the code in the tree but this bug open is just confusing.
Comment 28 Alex Vincent [:WeirdAl] 2011-12-20 14:59:45 PST
Yeah, you're right.  Sorry for the bugspam, everyone.

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