1088850 - disable http/1 framing enforcement from 237623

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

in bug 237623 we required correct http framing for http 1.1 and greater. That went to release channel with firefox 33.

There weren't any significant interop problems through beta, but release has brought 10+ interop problems - some with state agencies and large organizations like Blue Cross.

these are all server bugs, most of them have been verified by hand to be server bugs, but the interop problem is larger than I'm comfortable with.

So I'm going to change the framing check to apply to only http2 (or spdy) and greater - where we have a chance to prevent bad defacto behavior from sneaking in.

We can uplift this change.

Separately, we'll reopen 237623 and look for another route to address the issue with the longstanding issue with the download manager that this had fixed.

Comment 1

[Patrick McManus [:mcmanus]]

There are two classes of common errors we've seen

1] is confusion over the correct content length when applying gzip encoding. The spec is clear that the right content length is the encoded length, but some resources send the identity content length but only the encoded number of bytes in the message body

2] chunked encoding responses missing the terminating zero chunk. We're seeing this a lot with PDF - It appears Crystal reports has a aspx module that they distribute that creates these non compliant responses. Even if they were to fix the software (and maybe they have?) these old binaries would live on and on and on integrated into back offices.

Comment 2

[Patrick McManus [:mcmanus]]

its worth noting that even with the reversion, resources with the above problem have always had serious interoperability problems in conjunction with persistent connections

Comment 3

[Patrick McManus [:mcmanus]]

Your results will be at https://tbpl.mozilla.org/?tree=Try&rev=97f55b9fa8e1

Comment 4

[Patrick McManus [:mcmanus]]

Attachment: disable http/1 framing enforcement from bug 237623

Comment 5

[Patrick McManus [:mcmanus]]

[Tracking Requested - why for this release]: significant interop problem discovered with non compliant servers in release 33

Comment 6

[Daniel Stenberg [:bagder]]

Comment on attachment 8511295 [details] [diff] [review]
disable http/1 framing enforcement from bug 237623

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

I'm disappointed this is where we ended up but what can we do. I really like the mConnection->DontReuse() part that at least makes things better than they used to be before 237623.

This patch looks good.

Comment 7

[Patrick McManus [:mcmanus]]

try shows some more negative test coverage that needs to be adapted to the more tolerant nature of the patch..

Comment 8

[Patrick McManus [:mcmanus]]

just set some prefs in tests so they test what they mean to

Your results will be at https://tbpl.mozilla.org/?tree=Try&rev=f272d1237223

Comment 9

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8217c96e099e

Comment 10

[Patrick McManus [:mcmanus]]

Attachment: disable http/1 framing enforcement from bug 237623

Comment 11

[Patrick McManus [:mcmanus]]

Comment on attachment 8511584 [details] [diff] [review]
disable http/1 framing enforcement from bug 237623

Approval Request Comment

See bug description.

[Feature/regressing bug #]: 237623 - this is a partial reversion
[User impact if declined]: interop problems - can't load some css/js and pdfs
[Describe test coverage new/current, TBPL]: there is significant tbpl coverage for both this state and the previous one (separated by a pref). The issue wasn't a bug - it was interop with broken servers that we did not see even on beta.
[Risks and why]: very low- its a reversion. The biggest downside is that long standing problems with the download manager will return.
[String/UUID change made/needed]: none

I put a?release on the diff - its a close call if we would want to queue this up as a ridealong. I'm happy to let the release managers decide.

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8217c96e099e

Comment 13

[IU]

Could this be responsible for this new crash?

bp-9b88a94b-6440-48b6-bd09-df8e82141027

[@ mozilla::net::Http2Session::CleanupStream(mozilla::net::Http2Stream*, tag_nsresult, mozilla::net::Http2Session::errorType) ]

I get it each time following page takes too long to complete downloading and I click the stop button: http://www.cyberciti.biz/faq/bash-for-loop/

Other users also reporting crashes in the mozillazine nightly thread, starting with this post: http://forums.mozillazine.org/viewtopic.php?p=13842413#p13842413

Comment 14

[Patrick McManus [:mcmanus]]

(In reply to IU from comment #13)
> Could this be responsible for this new crash?
> 

very unlikely. That was probably 865314 (unrelated much riskier thing)

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8511584 [details] [diff] [review]
disable http/1 framing enforcement from bug 237623

Approving for Aurora/Beta.

Flagging ni? on SUMO so they are aware that we'll be seeing bug 237623 again in FF34 release.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c07d31725629
https://hg.mozilla.org/releases/mozilla-beta/rev/d9496ec99e83

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8511584 [details] [diff] [review]
disable http/1 framing enforcement from bug 237623

We had bug 237623 forever. This can wait a little bit longer...
Taking it for 33.1.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/b4f797f3cd52