Last Comment Bug 703380 - XMLHttpRequest can fire an abort event after a load event, but should not
: XMLHttpRequest can fire an abort event after a load event, but should not
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Masatoshi Kimura [:emk]
:
Mentors:
http://dev.w3.org/2006/webapi/progres...
Depends on:
Blocks: xhr-timeout 676172
  Show dependency treegraph
 
Reported: 2011-11-17 13:52 PST by Alex Vincent [:WeirdAl]
Modified: 2013-04-04 13:53 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Mochitest as patch (7.69 KB, patch)
2011-11-17 23:47 PST, Alex Vincent [:WeirdAl]
bugs: review-
Details | Diff | Review
trivial fix (1.82 KB, patch)
2011-11-18 04:44 PST, Masatoshi Kimura [:emk]
bugs: review+
Details | Diff | Review
patch for check in; r=smaug (1.91 KB, patch)
2011-11-19 07:12 PST, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Review
Test (4.92 KB, patch)
2011-11-19 07:18 PST, Masatoshi Kimura [:emk]
bugs: review+
Details | Diff | Review
Worker part (4.48 KB, patch)
2011-11-19 07:20 PST, Masatoshi Kimura [:emk]
bent.mozilla: review+
Details | Diff | Review
Worker part, for check in; r=bent (4.33 KB, patch)
2011-11-20 00:04 PST, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Review
Test fix & added a new test; r=smaug (5.30 KB, patch)
2011-11-21 06:09 PST, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Review

Description Alex Vincent [:WeirdAl] 2011-11-17 13:52:57 PST
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.
Comment 1 Masatoshi Kimura [:emk] 2011-11-17 17:35:01 PST
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?
Comment 2 Alex Vincent [:WeirdAl] 2011-11-17 23:47:39 PST
Created attachment 575402 [details] [diff] [review]
Mochitest as patch

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.
Comment 3 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-11-18 04:05:14 PST
(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.
Comment 4 Masatoshi Kimura [:emk] 2011-11-18 04:44:48 PST
Created attachment 575422 [details] [diff] [review]
trivial fix
Comment 5 Masatoshi Kimura [:emk] 2011-11-18 04:46:03 PST
Comment on attachment 575422 [details] [diff] [review]
trivial fix

attachment 575402 [details] [diff] [review] does not longer fail with this patch.
Comment 6 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-11-18 04:47:42 PST
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.
Comment 7 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-11-18 05:32:02 PST
Comment on attachment 575422 [details] [diff] [review]
trivial fix

This needs Alex' tests before landing.
Well, any tests :)
Comment 8 Alex Vincent [:WeirdAl] 2011-11-18 07:17:13 PST
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 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-11-18 07:30:29 PST
 > 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
Comment 10 Masatoshi Kimura [:emk] 2011-11-19 07:12:25 PST
Created attachment 575665 [details] [diff] [review]
patch for check in; r=smaug
Comment 11 Masatoshi Kimura [:emk] 2011-11-19 07:18:20 PST
Created attachment 575666 [details] [diff] [review]
Test

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.
Comment 12 Masatoshi Kimura [:emk] 2011-11-19 07:20:33 PST
Created attachment 575668 [details] [diff] [review]
Worker part

This patch will also fix bug 676172.
Comment 13 Masatoshi Kimura [:emk] 2011-11-19 07:22:50 PST
https://tbpl.mozilla.org/?tree=Try&rev=b46057874895
Comment 14 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-19 10:02:22 PST
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.
Comment 15 Masatoshi Kimura [:emk] 2011-11-20 00:04:50 PST
Created attachment 575731 [details] [diff] [review]
Worker part, for check in; r=bent
Comment 16 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-11-20 13:28:22 PST
Comment on attachment 575666 [details] [diff] [review]
Test

r+ but could you add still a test for
xhr.send(); xhr.abort();
Comment 17 Masatoshi Kimura [:emk] 2011-11-21 06:09:58 PST
Created attachment 575842 [details] [diff] [review]
Test fix & added a new test; r=smaug

Added a send(); abort(); test case.
Comment 19 Ed Morley [:emorley] 2011-11-21 19:09:48 PST
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 :-)
Comment 20 Masatoshi Kimura [:emk] 2011-11-21 20:12:19 PST
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.

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