1311798 - [XHR2] Align XHR abort() with the spec.

Status: RESOLVED FIXED

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

Description

[Thomas Wisniewski]

The web platform tests for XMLHttpRequest abort() are presently being updated to better-match the spec text [1, 2]. This patch updates Firefox accordingly so that the revised tests will pass. Try seems fine with it [3].

Note that I'm not 100% sure if it would be better to make both aOptionalException and aRv pointers or Optionals<> in the new RequestErrorSteps method, rather than using NS_OK as a sentinel value for "do not throw an exception". Thoughts?

Also note that the same changes to the web platform tests in this patch are already being merged remotely [4], so there is no need to review them here. Once we're ready to land this patch, I will revise it appropriately to avoid any merge conflicts on our end.

[1] https://github.com/whatwg/xhr/issues/88
[2] https://xhr.spec.whatwg.org/#the-abort()-method
[3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1183408163bc88f1b172f7cc7e502fca8800490
[4] https://github.com/w3c/web-platform-tests/pull/4033

Comment 1

[Thomas Wisniewski]

Attachment: update_xhr_abort_method_to_match_spec_requirements.diff

(adding the patch, as Bugzilla ate it in my original post).

Comment 2

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

FYI, baku is on vacation.

Comment 3

[Thomas Wisniewski]

Thanks. Would you mind doing the honors? I don't think there's any particular rush, but I also don't think it needs baku's attention specifically.

Comment 5

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

(::Abort(ErrorResult& arv)  odd naming for the argument. Should be aRv.)

Given how the spec is written, I think the approach you've taken is good enough.
Using Optional<> wouldn't really give us anything, I think.


But don't compare to NS_OK, but do NS_FAILED(aOptionalException)

Comment 6

[Andrea Marchesini [:baku]]

Comment on attachment 8803065 [details] [diff] [review]
update_xhr_abort_method_to_match_spec_requirements.diff

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

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +1065,5 @@
>  
>  void
> +XMLHttpRequestMainThread::RequestErrorSteps(const ProgressEventType aEventType,
> +                                            const nsresult aOptionalException,
> +                                            ErrorResult& aRv) {

{ in a new line.

@@ +1115,1 @@
>  XMLHttpRequestMainThread::Abort(ErrorResult& arv)

rename this to aRv

Comment 7

[Thomas Wisniewski]

Attachment: 1311798-align_xhr_abort_with_spec.diff

Thanks, baku! Review comments are addressed in this version, and a fresh try run is fine [1], so I'm carrying over r+ and requesting checkin.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cc69d558e457e23d58cdb2ebfa29d2e4cec58be

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3421c306d8cf
Align XMLHttpRequest.abort() with the spec. r=baku

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing event-readystatechange-loaded.htm and unexpected passes:

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf7e3bcf119a09e85bf221de52b9e84016c0e0d1

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

TEST-UNEXPECTED-PASS | /XMLHttpRequest/abort-during-open.htm | XMLHttpRequest: abort() during OPEN - expected FAIL
TEST-UNEXPECTED-PASS | /XMLHttpRequest/abort-during-open.worker.html | Untitled - {}
TEST-UNEXPECTED-PASS | /XMLHttpRequest/abort-event-abort.htm | XMLHttpRequest: The abort() method: do not fire abort event in OPENED state when send() flag is unset. - expected FAIL
TEST-UNEXPECTED-FAIL | /XMLHttpRequest/event-readystatechange-loaded.htm | XMLHttpRequest: the LOADING state change may be emitted multiple times - assert_equals: LOADING state change 
TEST-UNEXPECTED-PASS | /XMLHttpRequest/send-data-unexpected-tostring.htm | abort() called from data stringification - expected FAIL

Comment 10

[Thomas Wisniewski]

Attachment: 1311798-align_xhr_abort_with_spec.diff

Weird, I must have had a brain-fart and forgotten to mark those tests as passing. Here's a version rebased to inbound that does that. I can't replicate the failure, however; I wonder if it was a hiccup? Let's try again and see...

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Yes, the other jobs don't have that failure.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca8475c0038c
Align XMLHttpRequest.abort() with the spec. r=baku

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ca8475c0038c

Comment 14

[Sylvestre Ledru [:Sylvestre]]

I guess we want this to ride the train with 53.