Reusing an XHR2 object with responseType of "arraybuffer" does not work

RESOLVED FIXED in Firefox 7

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: Bryan Rockwood, Assigned: khuey)

Tracking

({testcase})

Trunk
mozilla7
x86_64
Windows 7
testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox7- fixed, firefox8- fixed, firefox9 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
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).
(Reporter)

Comment 1

6 years ago
Created attachment 554773 [details]
An example of the problem.
(Reporter)

Comment 2

6 years ago
Created attachment 554774 [details]
A binary file containing a five element float array where all values are 99998
(Reporter)

Comment 3

6 years ago
Created attachment 554775 [details]
A binary file containing a five element float array where all values are 1
(Reporter)

Comment 4

6 years ago
Recreating the XMLHttpRequest object before the next request allows the script to retrieve all the data properly so there is a workaround.
Attachment #554773 - Attachment mime type: text/plain → text/html
Component: General → General
Keywords: testcase
Product: Firefox → Core
QA Contact: general → general
Jonas, can you take a look into this, please?  This sounds pretty bad....  Possibly bad enough that we need to fix this on 7.
Status: UNCONFIRMED → NEW
tracking-firefox7: --- → ?
tracking-firefox8: --- → ?
tracking-firefox9: --- → ?
Component: General → DOM
Ever confirmed: true
QA Contact: general → general
Version: 6 Branch → Trunk
We're nulling out mResultArrayBuffer in nsXMLHttpRequest::Abort, which is the obvious bug ...

Digging in here.
Assignee: nobody → khuey
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.
Attachment #555226 - Flags: review?(jonas)
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.
Attachment #555226 - Flags: review?(jonas) → review+
For those playing along at home, FileReader does not have the same bug, because FileReader calls abort unconditionally.

I'll make the change suggested.
http://hg.mozilla.org/mozilla-central/rev/bcadab745173
http://hg.mozilla.org/mozilla-central/rev/3a474d3aaa9a
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status-firefox9: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
status-firefox7: --- → affected
status-firefox8: --- → affected
tracking-firefox9: ? → ---
Created attachment 555721 [details] [diff] [review]
Patch as landed
Attachment #554773 - Attachment is obsolete: true
Attachment #554774 - Attachment is obsolete: true
Attachment #554775 - Attachment is obsolete: true
Attachment #555226 - Attachment is obsolete: true
Attachment #555721 - Flags: review+
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.
Attachment #555721 - Flags: approval-mozilla-beta?
Attachment #555721 - Flags: approval-mozilla-aurora?
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.
Attachment #555721 - Flags: approval-mozilla-beta?
Attachment #555721 - Flags: approval-mozilla-beta+
Attachment #555721 - Flags: approval-mozilla-aurora?
Attachment #555721 - Flags: approval-mozilla-aurora+
http://hg.mozilla.org/releases/mozilla-aurora/rev/a233176c3910
http://hg.mozilla.org/releases/mozilla-aurora/rev/82e79db5a5fc
status-firefox8: affected → fixed
Target Milestone: mozilla9 → mozilla7
http://hg.mozilla.org/releases/mozilla-beta/rev/ede728bc03d2
http://hg.mozilla.org/releases/mozilla-beta/rev/bf3712567861
status-firefox7: affected → fixed

Comment 16

6 years ago
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.

Updated

6 years ago
Status: RESOLVED → VERIFIED
(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.
Status: VERIFIED → RESOLVED
Last Resolved: 6 years ago6 years ago

Comment 18

6 years ago
(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
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

6 years ago
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?
Testing builds made after the fix was checked in would help.

Updated

6 years ago
tracking-firefox7: ? → -
tracking-firefox8: ? → -

Comment 22

6 years ago
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

6 years ago
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".
Status: RESOLVED → VERIFIED
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."
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
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.
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
> 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.
Yeah, you're right.  Sorry for the bugspam, everyone.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.