XMLHttpRequest can fire an abort event after a load event, but should not

RESOLVED FIXED in mozilla11

Status

()

defect
RESOLVED FIXED
8 years ago
4 months ago

People

(Reporter: WeirdAl, Assigned: emk)

Tracking

Trunk
mozilla11
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(4 attachments, 3 obsolete attachments)

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.
Summary: XMLHttpRequest can fire an abort event after a load event → XMLHttpRequest can fire an abort event after a load event, but should not
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?
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)
(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.
Posted patch trivial fix (obsolete) — Splinter Review
Assignee: ajvincent → VYV03354
Status: NEW → ASSIGNED
Attachment #575422 - Flags: review?(bugs)
Comment on attachment 575422 [details] [diff] [review]
trivial fix

attachment 575402 [details] [diff] [review] does not longer fail with this patch.
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 on attachment 575422 [details] [diff] [review]
trivial fix

This needs Alex' tests before landing.
Well, any tests :)
Attachment #575422 - Flags: review?(bugs) → review+
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.
 > 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
Attachment #575422 - Attachment is obsolete: true
Attachment #575665 - Flags: review+
Posted patch Test (obsolete) — Splinter Review
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.
Attachment #575666 - Flags: review?(bugs)
Posted patch Worker part (obsolete) — Splinter Review
This patch will also fix bug 676172.
Attachment #575668 - Flags: review?(bent.mozilla)
Attachment #575665 - Attachment description: patch for check in → patch for check in; r=smaug
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+
Attachment #575668 - Attachment is obsolete: true
Attachment #575731 - Flags: review+
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+
Added a send(); abort(); test case.
Attachment #575666 - Attachment is obsolete: true
Attachment #575842 - Flags: review+
Keywords: checkin-needed
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: 8 years ago
Resolution: --- → FIXED
Blocks: 676172
Component: DOM: Mozilla Extensions → DOM
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.