Hallvord R. M. Steen [:hallvors] Reporter
	Description • 11 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

	Hallvord R. M. Steen [:hallvors] Reporter
	Comment 1 • 11 years ago

Hm, maybe I was mistaken in saying abort-event-loadend.htm was affected by this issue. Seems to pass just fine.

	Hallvord R. M. Steen [:hallvors] Reporter
	Comment 2 • 11 years ago

response-data-progress.htm also seems to pass just fine now. Maybe it was fixed already?

	Thomas Wisniewski Assignee
	Comment 3 • 8 years ago

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.");
         }
     });

	Thomas Wisniewski Assignee
	Comment 4 • 8 years ago

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.

	Thomas Wisniewski Assignee
	Comment 5 • 8 years ago

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.

	Thomas Wisniewski Assignee
	Comment 6 • 8 years 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.

	Thomas Wisniewski Assignee
	Comment 7 • 8 years ago

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.

	Thomas Wisniewski Assignee
	Comment 8 • 8 years ago

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.

	Thomas Wisniewski Assignee
	Comment 9 • 8 years ago

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

	Andrea Marchesini [:baku]
	Comment 10 • 8 years ago

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.

	Andrea Marchesini [:baku]
	Comment 11 • 8 years ago

wisniewskit can you rebase these patches on top of m-i and ask for a new review request? Thanks!

	Thomas Wisniewski Assignee
	Comment 12 • 8 years ago

Sure, here's a new patchset against m-i.

	Andrea Marchesini [:baku]
	Comment 17 • 8 years ago

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.

	Thomas Wisniewski Assignee
	Comment 18 • 8 years 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).

	Andrea Marchesini [:baku]
	Comment 19 • 8 years ago

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.

	Andrea Marchesini [:baku]
	Comment 20 • 8 years ago

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.

	Thomas Wisniewski Assignee
	Comment 21 • 8 years 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.

	Thomas Wisniewski Assignee
	Comment 22 • 8 years ago

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

	Pulsebot
	Comment 24 • 8 years 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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 25 • 8 years ago

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?

	Thomas Wisniewski Assignee
	Comment 26 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 27 • 8 years ago

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

	Thomas Wisniewski Assignee
	Comment 28 • 8 years 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.

	Boris Zbarsky [:bzbarsky]
	Comment 29 • 8 years ago

Thomas, is this ready to reland now?

	Thomas Wisniewski Assignee
	Comment 30 • 8 years 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.

	Thomas Wisniewski Assignee
	Comment 31 • 8 years ago

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

	Pulsebot
	Comment 33 • 8 years 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

	Carsten Book [:Tomcat]
	Comment 34 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/647a26ff5012
https://hg.mozilla.org/mozilla-central/rev/3b2b06d9c908

	Boris Zbarsky [:bzbarsky]
	Comment 35 • 8 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 36 • 8 years ago

Oh, and not only is it sitting around, but I would expect it to start failing if someone just updates the manifest blindly...

	Thomas Wisniewski Assignee
	Comment 37 • 8 years 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?

	Boris Zbarsky [:bzbarsky]
	Comment 38 • 8 years ago

New bug is probably better.