Closed Bug 806200 Opened 10 years ago Closed 10 years ago

Don't ignore "Encoding: gzipped" header

Categories

(Core :: Networking: HTTP, defect)

16 Branch
x86_64
Linux
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: persona, Assigned: mcmanus)

Details

Attachments

(1 file)

First of all: This is a bug that only shows up when people touch about:config.

I just discovered that I had the "network.http.accept-encoding" setting set to "". Probably done long ago while debugging stuff with tcpdump (where compression obviously gets in the way). This started causing me major troubles after upgrading to Firefox 16 (from 10) which supports SPDY. I was no longer able to use Google, all it'd ever get me was raw gzipped data.

The problem appears to be that with SPDY, servers are "allowed" to serve compressed responses even if the client didn't explicitly indicate gzip support. This seems to be part of the SPDY spec (section 3.2.1):

"User-agents MUST support gzip compression. Regardless of the Accept-Encoding sent by the user-agent, the server may always send content encoded with gzip or deflate encoding."

It looks like Firefox assumes it'll never receive compressed responses if it didn't ask for them. Google's following the rule above though (and only with SPDY enabled), and does also include a "Content-Encoding: gzip" header in its response.

Does Firefox intentionally ignore that and only decompress if compressed content was requested?
Component: Untriaged → Networking: HTTP
Product: Firefox → Core
interestingly we filter out content-encodings that don't correspond to accept-encodings. As you say, deflate/gzip is implicit in spdy (we never send it on the wire) so fixing this is just a matter of enforcing it in the accept-encodings of a spdy transaction to match the spec.
Attached patch patch 0Splinter Review
This patch makes us process gzip if we have a stream converter for it. Which frankly, is what I thought we already did. I don't really understand the value of IsAcceptableEncoding().

Other strategies I rejected:

* adding to the accept-encodings http header in the spdy stream code. The major problem with this is that "IsAcceptableEncoding" checks the configured AE headers instead of the actual transaction headers so it doesn't work.

* disable spdy when accept-encoding does not contain gzip.. but that just creates a legacy problem in trying to migrate forward long term for no good reason.

Honza, if you agree I would also be ok with just removing IsAcceptableEncoding entirely.. it might have a use in a world where necko was used for things outside of gecko but I think we all agree that's not what we're focused on doing now.
Assignee: nobody → mcmanus
Attachment #676113 - Flags: review?(honzab.moz)
Comment on attachment 676113 [details] [diff] [review]
patch 0

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

Hmm.. could we do this only for SPDY channels if that is easy to determine?  r=honzab only when that would be too complicated.

Otherwise rather remove ability to modify the request header at all, I mean remove the pref and IsAcceptableEncoding.  But people may find the pref useful for testing, depends...
Attachment #676113 - Flags: review?(honzab.moz) → review+
Comment on attachment 676113 [details] [diff] [review]
patch 0

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

FWIW I did find it quite handy to be able to suppress gzipped encoding when doing some debugging. But I'm an old-fashioned person who likes to use tcpdump. (But also, in the end having changed that setting long ago was a delayed shot in my own foot.)
I am going to plead too-hard here. The connection isn't necessarily available, this could be coming from cache for instance, and the injected spdy header isn't realiable for anything more than a convenient debugging hint.

I don't think IsAcceptableEncoding is right btw - it checks the prefs rather than what was actually sent..  http-on-modify-requst is an example of something that could change that header after the pref value is used to generate the request. And in the end, even if the server has sent an inapproriate C-E, all it does is prevent us from decoding some scenarios that we have working decoders available for - intentional breakage!
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
We should remove IsAcceptableEncoding completely.  In RFC there is nothing said about what UA should do when C-E is not in the A-E set.  We don't need to break.  I'm just not sure what we could potentially break (persisted cache entry, e.g.?) with this when applyConversion is not set on the channel.
I would r+ a patch to remove isacceptableencoding .. most of the time imo it just adds a bunch of redundant strcmps and it can occasionally do the wrong thing. (i.e. prevent decoding when we have a decoder available).
https://hg.mozilla.org/mozilla-central/rev/5326fc45fe46

Should this have a test?
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
FWIW - even with HTTP/1, a missing Accept-Encoding can be interpreted as "use whatever encoding you like."
You need to log in before you can comment on or make changes to this bug.