Closed
Bug 703380
Opened 13 years ago
Closed 13 years ago
XMLHttpRequest can fire an abort event after a load event, but should not
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: WeirdAl, Assigned: emk)
References
()
Details
Attachments
(4 files, 3 obsolete files)
(deleted),
patch
|
smaug
:
review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
emk
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
emk
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
emk
:
review+
|
Details | Diff | Splinter Review |
In my testing for bug 525816 comment 32, I discovered that a load event doesn't prevent an abort event firing later. Per both the XMLHttpRequest Level 2 specification and the ProgressEvent specification, this is wrong: once the load event fires, the abort event cannot fire, and vice versa. I believe this will break at least one content Mochitest.
Reporter | ||
Updated•13 years ago
|
Summary: XMLHttpRequest can fire an abort event after a load event → XMLHttpRequest can fire an abort event after a load event, but should not
Assignee | ||
Comment 1•13 years ago
|
||
The fix should be trivial. https://hg.mozilla.org/mozilla-central/file/b62e6ee5ba9b/content/base/src/nsXMLHttpRequest.cpp#l1163 The abort event was added by bug 435425. Smaug, why it is added outside the above if-statement?
Reporter | ||
Comment 2•13 years ago
|
||
Just in case someone has a bit of free time and wants to tackle this one before I can fix it... I'm attaching the Mochitest I used to find this bug, for review.
Attachment #575402 -
Flags: review?(bugs)
Comment 3•13 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #1) > Smaug, why it is added outside the > above if-statement? Good question. Either I made a mistake or the spec didn't define the behavior properly 2 years ago.
Assignee | ||
Comment 4•13 years ago
|
||
Assignee | ||
Comment 5•13 years ago
|
||
Comment on attachment 575422 [details] [diff] [review] trivial fix attachment 575402 [details] [diff] [review] does not longer fail with this patch.
Comment 6•13 years ago
|
||
Comment on attachment 575402 [details] [diff] [review] Mochitest as patch >diff -r 4b24c5ab84cb content/base/test/file_XHR_events.sjs >--- /dev/null Thu Jan 01 00:00:00 1970 +0000 >+++ b/content/base/test/file_XHR_events.sjs Thu Nov 17 23:04:37 2011 -0800 >@@ -0,0 +1,24 @@ >+var timer = null; >+ >+function handleRequest(request, response) >+{ >+ response.processAsync(); >+ var shouldError = request.queryString.indexOf("respondWithError=true") != -1; >+ >+ timer = Components.classes["@mozilla.org/timer;1"] >+ .createInstance(Components.interfaces.nsITimer); >+ timer.initWithCallback(function() >+ { >+ if (shouldError) { >+ response.setStatusLine(null, 302, "Moved"); >+ // this should trigger a cross-site redirect error >+ response.setHeader("Location", "http://mozilla.org", false); I know it shouldn't cause any requests to mozilla.org, but could you still use one of the test domains. nction() { >+ var req = new XMLHttpRequest(); >+ this.request = req; >+ req.open("GET", this.baseURL + "?" + this.getMessage()); >+ req.onerror = this; >+ req.onload = this; >+ req.onabort = this; >+ >+ var _this = this; >+ var abortTime = this.testCompleteTime / 4; >+ if (!this.scheduledAbortBefore) { >+ abortTime *= 3; >+ } I don't understand / 4 and *3 >+ window.setTimeout(function() { >+ _this.evaluateTest() >+ }, this.testCompleteTime); >+ window.setTimeout(function() { >+ req.abort() >+ }, abortTime); This looks error prone. Couldn't you just rely on that you get right events in right order.
Attachment #575402 -
Flags: review?(bugs) → review-
Comment 7•13 years ago
|
||
Comment on attachment 575422 [details] [diff] [review] trivial fix This needs Alex' tests before landing. Well, any tests :)
Attachment #575422 -
Flags: review?(bugs) → review+
Reporter | ||
Comment 8•13 years ago
|
||
I strongly recommend a try-server run first: http://mxr.mozilla.org/mozilla-central/source/content/base/test.test_bug482935.html?force=1#31 (In reply to Olli Pettay [:smaug] from comment #6) > I know it shouldn't cause any requests to mozilla.org, but could you still > use one of the test domains. Which are? :) > >+ var abortTime = this.testCompleteTime / 4; > >+ if (!this.scheduledAbortBefore) { > >+ abortTime *= 3; > >+ } > I don't understand / 4 and *3 1/4 of the full test time and 3/4 of the full test time. The load or error event takes place at 1/2 of the full test, so this is simply placing the abort() call before or after the load (or error) is scheduled to happen - and far enough away that other events (window.setTimeout to change XHR.timeout) can happen in between the big spaces. I don't know exactly how to make that clearer, other than a big comment. > >+ window.setTimeout(function() { > >+ _this.evaluateTest() > >+ }, this.testCompleteTime); > >+ window.setTimeout(function() { > >+ req.abort() > >+ }, abortTime); > This looks error prone. > Couldn't you just rely on that you get right events in right order. That's not what this code is doing. This code is scheduling the abort() call, relative to other actions (the load/error firing). I agree I can rewrite the test itself to count the number of events it gets among the exclusives (should be 1), and the name of the event it last received. Again, I'm trying to write the test so that I can change the XHR's timeout at a pre-determined point in time. That's pretty important per my reading of the XHR2 spec: while the XHR is still in progress, the user can change the timeout length, at will.
Comment 9•13 years ago
|
||
> The load or error > event takes place at 1/2 of the full test, How do you know that? The server runs in a different process, so if some strange garbage/cycle collector + sync I/O happens, the timers might run at quite different times than what you expect. I just want to be very sure we don't create yet more randomly-orange tests. And see http://mxr.mozilla.org/mozilla-central/source/build/pgo/server-locations.txt
Assignee | ||
Comment 10•13 years ago
|
||
Attachment #575422 -
Attachment is obsolete: true
Attachment #575665 -
Flags: review+
Assignee | ||
Comment 11•13 years ago
|
||
test_bug482935.html expected that abort() will fire an abort event even in the DONE state. I added test_xhr_abort_after_load.html which fails without patch and passes with patch.
Assignee | ||
Updated•13 years ago
|
Attachment #575666 -
Flags: review?(bugs)
Assignee | ||
Comment 12•13 years ago
|
||
This patch will also fix bug 676172.
Attachment #575668 -
Flags: review?(bent.mozilla)
Assignee | ||
Updated•13 years ago
|
Attachment #575665 -
Attachment description: patch for check in → patch for check in; r=smaug
Assignee | ||
Comment 13•13 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=b46057874895
Comment on attachment 575668 [details] [diff] [review] Worker part Yay, thanks for looking into this! >+ else if (mType.EqualsASCII(sEventStrings[STRING_abort])) { Nit: How about we clean this up slightly, to this: else if (mType.EqualsASCII(sEventStrings[STRING_abort])) { if ((mUploadEvent && !mProxy->mSeenUploadLoadStart) || (!mUploadEvent && !mProxy->mSeenLoadStart)) { // We've already dispatched premature abort events. return true; } } else if (mType.EqualsASCII(sEventStrings[STRING_readystatechange])) { if (mReadyState == 4 && !mUploadEvent && !mProxy->mSeenLoadStart) { // We've already dispatched premature abort events. return true; } } } That way we have simple blocks for each event type.
Attachment #575668 -
Flags: review?(bent.mozilla) → review+
Assignee | ||
Comment 15•13 years ago
|
||
Attachment #575668 -
Attachment is obsolete: true
Attachment #575731 -
Flags: review+
Comment 16•13 years ago
|
||
Comment on attachment 575666 [details] [diff] [review] Test r+ but could you add still a test for xhr.send(); xhr.abort();
Attachment #575666 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 17•13 years ago
|
||
Added a send(); abort(); test case.
Attachment #575666 -
Attachment is obsolete: true
Attachment #575842 -
Flags: review+
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Comment 18•13 years ago
|
||
Pushed to inbound patches that had been reviewed, including: attachment 575842 [details] [diff] [review] attachment 575665 [details] [diff] [review] attachment 575731 [details] [diff] [review] Changesets: https://hg.mozilla.org/integration/mozilla-inbound/rev/d3b066c050d3 https://hg.mozilla.org/integration/mozilla-inbound/rev/6b2d036a961e https://hg.mozilla.org/integration/mozilla-inbound/rev/c531673c8d2f
Keywords: checkin-needed
Target Milestone: --- → mozilla11
Comment 19•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d3b066c050d3 https://hg.mozilla.org/mozilla-central/rev/6b2d036a961e https://hg.mozilla.org/mozilla-central/rev/c531673c8d2f Leaving open for remaining patch (given wording of comment 18). Please resolve if in fact done here. Thanks :-)
Assignee | ||
Comment 20•13 years ago
|
||
I think Alex' tests are no longer required (I made a test to remove the requirement). He will add an equivalent test for bug 525816 anyway.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Component: DOM: Mozilla Extensions → DOM
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•