1179560 - Long delay between receiving a 421 response and parsing the body / returning the body to JS

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[mgunther]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.130 Safari/537.36

Steps to reproduce:

Execute the following code in FF's console:

(function() {
  var url = 'https://api.apple-cloudkit.com/database/1/iCloud.com.example.CloudKitCatalog/development/public/users/current?ckAPIToken=1c6d0ac40cf685931a021359dd217680182692e958d5c95248e36ee44696cd5d';

  var xhr = new XMLHttpRequest(); 
  xhr.open('GET', url, true); 
  xhr.onload = function() { console.log(xhr.status, xhr.responseText); }; 
  xhr.send();
})()


Actual results:

Note that you immediately see a warning: "not well-formed"

In the network tab, observe the XHR request:
-- The response headers are received and displayed promptly. 
-- The 'Timings' tab seems to indicate that the response was fully received.
-- However the columns 'Type', 'Transferred', 'Size' stay empty.

About 2 minutes later, these columns get populated and the xhr's onload handler gets triggered (console prints the status code and response text).


Expected results:

xhr.onload should have been called as soon as the response body was available.

Comment 1

[Alice0775 White]

[Tracking Requested - why for this release]: Regressed since Firefox36. 


Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c7c0cb53949d&tochange=2c56d741c3fd

Suspect: 
a2609830541a	Patrick McManus — Bug 1082261 - http 421 retry r=hurley

Comment 2

[Patrick McManus [:mcmanus]]

mgunther - thanks for the test case. There is definitely a firefox problem there, which I'll fix.

But also the apple API is also using 421 in a way inconsistent with the IANA registration. That's now defined to be "not authoritative" which is not what is meant by the service. So apple probably wants to change that to a different 4xx code.

But the bigger bug is on the firefox side. Thanks again.

Comment 3

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bab4d530447f

Comment 4

[Patrick McManus [:mcmanus]]

Attachment: some 421 retries dont work

Comment 5

[Patrick McManus [:mcmanus]]

Attachment: some 421 retries dont work

Comment 6

[Patrick McManus [:mcmanus]]

the change in comment 5 makes the error code a soft stream error, which will mean a h2 session with a 421 will keep the session alive for other transactions.

Comment 7

[u408661]

Comment on attachment 8629383 [details] [diff] [review]
some 421 retries dont work

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

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +773,5 @@
>      mWriter = nullptr;
>  
> +    if (mForceRestart) {
> +        // The forceRestart condition was dealt with on the stack, but it did not
> +        // clear the flag because nsPipe in the readsegment stack clears out

nit: this is the writesegment stack here, not readsegment :)

Comment 8

[Patrick McManus [:mcmanus]]

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

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

[Patrick McManus [:mcmanus]]

Comment on attachment 8629383 [details] [diff] [review]
some 421 retries dont work

Approval Request Comment
[Feature/regressing bug #]:1082261 (FF 38)
[User impact if declined]: sites using 421 HTTP response code might suffer long delays depending on exact response. This apparently includes api.apple-cloudkit.com. (prior to recent times 421 was undefined, but of course that does not mean it wasn't in use anywhere).
[Describe test coverage new/current, TreeHerder]: old general regression coverage and new case for specific issue here.
[Risks and why]: low risk as it only impacts uses of this (previously undefined code), but as h2 defined the code we can expect to see more of it and should handle it correctly
[String/UUID change made/needed]:none

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8629383 [details] [diff] [review]
some 421 retries dont work

Has tests, in m-c for 4 days, early in the beta cycle, taking it.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/73b08d7b5a60

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2e5f12e03d36

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Untracking for ESR38 as this does not meet the esr triage bar which primarily involves security related bug fixes and issues that have a significant end-user impact.