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.

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

XHR - Trigger readystatechange when transitioning from HEADERS_RECEIVED to DONE #3917

Merged
merged 2 commits into from Nov 7, 2014

Conversation

@mukilan
Copy link
Contributor

mukilan commented Nov 6, 2014

Fixes #3877

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Nov 6, 2014

Critic review: https://critic.hoppipolla.co.uk/r/3108

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@mukilan
Copy link
Contributor Author

mukilan commented Nov 6, 2014

The fix required updating the expectation of several test. I've explained the reasoning below:

/XMLHttpRequest/status-async.htm
--------------------------------
FAIL XMLHttpRequest: status/statusText - various responses 7 (GET 402)
FAIL XMLHttpRequest: status/statusText - various responses 9 (CHICKEN 402)

Both tests read the responseXML, which we do not support yet.

/XMLHttpRequest/anonymous-mode-unsupported.htm
----------------------------------------------
FAIL expected TIMEOUT XMLHttpRequest: anonymous mode unsupported
OK expected TIMEOUT [Parent]

We are not sending document.cookies as of now even for same-origin request. The test expects to find the cookie, otherwise assumes we support anonymous:true and fails.

/XMLHttpRequest/open-after-setrequestheader.htm
-----------------------------------------------
PASS expected TIMEOUT XMLHttpRequest: open() after setRequestHeader()
OK expected TIMEOUT [Parent]

Correct behaviour.

/XMLHttpRequest/response-json.htm
---------------------------------
FAIL JSON object roundtrip
FAIL JSON roundtrip with Japanese text

We recreate the JSON object each time xhr.response is accessed and do not cache it (which is what the test is asserting)

/XMLHttpRequest/send-entity-body-get-head-async.htm
---------------------------------------------------
FAIL expected NOTRUN XMLHttpRequest: send() - non-empty data argument and GET/HEAD - async, no upload events should fire (GET)
FAIL expected NOTRUN XMLHttpRequest: send() - non-empty data argument and GET/HEAD - async, no upload events should fire (HEAD)
OK expected TIMEOUT [Parent]

Before this fix, test.done() was never called. But now since it is called , assert_equal on the event array fails since the test asserts that no "upload" events are fired, while we do. Not sure if this is a bug in the test or in our implementation.

/XMLHttpRequest/send-no-response-event-loadend.htm
--------------------------------------------------
PASS expected TIMEOUT XMLHttpRequest: The send() method: Fire a progress event named loadend (no response entity body)
OK expected TIMEOUT [Parent]

Correct behaviour.

/XMLHttpRequest/send-no-response-event-order.htm
------------------------------------------------
PASS expected TIMEOUT XMLHttpRequest: The send() method: event order when there is no response entity body
OK expected TIMEOUT [Parent]

Correct behaviour

/XMLHttpRequest/status-error.htm
--------------------------------
PASS expected TIMEOUT XMLHttpRequest: status error handling GET 200
PASS expected TIMEOUT XMLHttpRequest: status error handling GET 400
PASS expected TIMEOUT XMLHttpRequest: status error handling GET 401
PASS expected TIMEOUT XMLHttpRequest: status error handling GET 404
PASS expected TIMEOUT XMLHttpRequest: status error handling GET 410
PASS expected TIMEOUT XMLHttpRequest: status error handling GET 500
PASS expected TIMEOUT XMLHttpRequest: status error handling GET 699
PASS expected TIMEOUT XMLHttpRequest: status error handling HEAD 200
PASS expected TIMEOUT XMLHttpRequest: status error handling HEAD 404
PASS expected TIMEOUT XMLHttpRequest: status error handling HEAD 500
PASS expected TIMEOUT XMLHttpRequest: status error handling HEAD 699
PASS expected TIMEOUT XMLHttpRequest: status error handling POST 200
PASS expected TIMEOUT XMLHttpRequest: status error handling POST 404
PASS expected TIMEOUT XMLHttpRequest: status error handling POST 500
PASS expected TIMEOUT XMLHttpRequest: status error handling POST 699
PASS expected TIMEOUT XMLHttpRequest: status error handling PUT 200
PASS expected TIMEOUT XMLHttpRequest: status error handling PUT 404
PASS expected TIMEOUT XMLHttpRequest: status error handling PUT 500
PASS expected TIMEOUT XMLHttpRequest: status error handling PUT 699
OK expected TIMEOUT [Parent]

Correct behaviour.

@Manishearth
Copy link
Member

Manishearth commented Nov 6, 2014

Filed #3919 for the JSON roundtrip thing.

@Manishearth

This comment has been minimized.

Copy link

Manishearth commented on dc02352 Nov 6, 2014

r+

This comment has been minimized.

Copy link

jdm replied Nov 6, 2014

@bors: retry

@mukilan
Copy link
Contributor Author

mukilan commented Nov 6, 2014

