[XHR2] Too few progress events being fired

RESOLVED FIXED in Firefox 51

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: hallvors, Assigned: Thomas Wisniewski)

Tracking

unspecified
mozilla51
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(URL)

Attachments

(2 attachments, 12 obsolete attachments)

15.87 KB, patch
Details | Diff | Splinter Review
78.47 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
Failures in these test cases indicate that Gecko doesn't fire all progress events the spec mandates:
http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/abort-after-send.htm
http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/abort-during-upload.htm
http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/abort-event-loadend.htm
http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/response-data-progress.htm

I haven't dug in to figure out exactly what detail in the spec isn't implemented, but I note that for example calling abort() must cause a progress event to fire per step 2.5: http://xhr.spec.whatwg.org/#the-abort%28%29-method
(Reporter)

Comment 1

4 years ago
Hm, maybe I was mistaken in saying abort-event-loadend.htm was affected by this issue. Seems to pass just fine.
(Reporter)

Comment 2

4 years ago
response-data-progress.htm also seems to pass just fine now. Maybe it was fixed already?
(Assignee)

Comment 3

a year ago
Created attachment 8760018 [details] [diff] [review]
918703-fire-xhr-events-in-the-correct-order-during-errors.diff

Those two web platform tests are indeed passing, but the other two are not. Here's a patch that orders the events as the tests expect, setting the progress values to 0,0 as per https://xhr.spec.whatwg.org/#request-error-steps .

I would also recommend changing response-data-progress.htm so that it doesn't have to time out to pass; for instance:

