Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

xhr/open-during-abort-processing.htm test does not seems to match the XHR spec #10217

Closed
cdumez opened this issue Mar 28, 2018 · 5 comments
Closed

xhr/open-during-abort-processing.htm test does not seems to match the XHR spec #10217

cdumez opened this issue Mar 28, 2018 · 5 comments
Labels
xhr

Comments

@cdumez
Copy link
Contributor

cdumez commented Mar 28, 2018

xhr/open-during-abort-processing.htm test does not seems to match the XHR spec. It expects the following upload events to get fired:
"upload.onabort 1",
"upload.onloadend 1",

The first request looks like this:

client.open("POST", "resources/content.py")
client.send('abcd')

Thus, yes, the request initially has a body so the following set in send():

If req’s body is null, set the upload complete flag.

Will not set the upload complete flag.

But then the test calls abort() in the loadstart event listener. After that the readystatechange event listener gets called and the test does:

log.push('onreadystatechange readyState before open() ' + client.readyState)
client.open("GET", "resources/content.py")
log.push('onreadystatechange readyState after open() ' + client.readyState)
client.send(null)

Because this is a 'GET' request, send() will set the request body to null and the upload complete flag WILL be set. The abort() steps will then keep going and it will fire the abort event via theses steps:
https://xhr.spec.whatwg.org/#request-error-steps

Because the upload complete flag is NOW set, I do not expect step 6 to fire any upload events. Yet, the test seems to expect those upload events.

My understanding is that this is the reason why this test is failing in Firefox. The test is passing in Chrome. I hit the issue when trying to align WebKit with the specification because upload events were no longer fired after my change.
@cdumez
Copy link
Contributor Author

cdumez commented Mar 28, 2018

cc @annevk

@cdumez
Copy link
Contributor Author

cdumez commented Mar 28, 2018

To summarize, if you look at those steps:

Step 5 (Fire an event named readystatechange) causes JS to be run, which can call open() / send() on the XHR again. If this happens, it will potentially change the value of the "upload complete flag" / "upload listener flag" before we reach Step 6.

This is an issue because step 6 relies on the "upload complete flag" / "upload listener flag" to decide wether on not to fire the events.

@foolip foolip added the xhr label Mar 28, 2018
hubot pushed a commit to WebKit/WebKit-http that referenced this issue Mar 29, 2018
Align XMLHttpRequest's open() / send() / abort() with the latest spec…
9f121e8 
…ification

https://bugs.webkit.org/show_bug.cgi?id=184108

Reviewed by Youenn Fablet.

LayoutTests/imported/w3c:

* web-platform-tests/XMLHttpRequest/abort-after-send-expected.txt:
* web-platform-tests/XMLHttpRequest/abort-during-open-expected.txt:
* web-platform-tests/XMLHttpRequest/abort-during-open.worker-expected.txt:
* web-platform-tests/XMLHttpRequest/abort-event-abort-expected.txt:
* web-platform-tests/XMLHttpRequest/abort-event-order-expected.txt:
* web-platform-tests/XMLHttpRequest/open-during-abort-event-expected.txt:
* web-platform-tests/XMLHttpRequest/open-during-abort-expected.txt:
* web-platform-tests/XMLHttpRequest/open-send-during-abort-expected.txt:
* web-platform-tests/XMLHttpRequest/security-consideration.sub-expected.txt:
* web-platform-tests/XMLHttpRequest/send-data-unexpected-tostring-expected.txt:
Rebaseline WPT tests that are now passing.

* web-platform-tests/XMLHttpRequest/open-during-abort-processing-expected.txt:
We now fail the test differently. Our results are consistent with Firefox. I believe this
test does not match the specification so I filed:
web-platform-tests/wpt#10217

Source/WebCore:

Align XMLHttpRequest's open() / send() / abort() with the latest specification:
- https://xhr.spec.whatwg.org

No new tests, rebaselined existing layout tests.

* xml/XMLHttpRequest.cpp:
(WebCore::XMLHttpRequest::open):
Align with https://xhr.spec.whatwg.org/#the-open()-method:
- Change the order of some steps to match the order in the spec. In particular,
  open() no longer resets the state to UNSENT or abort any existing load when it
  fails early due to being passed a bad method.

(WebCore::XMLHttpRequest::createRequest):
Align with https://xhr.spec.whatwg.org/#the-send()-method:
- Use the simpler "upload listener flag" logic from the spec instead of our more
  complex m_uploadEventsAllowed flag. This avoids constructing a SecurityOrigin
  objects on a background thread when XHR is used inside Web Workers, which was
  not thread-safe.
- Set the upload complete flag when the request has no body as per step 9.
- After firing the loadstartEvent, return early if the state is no longer OPEN or
  if the send flag is unset, as per step 11.3.

(WebCore::XMLHttpRequest::abort):
Align with https://xhr.spec.whatwg.org/#the-abort()-method:
- Only set the state to UNSENT if the state is still DONE after firing the error
  events, as per step 3.

(WebCore::XMLHttpRequest::didSendData):
Use new "upload listener flag".

(WebCore::XMLHttpRequest::dispatchErrorEvents):
Align with https://xhr.spec.whatwg.org/#request-error-steps:
- Stop firing a progress event in case of error as this is not as per specification
  and Firefox does not fire those either.

* xml/XMLHttpRequest.h:

LayoutTests:

* http/tests/xmlhttprequest/onloadend-event-after-abort.html:
* http/tests/xmlhttprequest/onloadend-event-after-error.html:
* http/tests/xmlhttprequest/simple-cross-origin-progress-events-expected.txt:
* http/tests/xmlhttprequest/upload-onloadend-event-after-abort.html:
* http/tests/xmlhttprequest/xmlhttprequest-sync-no-progress-events-expected.txt:
Fix tests that expected a progress event before error/abort event. This is not as
per specification and those tests were also failing in Firefox.