Filed #3920 for /XMLHttpRequest/send-entity-body-get-head-async.htm

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented on dc02352 Nov 6, 2014

saw approval from Manishearth
at mukilan@dc02352

This comment has been minimized.

Copy link
Contributor

bors-servo replied Nov 6, 2014

merging mukilan/servo/xhr-status-async = dc02352 into auto

This comment has been minimized.

Copy link
Contributor

bors-servo replied Nov 6, 2014

mukilan/servo/xhr-status-async = dc02352 merged ok, testing candidate = 7f668f2

This comment has been minimized.

Copy link
Contributor

bors-servo replied Nov 6, 2014

This comment has been minimized.

Copy link
Contributor

bors-servo replied Nov 6, 2014

saw approval from Manishearth
at mukilan@dc02352

This comment has been minimized.

Copy link
Contributor

bors-servo replied Nov 6, 2014

merging mukilan/servo/xhr-status-async = dc02352 into auto

This comment has been minimized.

Copy link
Contributor

bors-servo replied Nov 6, 2014

mukilan/servo/xhr-status-async = dc02352 merged ok, testing candidate = efac4658

This comment has been minimized.

Copy link
Contributor

bors-servo replied Nov 7, 2014

bors-servo pushed a commit that referenced this pull request Nov 6, 2014
@mukilan
Copy link
Contributor Author

mukilan commented Nov 6, 2014

In the log from buildbot:

 3:05.90 PROCESS_OUTPUT: Thread-TestrunnerManager-1 "invalid http version" (pid:90165 command:/Users/servo/buildbot/slave/mac1/build/target/servo --cpu --hard-fail http://localhost:8000/XMLHttpRequest/status-async.htm)
 3:05.90 PROCESS_OUTPUT: Thread-TestrunnerManager-1 "invalid http version" (pid:90165 command:/Users/servo/buildbot/slave/mac1/build/target/servo --cpu --hard-fail http://localhost:8000/XMLHttpRequest/status-async.htm)
 3:05.90 PROCESS_OUTPUT: Thread-TestrunnerManager-1 "invalid http version" (pid:90165 command:/Users/servo/buildbot/slave/mac1/build/target/servo --cpu --hard-fail http://localhost:8000/XMLHttpRequest/status-async.htm)
 3:05.90 PROCESS_OUTPUT: Thread-TestrunnerManager-1 "invalid http version" (pid:90165 command:/Users/servo/buildbot/slave/mac1/build/target/servo --cpu --hard-fail http://localhost:8000/XMLHttpRequest/status-async.htm)
 3:05.90 PROCESS_OUTPUT: Thread-TestrunnerManager-1 "invalid http version" (pid:90165 command:/Users/servo/buildbot/slave/mac1/build/target/servo --cpu --hard-fail http://localhost:8000/XMLHttpRequest/status-async.htm)
 3:05.90 PROCESS_OUTPUT: Thread-TestrunnerManager-1 "invalid http version" (pid:90165 command:/Users/servo/buildbot/slave/mac1/build/target/servo --cpu --hard-fail http://localhost:8000/XMLHttpRequest/status-async.htm)

What does the "invalid http verison" mean? I cannot reproduce this locally.

@jdm

This comment has been minimized.

Copy link

jdm commented on 504fdbf Nov 7, 2014

r+

This comment has been minimized.

Copy link

jdm replied Nov 7, 2014

@bors: retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented on 504fdbf Nov 7, 2014

saw approval from jdm
at mukilan@504fdbf

This comment has been minimized.

Copy link
Contributor

bors-servo replied Nov 7, 2014

merging mukilan/servo/xhr-status-async = 504fdbf into auto

This comment has been minimized.

Copy link
Contributor

bors-servo replied Nov 7, 2014

mukilan/servo/xhr-status-async = 504fdbf merged ok, testing candidate = 125c8c1

This comment has been minimized.

Copy link
Contributor

bors-servo replied Nov 7, 2014

This comment has been minimized.

Copy link
Contributor

bors-servo replied Nov 7, 2014

saw approval from jdm
at mukilan@504fdbf

This comment has been minimized.

Copy link
Contributor

bors-servo replied Nov 7, 2014

merging mukilan/servo/xhr-status-async = 504fdbf into auto

This comment has been minimized.

Copy link
Contributor

bors-servo replied Nov 7, 2014

mukilan/servo/xhr-status-async = 504fdbf merged ok, testing candidate = 338a9c3

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

bors-servo replied Nov 7, 2014

fast-forwarding master to auto = 338a9c3

bors-servo pushed a commit that referenced this pull request Nov 7, 2014
bors-servo pushed a commit that referenced this pull request Nov 7, 2014
@bors-servo bors-servo closed this Nov 7, 2014
@bors-servo bors-servo merged commit 504fdbf into servo:master Nov 7, 2014
1 check passed
1 check passed
default all tests passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.