Bug 115353 - [Soup] Add support to disable "Accept-Encoding:" HTTP header on soup ResourceRequest
Summary: [Soup] Add support to disable "Accept-Encoding:" HTTP header on soup Resource...
Status: RESOLVED DUPLICATE of bug 115354
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 115354
  Show dependency treegraph
 
Reported: 2013-04-29 07:49 PDT by Andre Moreira Magalhaes
Modified: 2013-08-07 09:28 PDT (History)
7 users (show)

See Also:


Attachments
Patch (5.59 KB, patch)
2013-04-29 07:50 PDT, Andre Moreira Magalhaes
no flags Details | Formatted Diff | Diff
Patch (5.38 KB, patch)
2013-05-20 10:03 PDT, Andre Moreira Magalhaes
mrobinson: review-
andrunko: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andre Moreira Magalhaes 2013-04-29 07:49:36 PDT
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.
Comment 1 Andre Moreira Magalhaes 2013-04-29 07:50:41 PDT
Created attachment 200010 [details]
Patch
Comment 2 Dan Winship 2013-04-29 09:51:07 PDT
(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.
Comment 3 Andre Moreira Magalhaes 2013-04-29 10:25:44 PDT
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.
Comment 4 Andre Moreira Magalhaes 2013-04-29 10:28:35 PDT
See also https://bugs.webkit.org/show_bug.cgi?id=115354#c13
Comment 5 Dan Winship 2013-04-29 11:23:47 PDT
(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...
Comment 6 Sergio Villar Senin 2013-05-13 03:44:12 PDT
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
Comment 7 Andre Moreira Magalhaes 2013-05-20 10:03:39 PDT
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.
Comment 8 Andre Moreira Magalhaes 2013-05-20 10:31:12 PDT
Thanks all for the review, all patches are updated now with comments applied.
Comment 9 Philippe Normand 2013-06-03 03:17:54 PDT
Adding Gustavo and Martin in CC.
Comment 10 Martin Robinson 2013-06-03 04:23:58 PDT
Comment on attachment 202285 [details]
Patch

It appears that nothing in this patch is actually calling setAcceptEncoding.
Comment 11 Philippe Normand 2013-06-03 04:34:08 PDT
The patch in bug 115354 does :)
Comment 12 Martin Robinson 2013-06-03 04:51:32 PDT
(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?
Comment 13 Andre Moreira Magalhaes 2013-06-04 09:50:24 PDT
(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.
Comment 14 Martin Robinson 2013-06-04 14:01:30 PDT
(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.
Comment 15 Sebastian Dröge (slomo) 2013-06-06 07:26:27 PDT
(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.
Comment 16 Martin Robinson 2013-06-06 07:29:07 PDT
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. :)
Comment 17 Martin Robinson 2013-06-06 07:46:16 PDT
(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. :)
Comment 18 Philippe Normand 2013-06-24 03:27:49 PDT
(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.
Comment 19 Gustavo Noronha (kov) 2013-08-07 09:28:24 PDT
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 ***