* http/tests/xmlhttprequest/readystatechange-and-abort.html:
Fix test that expected abort() to reset state to UNSENT as this is not as per specification.
This test was failing in both Firefox and Chrome.

* http/tests/xmlhttprequest/xmlhttprequest-abort-readyState-shouldNotDispatchEvent.html:
Re-sync test from Blink. The test was wrongly expecting abort() to reset the state to
UNSENT.


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@230066 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@annevk
Copy link
Member

annevk commented Mar 29, 2018

I think you're right that we should change this test. @wisniewskit thoughts?

@wisniewskit
Copy link
Contributor

wisniewskit commented Mar 29, 2018 •
edited
Loading

Interestingly, I have a new re-implementation of the XHR spec for Firefox in progress, and it's not failing this test. I'll have to investigate this a bit more closely to see why it's passing.

@wisniewskit
Copy link
Contributor

Funny enough, I had forgotten at the time I first read this issue that I had filed this bug already as #7933, and worked around it in my new XHR implementation. So yes, I agree with @cdumez, and the fix is already being considered in #7933. I'll close this as a duplicate. Sorry for not noticing sooner!

ryanhaddad pushed a commit to WebKit/WebKit that referenced this issue Dec 22, 2020
@cdumez
Align XMLHttpRequest's open() / send() / abort() with the latest spec…
d53d4b6 
…ification

https://bugs.webkit.org/show_bug.cgi?id=184108

Reviewed by Youenn Fablet.

LayoutTests/imported/w3c:

* web-platform-tests/XMLHttpRequest/abort-after-send-expected.txt:
* web-platform-tests/XMLHttpRequest/abort-during-open-expected.txt:
* web-platform-tests/XMLHttpRequest/abort-during-open.worker-expected.txt:
* web-platform-tests/XMLHttpRequest/abort-event-abort-expected.txt:
* web-platform-tests/XMLHttpRequest/abort-event-order-expected.txt:
* web-platform-tests/XMLHttpRequest/open-during-abort-event-expected.txt:
* web-platform-tests/XMLHttpRequest/open-during-abort-expected.txt:
* web-platform-tests/XMLHttpRequest/open-send-during-abort-expected.txt:
* web-platform-tests/XMLHttpRequest/security-consideration.sub-expected.txt:
* web-platform-tests/XMLHttpRequest/send-data-unexpected-tostring-expected.txt:
Rebaseline WPT tests that are now passing.

* web-platform-tests/XMLHttpRequest/open-during-abort-processing-expected.txt:
We now fail the test differently. Our results are consistent with Firefox. I believe this
test does not match the specification so I filed:
web-platform-tests/wpt#10217

Source/WebCore:

Align XMLHttpRequest's open() / send() / abort() with the latest specification:
- https://xhr.spec.whatwg.org

No new tests, rebaselined existing layout tests.

* xml/XMLHttpRequest.cpp:
(WebCore::XMLHttpRequest::open):
Align with https://xhr.spec.whatwg.org/#the-open()-method:
- Change the order of some steps to match the order in the spec. In particular,
  open() no longer resets the state to UNSENT or abort any existing load when it
  fails early due to being passed a bad method.

(WebCore::XMLHttpRequest::createRequest):
Align with https://xhr.spec.whatwg.org/#the-send()-method:
- Use the simpler "upload listener flag" logic from the spec instead of our more
  complex m_uploadEventsAllowed flag. This avoids constructing a SecurityOrigin
  objects on a background thread when XHR is used inside Web Workers, which was
  not thread-safe.
- Set the upload complete flag when the request has no body as per step 9.
- After firing the loadstartEvent, return early if the state is no longer OPEN or
  if the send flag is unset, as per step 11.3.

(WebCore::XMLHttpRequest::abort):
Align with https://xhr.spec.whatwg.org/#the-abort()-method:
- Only set the state to UNSENT if the state is still DONE after firing the error
  events, as per step 3.

(WebCore::XMLHttpRequest::didSendData):
Use new "upload listener flag".

(WebCore::XMLHttpRequest::dispatchErrorEvents):
Align with https://xhr.spec.whatwg.org/#request-error-steps:
- Stop firing a progress event in case of error as this is not as per specification
  and Firefox does not fire those either.

* xml/XMLHttpRequest.h:

LayoutTests:

* http/tests/xmlhttprequest/onloadend-event-after-abort.html:
* http/tests/xmlhttprequest/onloadend-event-after-error.html:
* http/tests/xmlhttprequest/simple-cross-origin-progress-events-expected.txt:
* http/tests/xmlhttprequest/upload-onloadend-event-after-abort.html:
* http/tests/xmlhttprequest/xmlhttprequest-sync-no-progress-events-expected.txt:
Fix tests that expected a progress event before error/abort event. This is not as
per specification and those tests were also failing in Firefox.

* http/tests/xmlhttprequest/readystatechange-and-abort.html:
Fix test that expected abort() to reset state to UNSENT as this is not as per specification.
This test was failing in both Firefox and Chrome.

* http/tests/xmlhttprequest/xmlhttprequest-abort-readyState-shouldNotDispatchEvent.html:
Re-sync test from Blink. The test was wrongly expecting abort() to reset the state to
UNSENT.


Canonical link: https://commits.webkit.org/199697@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@230066 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
xhr
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@wisniewskit @foolip @cdumez @annevk