--- a/XMLHttpRequest/response-data-progress.htm
+++ b/XMLHttpRequest/response-data-progress.htm
@@ -19,14 +19,15 @@ var test = async_test();
 
 test.step(function() {
     var client = new XMLHttpRequest();
-    var lastSize = 0;
+    var lastSize = 0, passed = false;
 
     client.onprogress = test.step_func(function() {
         var currentSize = client.responseText.length;
 
         if (lastSize > 0 && currentSize > lastSize) {
             // growth from a positive size to bigger!
-
+            passed = true;
+            try { client.abort(); } catch(ex) {}
             test.done();
         }
 
@@ -34,7 +35,7 @@ test.step(function() {
     });
 
     client.onreadystatechange = test.step_func(function() {
-        if (client.readyState === 4) {
+        if (client.readyState === 4 && !passed) {
             assert_unreached("onprogress not called multiple times, or response body did not grow.");
         }
     });
Attachment #8760018 - Flags: review?(jonas)
(Assignee)

Comment 4

a year ago
Created attachment 8760087 [details] [diff] [review]
918703-fire-xhr-events-in-the-correct-order-during-errors.diff

Sorry for the bugspam; I've updated the patch to remove a now-useless line.

I'm also going to try another reviewer, since :sicking seems to have a lot of reviews on his plate.

Also note that this patch fixes bug 918704 as well.
Attachment #8760018 - Attachment is obsolete: true
Attachment #8760018 - Flags: review?(jonas)
Attachment #8760087 - Flags: review?(jst)
(Assignee)

Comment 5

a year ago
Created attachment 8760094 [details] [diff] [review]
918703-fire-xhr-events-in-the-correct-order-during-errors.diff

And sorry again for the bugspam. I didn't catch that one more test was now passing (XMLHttpRequest/send-timeout-events.htm), which incidentally means that bug 1028934 is also fixed by this patch.
Attachment #8760087 - Attachment is obsolete: true
Attachment #8760087 - Flags: review?(jst)
Attachment #8760094 - Flags: review?(jst)
(Assignee)

Comment 6

a year ago
A try run shows that one related test fails, because it tests that events are ordered in a specific way: https://treeherder.mozilla.org/#/jobs?repo=try&revision=086763e93c4f

I'm having a bit of trouble running the test locally, though. It keeps crashing with a weird "FATAL ERROR: Non-local network connections are disabled and a connection attempt to nosuchdomain.localhost" message, and it's a bit of an intricate test on top of that, so I'll probably need some time to correct it.
Attachment #8760094 - Flags: review?(jst) → review+
(Assignee)

Comment 7

a year ago
Created attachment 8765689 [details] [diff] [review]
918703-part1-use-enum-and-const-for-some-strings.diff

Even with the above patch, there are a number of issues with XHR ProgressEvents, and fixing each one on its own felt overly convoluted. As such I decided to just deal with them all here in this incoming patchset, as the real issue making things tricky was having a central MaybeDispatchProgressEvents() function that tried to handle everything, and was confusing in the details. These patches remove that function, and just have each callsite do what it needs to do. Then I adjust each place in the code to match the order of events in the spec, and which values their attributes should have.

Part 1 - This patch just replaces some uses of strings with consts and enums, which simplifies the subsequent patches and is less wasteful.
Attachment #8760094 - Attachment is obsolete: true
Attachment #8765689 - Flags: review?(amarchesini)
(Assignee)

Comment 8

a year ago
Created attachment 8765690 [details] [diff] [review]
918703-part2-replace-MaybeDispatchProgressEvents-with-simpler-code.diff

Part 2 - This patch actually gets rid of MaybeDispatchProgressEvents() and replaces each callsite with the actual code required in its location (if any). The patch was sent through a full try run to make sure it didn't change behavior in an unexpected way, and it passed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1681e9ef6da1

Note that the chunked-response truncation is also moved to DispatchProgressEvent(), where it can be performed every time a download progress event fires.

Similarly, a patch-hunk is added in the appropriate place to follow the spec's "security considerations" section while keeping that code in one obvious place.
Attachment #8765690 - Flags: review?(amarchesini)
(Assignee)

Comment 9

a year ago
Created attachment 8765692 [details] [diff] [review]
918703-part3-send-ProgressEvents-in-spec-order-and-with-spec-values.diff

Part 3 - This patch actually makes ProgressEvents fire in the spec order and with the specced values for total, length, and lengthComputable. Note that :jst already reviewed the bit of this patch that handles the case this specific bug is about.

Note that in the spec, lengthComputable is actually simply set at the time each ProgressEvent event is initialized, to "length != 0". As such we don't have to keep track of that value as was currently being done in the code, letting me remove a parameter to DispatchProgressEvent() and also remove a couple of instance variables.

The bulk of this patch is really about updating the mochitests and improving the web platform tests (as such I'm asking for two reviewers here, one for the WPTs and one for the rest of the patch).

Several mochitests had to be fixed to expect the spec's expected values for lengthComputable. In addition, the test for bug 435425 also had to be adjusted to expect the correct sequence of ProgressEvents (I also changed it to handle multiple progress events properly, since there's no guarantee there will only be one). The test for bug 1063538 was also calling finish more than once now, due to there being a second progress event after it aborted (per spec), so I just modded its logic to expect one progress event before the abort.

As for the web platform tests, I added a couple of new ones and adjusted the existing ones so they all check the events that are fired more thoroughly. Unexpected events, the order of each event among the readystates, and the values for loaded, total, and lengthComputable are now all checked according to what the spec says they ought to be. I also changed event-error.html so that it doesn't crash in the Firefox test harness by accessing a non-existent URL.

The full patchset also passes try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=98f3f78b8338
Attachment #8765692 - Flags: review?(hsteen)
Attachment #8765692 - Flags: review?(amarchesini)
Comment on attachment 8765689 [details] [diff] [review]
918703-part1-use-enum-and-const-for-some-strings.diff

Review of attachment 8765689 [details] [diff] [review]:
-----------------------------------------------------------------

This patch looks good, but it doesn't apply to m-i. Just yesterday I landed a huge amount of code to move xhr in dom/xhr and remove the worker+main-thread binding.
Can you send a new patch for m-i?

::: dom/base/nsXMLHttpRequest.cpp
@@ +1416,5 @@
>    init.mLengthComputable = aLengthComputable;
>    init.mLoaded = aLoaded;
>    init.mTotal = (aTotal == -1) ? 0 : aTotal;
>  
> +  const nsAString& aTypeString = ProgressEventTypeStrings[aType];

typeString

::: dom/base/nsXMLHttpRequest.h
@@ +177,5 @@
>  };
>  
>  class nsXMLHttpRequestXPCOMifier;
>  
> +enum ProgressEventType {

Move this into class XMLHttpRequest.

@@ +187,5 @@
> +  eProgressEvent_load,
> +  eProgressEvent_loadend
> +};
> +
> +const nsLiteralString ProgressEventTypeStrings[] = {

This doesn't need to stay here. Move it to the C++ in an anonymous namespace.

@@ +199,5 @@
> +};
> +
> +const nsString kLiteralString_readystatechange = NS_LITERAL_STRING("readystatechange");
> +const nsString kLiteralString_xmlhttprequest = NS_LITERAL_STRING("xmlhttprequest");
> +const nsString kLiteralString_DOMContentLoaded = NS_LITERAL_STRING("DOMContentLoaded");

same for these 3.
Attachment #8765689 - Flags: review?(amarchesini)
wisniewskit can you rebase these patches on top of m-i and ask for a new review request? Thanks!
Flags: needinfo?(wisniewskit)
Attachment #8765690 - Flags: review?(amarchesini)
Attachment #8765692 - Flags: review?(amarchesini)
(Assignee)

Comment 12

a year ago
Created attachment 8766092 [details] [diff] [review]
918703-part1-use-enum-and-const-for-some-strings.diff

Sure, here's a new patchset against m-i.
Attachment #8765689 - Attachment is obsolete: true
Flags: needinfo?(wisniewskit)
Attachment #8766092 - Flags: review?(amarchesini)
(Assignee)

Comment 13

a year ago
Created attachment 8766093 [details] [diff] [review]
918703-part2-replace-MaybeDispatchProgressEvents-with-simpler-code.diff
Attachment #8765690 - Attachment is obsolete: true
Attachment #8766093 - Flags: review?(amarchesini)
(Assignee)

Comment 14

a year ago
Created attachment 8766094 [details] [diff] [review]
918703-part3-send-ProgressEvents-in-spec-order-and-with-spec-values.diff
Attachment #8765692 - Attachment is obsolete: true
Attachment #8765692 - Flags: review?(hsteen)
Attachment #8766094 - Flags: review?(amarchesini)

Updated

a year ago
Duplicate of this bug: 1028934

Updated

a year ago
Duplicate of this bug: 1028934
Comment on attachment 8766092 [details] [diff] [review]
918703-part1-use-enum-and-const-for-some-strings.diff

Review of attachment 8766092 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +104,5 @@
> +    NS_LITERAL_STRING("timeout"),
> +    NS_LITERAL_STRING("load"),
> +    NS_LITERAL_STRING("loadend")
> +  };
> +

add a static_assert comparing the length of this array with the last enum value of ProgressEventType.
Attachment #8766092 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 18

a year ago
Comment on attachment 8766092 [details] [diff] [review]
918703-part1-use-enum-and-const-for-some-strings.diff

Ah, sorry baku, I forgot to mark this patch as obsolete. We've already checked it in as part of the XHR refactor bug 1285036 (with your comments addressed).
Attachment #8766092 - Attachment is obsolete: true
Attachment #8766092 - Flags: review+
Comment on attachment 8766093 [details] [diff] [review]
918703-part2-replace-MaybeDispatchProgressEvents-with-simpler-code.diff

Review of attachment 8766093 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay. XHR is not easy code to read.

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +1815,5 @@
>        (mState & XML_HTTP_REQUEST_ASYNC)) {
> +    StopProgressEventTimer();
> +
> +    mUploadTransferred = mUploadTotal;
> +

You don't change mUploadLengthComputable. But this point it should be true. Correct?
Do we want to add an assertion about this?

@@ +2149,5 @@
>  XMLHttpRequestMainThread::ChangeStateToDone()
>  {
> +  StopProgressEventTimer();
> +
> +  NS_ASSERTION(!(mState & XML_HTTP_REQUEST_PARSEBODY),

MOZ_ASSERT

@@ +2152,5 @@
> +
> +  NS_ASSERTION(!(mState & XML_HTTP_REQUEST_PARSEBODY),
> +               "ChangeStateToDone() called before async HTML parsing is done.");
> +
> +  if (mProgressSinceLastProgressEvent && !mErrorLoad &&

Write a comment about why we are dispatching this additional progress event.
Attachment #8766093 - Flags: review?(amarchesini) → review+
Comment on attachment 8766094 [details] [diff] [review]
918703-part3-send-ProgressEvents-in-spec-order-and-with-spec-values.diff

Review of attachment 8766094 [details] [diff] [review]:
-----------------------------------------------------------------

wow, thanks for doing this!

::: dom/base/test/test_bug435425.html
@@ +406,2 @@
>                         {target: XHR, type: "error", optional: false},
> +                       {target: XHR, type: "loadend", optional: false}]},

do we have any optional: true? if not, remove them all and remove the check as well.

::: dom/workers/test/test_bug1063538.html
@@ +26,5 @@
>    var worker = new Worker("bug1063538_worker.js");
>  
>    worker.onmessage = function(e) {
>      if (e.data.type == 'finish') {
> +      ok(e.data.progressFired, "Progress was fired.\n");

remove '\n' :)

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +2090,5 @@
>          return rv.StealNSResult();
>        }
>      }
> +
> +    mLoadTotal = mResponseBlob->GetSize(rv);

this can fail. You should check the error value:

if (NS_WARN_IF(rv.Failed())) {
  status = rv.StealNSResult();
}

::: dom/xhr/tests/test_xhr_progressevents.html
@@ +237,5 @@
>          if (responseType.chunked) {
>            xhr.responseType;
>            is(xhr.response, null, "chunked data has null response for " + testState.name);
>          }
>        

remove these extra spaces.

@@ +246,5 @@
>  
>          if (responseType.chunked) {
>            is(xhr.response, null, "chunked data has null response for " + testState.name);
>          }
>        

remove these extra spaces.
Attachment #8766094 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 21

a year ago
>Sorry for the delay. XHR is not easy code to read.

No worries!


>You don't change mUploadLengthComputable. But this point it should be true. Correct?
Do we want to add an assertion about this?

Sure, I don't mind adding that line there just in case, but in part 3 I remove the code trying to keep track of lengthComputable anyhow, since per spec it's a simple derived value set once at event-init-time:

>To fire a progress event named e given transmitted and length,
>if length is not 0, set the lengthComputable attribute value to true [otherwise set it to false]


I'll address your other comments ASAP and do a fresh try-run before requesting check-in.
(Assignee)

Comment 22

a year ago
Created attachment 8771718 [details] [diff] [review]
918703-1-replace-MaybeDispatchProgressEvents-with-simpler-code.diff

Incoming simple rebases of the patches.

The MOZ_ASSERT change in the last patch was a great suggestion, as it immediately uncovered that the flag had never been being unset in the successful case. I don't think the minor tweaks necessary to un-set the flag are enough to warrant a re-review though, so I'm carrying over r+.

A fresh try run only shows unrelated intermittents: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b4490e73584
Assignee: nobody → wisniewskit
Attachment #8766093 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 23

a year ago
Created attachment 8771719 [details] [diff] [review]
918703-2-send-ProgressEvents-in-spec-order-and-with-spec-values.diff
Attachment #8766094 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 24

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c0425073111
Part 1: Remove MaybeDispatchProgressEvents() and have its callsites only do what they need to do, to make the XHR code more readable. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/f52c26d677b0
Part 2: Correct progress event logic so events are sent in the correct order and with the correct values according to spec. r=baku
Keywords: checkin-needed
Backed out for failing mochitest test_bug435425.html:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8201a97184f2f6cdcd70f19d8d6fc90e5589f33
https://hg.mozilla.org/integration/mozilla-inbound/rev/33e438fbb442212265493d533b3794a29308b981

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1d657d984bed365b1c2bdc499532cf34c60b1edd
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=31920884&repo=mozilla-inbound

03:02:52     INFO -  837 INFO TEST-PASS | dom/base/test/test_bug435425.html | Wrong event!
03:02:52     INFO -  838 INFO TEST-PASS | dom/base/test/test_bug435425.html | Wrong event target [[object XMLHttpRequest],progress]!
03:02:52     INFO -  839 INFO TEST-PASS | dom/base/test/test_bug435425.html | Extra or wrong event?
03:02:52     INFO -  840 INFO TEST-UNEXPECTED-FAIL | dom/base/test/test_bug435425.html | Wrong event! - got "progress", expected "load"
03:02:52     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:268:5
03:02:52     INFO -      logEvent@dom/base/test/test_bug435425.html:51:3
03:02:52     INFO -      start/xhr.onprogress@dom/base/test/test_bug435425.html:134:7
03:02:52     INFO -      EventHandlerNonNull*start@dom/base/test/test_bug435425.html:133:1
03:02:52     INFO -      runTest@dom/base/test/test_bug435425.html:412:3
03:02:52     INFO -      @SimpleTest/SimpleTest.js:622:1

Does this test need just an update?
Flags: needinfo?(wisniewskit)
(Assignee)

Comment 26

a year ago
Created attachment 8771789 [details] [diff] [review]
918703-2-send-ProgressEvents-in-spec-order-and-with-spec-values.diff

>Does this test need just an update?

Yeah, here's an updated version of the second patch. It just needed a further tweak on top of what I had already changed. Basically I just had to move a three-line chunk of test logic down below the next code-block, so that the "multiple progress events in a row" case was being tested properly (which can happen intermittently, by design).

And while I was driving by, I also noticed that the test was never actually running its final case, so I fixed the off-by-one error causing that.

Neither of these changes are big enough that I suspect they should be re-reviewed, so I'm carrying over r+ and re-requesting checkin.
Attachment #8771719 - Attachment is obsolete: true
Flags: needinfo?(wisniewskit)
(Assignee)

Updated

a year ago
Keywords: checkin-needed
has conflicts like 

renamed 918703 -> 918703-2-send-ProgressEvents-in-spec-order-and-with-spec-values.diff
applying 918703-2-send-ProgressEvents-in-spec-order-and-with-spec-values.diff
patching file dom/xhr/XMLHttpRequestMainThread.cpp
Hunk #2 FAILED at 1006
1 out of 12 hunks FAILED -- saving rejects to file dom/xhr/XMLHttpRequestMainThread.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 918703-2-send-ProgressEvents-in-spec-order-and-with-spec-values.diff
Flags: needinfo?(wisniewskit)
Keywords: checkin-needed
(Assignee)

Comment 28

a year ago
Ah, my apologies. I didn't notice that the specific checkins I requested would clash. I'll keep a closer eye on that from now on. I'll wait for the patch in 447689 to stick before re-requesting checkin on this and the patches in bug 1285036.
Flags: needinfo?(wisniewskit)
Thomas, is this ready to reland now?
Flags: needinfo?(wisniewskit)
(Assignee)

Comment 30

a year ago
It was waiting to see whether a couple of patches stick their landing in bug 1285036 to prevent merge conflicts, but it sounds reasonable that these patches should also handle the issue uncovered in bug 1254382 comments 14-16.
Flags: needinfo?(wisniewskit)
(Assignee)

Comment 31

a year ago
Created attachment 8778527 [details] [diff] [review]
918703-1-replace-MaybeDispatchProgressEvents-with-simpler-code.diff

Since it turns out that bug 1285036 is essentially unrelated to these patches, I've rebased them and will request check-in.

There are no significant changes other than rebasing (just a tweak or two to tests), so I'm carrying over r+.

A fresh try run seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a830f17ede59
Attachment #8771718 - Attachment is obsolete: true
(Assignee)

Comment 32

a year ago
Created attachment 8778528 [details] [diff] [review]
918703-2-send-ProgressEvents-in-spec-order-and-with-spec-values.diff
Attachment #8771789 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 33

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/647a26ff5012
Part 1: Remove MaybeDispatchProgressEvents() and have its callsites only do what they need to do, to make the XHR code more readable. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b2b06d9c908
Part 2: Correct progress event logic so events are sent in the correct order and with the correct values according to spec. r=baku
Keywords: checkin-needed

Comment 34

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/647a26ff5012
https://hg.mozilla.org/mozilla-central/rev/3b2b06d9c908
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
The commit for this bug changed the manifest entry from XMLHttpRequest/event-error.html to XMLHttpRequest/event-error.sub.html but instead of renaming the file just added a the .sub version (not as an hg cp, even).  In particular, XMLHttpRequest/event-error.html is still sitting around.  Is that expected?
Flags: needinfo?(wisniewskit)
Oh, and not only is it sitting around, but I would expect it to start failing if someone just updates the manifest blindly...
(Assignee)

Comment 37

a year ago
That was indeed an oversight on my part; the non-sub version of the file should have been removed. Thanks for double-checking on that! Would it be alright to reopen this bug and do a follow-up patch here, or is it best to file a new bug for this?
Flags: needinfo?(wisniewskit) → needinfo?(bzbarsky)
New bug is probably better.
Flags: needinfo?(bzbarsky)
(Assignee)

Updated

a year ago
Depends on: 1298273
You need to log in before you can comment on or make changes to this bug.