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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Alex Vincent [:WeirdAl]]

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]]

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]]

Attachment: 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][[e-mail]]]

(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]]

Attachment: trivial fix

Comment 5

[Masatoshi Kimura [:emk]]

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][[e-mail]]]

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][[e-mail]]]

Comment on attachment 575422 [details] [diff] [review]
trivial fix

This needs Alex' tests before landing.
Well, any tests :)

Comment 8

[Alex Vincent [:WeirdAl]]

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][[e-mail]]]

 > 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]]

Attachment: patch for check in; r=smaug

Comment 11

[Masatoshi Kimura [:emk]]

Attachment: 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]]

Attachment: Worker part

This patch will also fix bug 676172.

Comment 13

[Masatoshi Kimura [:emk]]

https://tbpl.mozilla.org/?tree=Try&rev=b46057874895

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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]]

Attachment: Worker part, for check in; r=bent

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

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]]

Attachment: Test fix & added a new test; r=smaug

Added a send(); abort(); test case.

Comment 18

[Scott Johnson (:jwir3)]

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

Comment 19

[Ed Morley [:emorley]]

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]]

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.