Bug 115353 – [Soup] Add support to disable "Accept-Encoding:" HTTP header on soup ResourceRequest

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebCore Misc.

[Andre Moreira Magalhaes]

If ResourceRequest::setAcceptEncoding(false) is called we make sure the "Accept-Encoding:" header is not added to the soup message, as this could lead to issues where for example the server sends compressed data and the Content-Length (ResourceResponse::expectedContentLength()) is the size of the compressed data, but the data received in ResourceHandle::didReceiveData() is uncompressed (done by SoupContentDecoder), so the data received has a different size from what is actually reported.
    
This is especially needed by the webkit gstreamer source element which needs to rely on the correct data size to download streams.
    
The default value of ResourceRequest::acceptEncoding() is true to keep backwards compatibility.

Patch to follow.

[Andre Moreira Magalhaes]

Created attachment 200010 [details]
Patch

[Dan Winship]

(In reply to comment #0)
> This is especially needed by the webkit gstreamer source element which needs to rely on the correct data size to download streams.

That seems improbable... what do you do if there's no Content-Length header at all?

I think the problem is just that the gstreamer source needs to know to not look at the declared Content-Length if the SOUP_MESSAGE_CONTENT_DECODED flag is set.

[Andre Moreira Magalhaes]

What happens here is that:
1 - If we don't set the size on gst appsrc (set to -1), it will break seek and consider the stream a live stream
2 - if we set the wrong size (in case the content-length has the compressed size opposed to the actual size), gst will consider EOF before the stream ends

We could probably indeed set the size to -1 by checking this flag you mentioned, but seek would be broken for all servers returning compressed data.(In reply to comment #2)
> (In reply to comment #0)
> > This is especially needed by the webkit gstreamer source element which needs to rely on the correct data size to download streams.
> 
> That seems improbable... what do you do if there's no Content-Length header at all?
> 
> I think the problem is just that the gstreamer source needs to know to not look at the declared Content-Length if the SOUP_MESSAGE_CONTENT_DECODED flag is set.

What happens here is that:
1 - If we don't set the size on gst appsrc (set to -1), it will break seek and consider the stream a live stream
2 - if we set the wrong size (in case the content-length has the compressed size opposed to the actual size), gst will consider EOF before the stream ends

We could probably indeed set the size to -1 by checking this flag you mentioned, but seek would be "broken" (disabled) for all servers returning compressed data.

[Andre Moreira Magalhaes]

See also https://bugs.webkit.org/show_bug.cgi?id=115354#c13

[Dan Winship]

(In reply to comment #3)
> We could probably indeed set the size to -1 by checking this flag you mentioned, but seek would be broken for all servers returning compressed data.

There's a bug open about adding an API to SoupMessage to extract the actual content length out of the gzip metadata when the stream is compressed. (https://bugzilla.gnome.org/show_bug.cgi?id=664620). So that would be another solution. But it's true, as Sebastian notes in the other bug, that multimedia streams ought to already contain compressed data and thus not be usefully compressible anyway...

[Sergio Villar Senin]

Comment on attachment 200010 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=200010&action=review

Overall I agree with Dan's comments, I think they're quite aligned with my concerns exposed in bug 115354. Likely we should use some libsoup API to provide such information but as we aren't bumping requirements soon I guess it's fine.

Anyway, based on the fact that multimedia streams are most of them already compressed data, it shouldn't hurt not to use compression at HTTP level.

> Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:61
> +        // Disable SoupContentDecoder as it adds a "Accept-Encoding:" field to the headers

I don't think this comment is needed

[Andre Moreira Magalhaes]

Created attachment 202285 [details]
Patch

(In reply to comment #6)
> > Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:61
> > +        // Disable SoupContentDecoder as it adds a "Accept-Encoding:" field to the headers
> 
> I don't think this comment is needed
Removed comment.

Also updated Changelog to use the correct format.

[Andre Moreira Magalhaes]

Thanks all for the review, all patches are updated now with comments applied.

[Philippe Normand]

Adding Gustavo and Martin in CC.

[Martin Robinson]

Comment on attachment 202285 [details]
Patch

It appears that nothing in this patch is actually calling setAcceptEncoding.

[Philippe Normand]

The patch in bug 115354 does :)

[Martin Robinson]

(In reply to comment #11)
> The patch in bug 115354 does :)

This should really be one change instead of pushing a patch that uses dead code. I'm also not sure about the name. Shouldn't it be something like setCompressionEnabled or similar? I'm also a bit wary of this because it's a new method on ResourceRequestBase, but only implemented for libsoup. Is this a problem for Qt?

[Andre Moreira Magalhaes]

(In reply to comment #12)
> (In reply to comment #11)
> > The patch in bug 115354 does :)
> 
> This should really be one change instead of pushing a patch that uses dead code. I'm also not sure about the name. Shouldn't it be something like setCompressionEnabled or similar? I'm also a bit wary of this because it's a new method on ResourceRequestBase, but only implemented for libsoup. Is this a problem for Qt?

As for the patch split, I am not used to the webkit commit scheme and I usually try to make patches per functionality. In this case one patch is to enable the support to disable AcceptEncoding field on requests (this bug) and the other is to actually disable it on the gstreamer player (bug #115354), hence the separate bugs.

The name chosen was because the header is called Accept-Encoding, but I have no hard preference here.

As for this being a problem with Qt, I don't think this is the case as the patch keeps the same default behaviour.

[Martin Robinson]

(In reply to comment #13)

> As for this being a problem with Qt, I don't think this is the case as the patch keeps the same default behaviour.

What I mean is that there is no implementation of this fix for the Qt networking layer, yet the Qt port uses libsoup. Does this problem affect Qt? If so we need to fix it as well or open a bug. If not, that may lead to the interesting question of "Why not?" and perhaps an alternate fix for this issue. Just some thoughts.

[Sebastian Dröge (slomo)]

(In reply to comment #14)
> (In reply to comment #13)
> 
> > As for this being a problem with Qt, I don't think this is the case as the patch keeps the same default behaviour.
> 
> What I mean is that there is no implementation of this fix for the Qt networking layer, yet the Qt port uses libsoup. Does this problem affect Qt? If so we need to fix it as well or open a bug. If not, that may lead to the interesting question of "Why not?" and perhaps an alternate fix for this issue. Just some thoughts.

It affects any port, it's just not *that* bad (with Andre's other patches) and will not cause any actual problems... other than causing more load on the server to compress something that is already compressed.

[Martin Robinson]

In any case, we should not add a property to a base class that's only used by a subclass and only called in another patch, thus making it dead code until some future time. :)

[Martin Robinson]

(In reply to comment #14)
 
> What I mean is that there is no implementation of this fix for the Qt networking layer, yet the Qt port uses libsoup. 

danw pointed out that "the Qt port uses libsoup" is wildly inaccurate. That's because I meant to write "the Qt port uses GStreamer." I'm just noting this for posterity. :)

[Philippe Normand]

(In reply to comment #16)
> In any case, we should not add a property to a base class that's only used by a subclass and only called in another patch, thus making it dead code until some future time. :)

Sounds fair enough :) Andre can you merge this patch and the one in bug 115354 ? If possible not on top of the one from bug 115352.

[Gustavo Noronha (kov)]

I'm working on this merge, I think it makes sense to handle it in the other bug, which talks about the actual change we want to make to behaviour, so closing this one as a dup.

*** This bug has been marked as a duplicate of bug 115354 ***