disable http/1 framing enforcement from 237623

RESOLVED FIXED in Firefox 33

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

32 Branch
mozilla36
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox33+ fixed, firefox34+ fixed, firefox35+ fixed, firefox36 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
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.
(Assignee)

Comment 2

4 years ago
its worth noting that even with the reversion, resources with the above problem have always had serious interoperability problems in conjunction with persistent connections
(Assignee)

Comment 4

4 years ago
Created attachment 8511295 [details] [diff] [review]
disable http/1 framing enforcement from bug 237623
Attachment #8511295 - Flags: review?(daniel)
(Assignee)

Updated

4 years ago
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
(Assignee)

Comment 5

4 years ago
[Tracking Requested - why for this release]: significant interop problem discovered with non compliant servers in release 33
status-firefox33: --- → affected
status-firefox34: --- → affected
status-firefox35: --- → affected
status-firefox36: --- → affected
tracking-firefox34: --- → ?
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.
Attachment #8511295 - Flags: review?(daniel) → review+
(Assignee)

Comment 7

4 years ago
try shows some more negative test coverage that needs to be adapted to the more tolerant nature of the patch..
(Assignee)

Comment 8

4 years ago
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
(Assignee)

Comment 10

4 years ago
Created attachment 8511584 [details] [diff] [review]
disable http/1 framing enforcement from bug 237623
(Assignee)

Updated

4 years ago
Attachment #8511295 - Attachment is obsolete: true
(Assignee)

Comment 11

4 years ago
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.
Attachment #8511584 - Flags: review+
Attachment #8511584 - Flags: approval-mozilla-release?
Attachment #8511584 - Flags: approval-mozilla-beta?
Attachment #8511584 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/8217c96e099e
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
status-firefox36: affected → fixed
tracking-firefox33: --- → +
tracking-firefox34: ? → +
tracking-firefox35: --- → +

Comment 13

4 years ago
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
(Assignee)

Updated

4 years ago
Blocks: 1088940
(Assignee)

Comment 14

4 years ago
(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 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.
Flags: needinfo?(tdowner)
Attachment #8511584 - Flags: approval-mozilla-beta?
Attachment #8511584 - Flags: approval-mozilla-beta+
Attachment #8511584 - Flags: approval-mozilla-aurora?
Attachment #8511584 - Flags: approval-mozilla-aurora+
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.
Attachment #8511584 - Flags: approval-mozilla-release? → approval-mozilla-release+

Updated

4 years ago
Blocks: 1083740

Updated

4 years ago
Depends on: 1095804
Flags: needinfo?(tdowner)
You need to log in before you can comment on or make changes to this bug.