1303121 - [XHR2] Do not fire extra progress events in XHR CloseRequestWithError().

Status: RESOLVED FIXED

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

Description

[Thomas Wisniewski]

Attachment: do_not_fire_final_error_progress_events_for_xhrs.diff

The XHR spec is making a change to better-align with current browser implementations, which do not fire a final event named progress in the "request error steps" step of the send() method: https://github.com/whatwg/xhr/issues/72

Firefox recently changed behavior in bug 918703 and now fires those events, so here is a patch to remove them again. I'm doing a try run just in case I missed any mochitests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b11ee895118e

Anne, I'd like you to review the web platform test changes in this patch, as we worked on the spec change in question. Aside from the changes to progress-events-response-data-gzip.htm, it's just updating the tests which check for event ordering wherever "progress" events are involved.

In making these changes however, I've uncovered a bug with Chrome and WebKit (I have not checked Edge yet). They do not follow spec step 10.2 in the send() method: they fire upload.loadstart with total=0 and lengthComputable=false, rather than 12 and true as Firefox does. I'm veering towards this being their bug, but that means they will suddenly start failing this test until they correct it (since it's related to ProgressEvent correctness). Do you agree, or do you think this will be too contentious to just change without filing a spec issue for further discussion?

I'll request review on the non-WPT parts of the patch once we decide on this.

Comment 1

[Anne (:annevk)]

I'm not sure, that relates to https://www.w3.org/Bugs/Public/show_bug.cgi?id=25587 and I don't really have a strong opinion on how it should be solved.

Comment 2

[Thomas Wisniewski]

It seems nobody has a strong opinion, given how long this has gone unresolved. As such I would recommend one of two things:

1. just changing the spec to leave the precise values user-defined.
2. just biting the bullet and leaving the standard as-is, changing the tests so UAs know when they're wrong.

I would force the issue and go with option 2, as I prefer not having ambiguity, and the spec makes sense as-is. After all, UAs should know the values at upload.loadstart. Also, it provides more value to users to know the non-decoded size, since they can always get the decoded size after-the-fact, but cannot retroactively know how many bytes were actually transferred otherwise.

Thoughts?

Comment 3

[Anne (:annevk)]

That sounds fine to me. Perhaps also communicate that in the bug so we can close that as "WORKSFORME". If folks still object they'll have to come with a proposal for how to change things.

Comment 4

[Thomas Wisniewski]

Alright, I've added a comment to that w3 bug. My patch here will change the WPTs accordingly (once you've had a chance to review it, of course).

Comment 5

[Anne (:annevk)]

Comment on attachment 8791722 [details] [diff] [review]
do_not_fire_final_error_progress_events_for_xhrs.diff

This looks fine assuming you ran them.

Comment 6

[Thomas Wisniewski]

Yes I did, though I also just did a try run to be on the safe side (which seems fine despite the build infrastructure struggling a bit at the time): https://treeherder.mozilla.org/#/jobs?repo=try&revision=32cb7ca8bcb6

The patch still applies on inbound and central, so I'm requesting check-in.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/490c671f8047
Do not fire one last progress event on XHR errors, to match a spec change. r=annevk

Comment 8

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/51836860e20b

Somebody must have snuck something in on you pretty recently, to have had such a green try run and then gotten https://treeherder.mozilla.org/logviewer.html#?job_id=36590532&repo=mozilla-inbound

Comment 9

[Thomas Wisniewski]

Attachment: 1303121-do_not_fire_final_error_progress_events_for_xhrs.diff

Yeah, there has been a bit of churn in the related code lately, thanks for the back-out.

Here's a new version of the patch which corrects that failing test-case. It just required a change to the test-case itself, and try is passing fine again (just unrelated intermittents), so I'm carrying over r+ and re-requesting checkin.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=21d57ba4a57c

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/921fca780bc0
Do not fire one last progress event on XHR errors, to match a spec change. r=annevk

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/921fca780bc0

Comment 12

[Gerry Chang [:gchang]]

Mark 51 as fix-optional. If it's worth uplifting to 51, feel free to nominate it.