1043256 - Invalid encoding error reported when being redirected to traffic.outbrain.com

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[:gerard-majax]

[Blocking Requested - why for this release]: This is breaking user access to content.

STR:
 0. Load a website that is using traffic.outbrain.com
 1. Click on a link that gets handled by this provider

Expected:
 Link is loaded properly

Actual:
 An error page stating encoding issue is shown

A 100% reproductibility is for example on http://m.leparisien.fr/. Open an article, click on links at bottom of the page, it gets redirected through paid.outbrain.com

Comment 1

[:gerard-majax]

Of course, no logcat output helps.

Comment 2

[Jason Smith [:jsmith]]

QA Wanted for branch checks.

Comment 3

[:gerard-majax]

I have never noticed how much this is used by at least french news websites. Ouest France is making use of this, and is hit by the bug.

Comment 4

[:gerard-majax]

Attachment: bug1043256-http.log.child-1.bz2

Comment 5

[:gerard-majax]

Attachment: bug1043256-http.log.child-1.bz2

Comment 6

[:gerard-majax]

Attachment: bug1043256-http.log.bz2

Comment 7

[:gerard-majax]

This is only happening on B2G, and on multiple devices.

Comment 8

[Dragana Damjanovic [:dragana]]

I am not sure can this be the same problem as Bug 1012917?

Comment 9

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

This issue does occur on the latest 2.0 Flame, 2.1 Flame, and 2.1 Buri builds.  An encoding error is recieved. 

Environmental Variables:
Device: Flame Master
BuildID: 20140730124313
Gaia: b67ddd7d40b52e65199478b8d6631c2c28fdf41d
Gecko: b3cbce8a2b87
Version: 34.0a1 (Master) 
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0


Environmental Variables:
Device: Flame 2.0
BuildID: 20140730080807
Gaia: e8425788e0372fbbcc40387b799f0636c041fc14
Gecko: 20c472fc0ee3
Version: 32.0 (2.0) 
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Environmental Variables:
Device: Buri Master
BuildID: 20140730045709
Gaia: 25e998814ba89f30fe44cd2fdfbb44d160a04641
Gecko: 08c23f12a43e
Version: 34.0a1 (Master) 
Firmware Version: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

An error that the page uses an "invalid or unsupported form of compression." is seen when reproducing this issue on the latest 1.4 Flame build.  Listed separately in case the difference means more than just a wording change in more recent builds.

Environmental Variables:
Device: Flame 1.4
BuildID: 20140730110705
Gaia: 142953262d2cb031e3db217206edc3507580b0df
Gecko: 3c780088aae4
Version: 30.0 (1.4) 
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0

Comment 10

[Joshua Mitchell (Inactive)]

not a regression

Comment 11

[Jason Duell]

Hmm, the HTTP logs are huge, so I'm not positive I'm looking at the right HTTP transactions.  But all of the requests to paid.outbrain.com URIs seem to not do redirects: instead they are replying with 200 (normal response) chunked gzipped replies, which seem to happen normally (OnDataAvailable called with 184 bytes of data, OnStopRequest is called with NS_OK)

I do see 

  converter removed 'gzip' content-encoding

in both the parent and child log--is it possible that we're trying unzip twice somehow for a chunked gzipped reply?  I'm not sure how common those are.  But I'm seeing the ChannelChild call OnDataAvailable with 184 bytes and OnStopRequest with NS_OK, too, so that seems fine.

Basically I'm not sure the log is capturing the error. The only channels with an error status in the log are error=804b0002 (NS_BINDING_ABORTED), which doesn't sound like it would cause the encoding error you mention.  Could you try again, and/or give me the exact URI that you click?

I don't think this is bug 1012917 because that requres diverting the channel (which we only do for downloads or external content handlers), and this is allegedly a redirect.

Comment 12

[Dragana Damjanovic [:dragana]]

I looked at it.

we get data, decompressed them on parent then send them to child that tries to decompress them again. 
so we need to suspend decompression on parent (or child). The proper way to do it should be solved (should be found...) in bug 1012917 (something similar we need to do for FTP over http (the bug that i was working on), there you mention that we should wait for bug 1012917 to find the way to do it properly?)

Comment 13

[:gerard-majax]

Thanks :)

Comment 14

[Jason Duell]

Dragana,

Bug 1012917 covers more stuff than this, and doesn't need to be fixed for B2G right now.  Can you make a fix that works just for this case (I'm guessing it's specific for chunked compressed replies over e10s), and we'll fix divert after that?  (It may be the code that you already suggested in a previous bug, which I then told you to wait for bug 1012917 to add :)  We want a quick, simple fix here if possible.

Comment 15

[:gerard-majax]

Jason, can we check whether it is a problem for 1.3 ? This may be important for France market.

Comment 16

[Jason Smith [:jsmith]]

Sure. qawanted to check 1.3.

Comment 18

[Dragana Damjanovic [:dragana]]

Attachment: bug-1043256-fix1.patch

Comment 19

[Dragana Damjanovic [:dragana]]

Attachment: fix v1

If we apply converters on parent,  we need to clear content-encoding field from the header.
If a conversion could not be applied on parent, it will not be appied on child too? Because i am clearing the whole content-encoding field even if they are not applied because of of this check "if (gHttpHandler->IsAcceptableEncoding(val)) "?

