Closed Bug 1088850 Opened 5 years ago Closed 5 years ago

disable http/1 framing enforcement from 237623

Categories

(Core :: Networking: HTTP, defect)

32 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox33 + fixed
firefox34 + fixed
firefox35 + fixed
firefox36 --- fixed

People

(Reporter: mcmanus, Assigned: mcmanus)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
its worth noting that even with the reversion, resources with the above problem have always had serious interoperability problems in conjunction with persistent connections
Attachment #8511295 - Flags: review?(daniel)
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
[Tracking Requested - why for this release]: significant interop problem discovered with non compliant servers in release 33
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+
try shows some more negative test coverage that needs to be adapted to the more tolerant nature of the patch..
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
Attachment #8511295 - Attachment is obsolete: true
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
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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
Blocks: 1088940
(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+
Blocks: 1083740
Depends on: 1095804
Flags: needinfo?(tdowner)
You need to log in before you can comment on or make changes to this bug.