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

RESOLVED FIXED in mozilla11

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: WeirdAl, Assigned: emk)

Tracking

Trunk
mozilla11
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

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

6 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

6 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

6 years ago
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.
Attachment #575402 - Flags: review?(bugs)

Comment 3

6 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

6 years ago
Created attachment 575422 [details] [diff] [review]
trivial fix
Assignee: ajvincent → VYV03354
Status: NEW → ASSIGNED
Attachment #575422 - Flags: review?(bugs)
(Assignee)

Comment 5

6 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

6 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

6 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

6 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

6 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

6 years ago
Created attachment 575665 [details] [diff] [review]
patch for check in; r=smaug
Attachment #575422 - Attachment is obsolete: true
Attachment #575665 - Flags: review+
(Assignee)

Comment 11

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

Updated

6 years ago
Attachment #575666 - Flags: review?(bugs)
(Assignee)

Comment 12

6 years ago
Created attachment 575668 [details] [diff] [review]
Worker part

This patch will also fix bug 676172.
Attachment #575668 - Flags: review?(bent.mozilla)
(Assignee)

Updated

6 years ago
Attachment #575665 - Attachment description: patch for check in → patch for check in; r=smaug
(Assignee)

Comment 13

6 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

6 years ago
Created attachment 575731 [details] [diff] [review]
Worker part, for check in; r=bent
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+
(Assignee)

Comment 17

6 years ago
Created attachment 575842 [details] [diff] [review]
Test fix & added a new test; r=smaug

Added a send(); abort(); test case.
Attachment #575666 - Attachment is obsolete: true
Attachment #575842 - Flags: review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
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
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

6 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
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Blocks: 676172
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.