New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 711810 link

Starred by 0 users

Issue metadata

Status: Untriaged
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Chrome network stack should not allow Accept-Encoding to be set by the consumer

Project Member Reported by rdsmith@chromium.org, Apr 14 2017

Issue description

Accept-Encoding is an HTTP header which specifies which content encodings the server may server content encoded with.  Content-encodings are processed and decoded in the network stack (specifically in URLRequestHttpJob & net/filter).  Unrecognized content-encodings are passed through the network stack without decoding (and this behavior is the same in Firefox, Safari, and Edge); however, if content encodings are chained and the stack only knows how to decode a subset of them, no reasonable behavior is possible.  We should disallow setting of acceptable content-encodings by consumers.

There are two cases in which this is done:
* Cronet has some embedders that set content-encodings and take responsibility for decoding the results.  They should be modified to not use HTTP heads and instead simply choose a body format that allows the same behavior.

* The media player uses Accept-Encoding to disallow any encoding except for the identity encoding.  This is a specific behavior that should be implemented by a load flag.
 
Cc: hubbe@chromium.org
After thinking about it some, I'm not clear why the media player disallows non-identity encodings.  They may not be useful in terms of compression for those formats, but why is that not a decision to be made on the server side? 

Fredrik, any insight on this one?

Comment 2 by hubbe@chromium.org, Apr 20 2017

Compressing media is futile, as those formats are already compressed. (Unless you re-encode the audio/video, but that's different.)
I don't know why this was added originally, but I suspect that some servers and proxies compress videos, degrade performance, and possibly breaks seeks without it.

Oh, that's right, you do seeks via range-request.  That makes sense.  Ok, back to my original "We should implement this with a load flag".  Thanks!

(I sorta feel as if we shouldn't get in the way of server's doing stupid things that bork performance when it's reasonable to make it a server decision.  Functionality's different.)


I think setting identity Accept-Encoding is a legitimate use case. This is recently brought up by a Cronet client ( Issue 767025 ). 

Instead of adding a load flag, can we simply allow "identity" AE (and its variants) in URLRequestHttpJob::AddExtraHeaders() 
https://cs.chromium.org/chromium/src/net/url_request/url_request_http_job.cc?rcl=ec42aaaf6a71aad5b553c3e222eaea62deda261b&l=522 ?
Helen, what's your argument that setting identity Accept-Encoding is a legitimate use case, rather than specifying it through a load flag?  To me it's a fairly clear layering violation.

Why would it be a layering violation? Sorry, I think I missed the context for why we need a load flag.

In Cronet on Android, we ignore Accept-Encoding headers set by consumers (https://cs.chromium.org/chromium/src/components/cronet/android/java/src/org/chromium/net/impl/UrlRequestBuilderImpl.java?rcl=dc6e43337fd8a150e009a281128dc0f262e12516&l=112). 

I am thinking that we can do the same lower in the stack. For example, we could make URLRequestHttpJob strip off any Accept-Encoding headers that are not "identity" or its variants like "identity;q=1".  Will that be a layering violation?
I tried to give it in the issue description?  IMO, any use of Accept-Encoding: by layers above the network stack is a layering violation.  The network stack adds AE lines based on what it is willing to decode, and validates them coming back in a similar way (modulo dodges we've had to put in to handle exactly this use case).  AE and CE are headers specific to HTTP and the communication with the server over the network, and shouldn't be messed with outside that context.   

One specific "gotcha" is that it's not clear what to do if an AE is specified as accepted by a consumer, but is in the middle of a chain the other elements of which the netstack has to decode.  Restricting consumers to be able to say "only use identity" doesn't have this problem, but doing that through setting of AE is a bug prone interface with a lot more complexity than a "LOAD_DISABLE_COMPRESSION" load flag.  It's hard to be confused by what a consumer means by that load flag :-}.



Comment 8 by hubbe@chromium.org, Sep 21 2017

As a client, it's easy to mistake the network layer for a caching http proxy[1], which confuses the issue of who should and should not set the AE headers.

[1] Unfortunately the broken HTTP spec does not allow proxies to do a lot of the things that the network layer does, so the network layer is not really a proxy.

Cc: pauljensen@chromium.org mmenke@chromium.org
Ah, okay. Thanks for the context. I see the argument for a load flag now.  I am fine with adding a load flag.  Matt had some reservation about adding new load flags. I am not sure if that applies here. Matt, could you comment?
I'd rather not add a load flag, because load flags are too much of a catch all (They currently make it all the way to the socket pool layers, which is bizarre, since the socket pools only care about one or two flags).  I'd rather turn LoadFlags into cache flags, so they have a clear source and destinations.  I'm fine with a bool, however.  I'm also fine with just having the consumer set a magic header, and if we don't like it, ignoring it (The consumer being Cronet, or the Cronet embedder).
A magic header, meaning something other than AE?  Or seeing an AE itself?  I'd really rather avoid that.

Also, where would such a bool go?  A setter on the URLRequest?

I meant setting A-E itself (Note that the cache does something like this).  And yea, the bool would be a setter on the URLRequest, and probably a field on  HttpRequestInfo.
Matt, could you point me to the code where the cache modifies A-E? 
Sorry, I wasn't clear - the cache looks for, and respects, certain magic cache headers (Not sure what it does if it doesn't like those headers, though)
So from my perspective, a magic header that isn't in the HTTP spec is a reasonable way for consumers to communicate with the //net implementation.  A header that is modified by //net with details and trickiness from the HTTP spec is not.  

I'd be fine with a bool.  I'm not sure this belongs in a "cache flags" category--it's more, to me, about a performance optimization for transfer on the wire (though that would suggest just a hint) and making it possible for consumers to do reasonable things with range requests (i.e. about the relationship between the value specified in a range request and the location in the underlying resource).  Neither one of those things scream "caching" to me.


Sign in to add a comment