Closed Bug 1179560 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Networking, defect)

38 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- wontfix
firefox40 --- fixed
firefox41 --- fixed
firefox42 --- fixed
firefox-esr31 --- unaffected
firefox-esr38 - affected

People

(Reporter: mgunther, Assigned: mcmanus)

References

Details

(4 keywords)

Attachments

(1 file, 1 obsolete file)

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.
Component: Untriaged → Networking
Product: Firefox → Core
[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
Blocks: 1082261
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(mcmanus)
Keywords: regression
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.
Flags: needinfo?(mcmanus)
Attached patch some 421 retries dont work (obsolete) — Splinter Review
Attachment #8629192 - Flags: review?(hurley)
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Attachment #8629383 - Flags: review?(hurley)
Attachment #8629192 - Attachment is obsolete: true
Attachment #8629192 - Flags: review?(hurley)
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 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 :)
Attachment #8629383 - Flags: review?(hurley) → review+
https://hg.mozilla.org/mozilla-central/rev/d9ea769d96b8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
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
Attachment #8629383 - Flags: approval-mozilla-beta?
Attachment #8629383 - Flags: approval-mozilla-aurora?
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.
Attachment #8629383 - Flags: approval-mozilla-beta?
Attachment #8629383 - Flags: approval-mozilla-beta+
Attachment #8629383 - Flags: approval-mozilla-aurora?
Attachment #8629383 - Flags: approval-mozilla-aurora+
QA Whiteboard: [good first verify]
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.
You need to log in before you can comment on or make changes to this bug.