Or should i just delete applied converters?

Comment 20

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

This issue does occur on Buri 1.3.  An error page occurs when visiting traffic.outbrain.com.
Environmental Variables:
Device: Buri 1.3
BuildID: 20140721151227
Gaia: 23f55be856cef53c6604a6fe4aeb09061afbc897
Gecko: 9727017eabb9
Version: 28.0 (1.3) 
Firmware Version: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:28.0) Gecko/28.0 Firefox/28.0

Comment 21

[:gerard-majax]

This may impact ZTE for France market, per comment 20 :(

Comment 22

[Jason Duell]

Comment on attachment 8466958 [details] [diff] [review]
fix v1

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

Discussed w/Dragana.

We don't want to clear the HTTP header--that seems likely to break some addon or something eventually down the road.  Better to figure out why we're trying to decode twice (only for chunked data?  I suspect so) and do something internally to stop that from happening (IPDL flag to SendOnStartRequest, etc).  I suspect that this is just happening for chunked replies (we would have gotten a lot of bug reports sooner otherwise), so we probably forgot to handle that special case. Also we need tests :)

Comment 23

[Dragana Damjanovic [:dragana]]

Attachment: fix 2

It was not chunked data but data without content-type.
In case of data without content-type, the real listener is started later (onDataAvailable or OnStopRequest) but content-ending will be loaded.
That is why content sniffing worked for compressed data too.

I move loading of content-encoders into content sniffer(if it is used) so that it is always after OnStartRequest. And i changed content sniffer to be able to detect content type even if data is encoded.

Comment 24

[:gerard-majax]

I just tested this patch on my Nexus S, and it does the job :)

Comment 25

[Dragana Damjanovic [:dragana]]

thank you for testing.

Comment 26

[:gerard-majax]

Dragana, do you think this patch could be applied to v1.3 ? Partner may be interested in fixing this, since it's affecting v1.3 recently launched in France and that there seems to be quite a couple of users of this in french website.

Comment 27

[--]

Comment on attachment 8468351 [details] [diff] [review]
fix 2

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

(jduell asked me to take a look at this patch)

I'm review+ on this with just one minor little nit. Looks clean and clear and it provides a test case!

::: netwerk/streamconv/converters/nsUnknownDecoder.cpp
@@ +371,5 @@
>  {
>    NS_ASSERTION(mContentType.IsEmpty(), "Content type is already known.");
>    if (!mContentType.IsEmpty()) return;
>  
> +  const char* testData = (const char*)mBuffer;

This should be converted to a C++ish cast.

Comment 28

[Dragana Damjanovic [:dragana]]

Attachment: fix v2

Thanks Daniel for the review. this is with the fix.

Comment 29

[Dragana Damjanovic [:dragana]]

Attachment: fix for v 1.3

Here is a bit changed patch that complies with v1.3.
Can somebody test.

I have removed the test from this patch because it is based on another test that is not in v1.3. I can add it if we need it.

Comment 30

[Dragana Damjanovic [:dragana]]

Attachment: fix for v1.3

just a small fix.

Comment 31

[Dragana Damjanovic [:dragana]]

Attachment: fix v2

Comment 32

[Dragana Damjanovic [:dragana]]

Attachment: fix v2

just small changes

Comment 33

[Dragana Damjanovic [:dragana]]

Attachment: fix for v 1.3

this is patch with small changes so that it is compiling with v 1.3.

Comment 34

[Dragana Damjanovic [:dragana]]

https://tbpl.mozilla.org/?tree=Try&rev=4d74dae43246

Comment 35

[:gerard-majax]

(In reply to Dragana Damjanovic from comment #33)
> Created attachment 8471724 [details] [diff] [review]
> fix for v 1.3
> 
> this is patch with small changes so that it is compiling with v 1.3.

Thanks, I'm rebuilding a v1.3 on my Flame to test this!

Comment 36

[:gerard-majax]

Comment on attachment 8471724 [details] [diff] [review]
fix for v 1.3

That also fixes the issue on my Flame after applying it to v1.3 :)

Comment 37

[--]

Comment on attachment 8471719 [details] [diff] [review]
fix v2

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +928,5 @@
> +    if (!unknownDecoderStarted) {
> +      nsCOMPtr<nsIStreamListener> listener;
> +      nsISupports *ctxt = mListenerContext;
> +      rv = DoApplyContentConversions(mListener, getter_AddRefs(listener), ctxt);
> +      if (NS_FAILED(rv)) return rv;

Maybe you can fix this line while you're at it? Add braces + newline I mean.

Comment 38

[Dragana Damjanovic [:dragana]]

Attachment: fix 3

Thanks Daniel.

Comment 40

[Dragana Damjanovic [:dragana]]

https://tbpl.mozilla.org/?tree=Try&rev=a92a1c47a710

Comment 41

[Ryan VanderMeulen [:RyanVM]]

I don't see Daniel listed as a Necko peer. Is he?
https://wiki.mozilla.org/Modules/Core#Necko

Comment 42

[Jason Duell]

Ryan--thanks for noticing!  :)

Necko policy is that any necko peer can delegate a review to someone.  I'm fine with Daniel having done the review here.

But thanks so much for doing your best to keep random elements from landing code in our part of the tree :)  I really do appreciate it.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a474d70179a

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4a474d70179a