1410146 - Missing HTTP content-type header in remote request causes Firefox to not render HTML on a website

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

[Chris More [:cmore]]

This testing was done on Firefox 57 beta on OSX:

The following URLs display the HTML directly and do not render into the specific page being accessed:

https://www.penfed.org/contact-us/
https://www.penfed.org/credit-cards/
https://www.penfed.org/personal/student-loan-refinance/

If you remove the trailing slash on the URL, all pages then render the HTML:

https://www.penfed.org/contact-us
https://www.penfed.org/credit-cards
https://www.penfed.org/personal/student-loan-refinance

On penfed.org, some HTML links point to a mix of trailing and non-trailing slash pages. Thus, if you are trying to use their website one page may render and the next may not. If you wanted to continue using penfed.org and didn't know about the trailing slash issue, you may have to switch to another browser to continue.

I talked to Mike Kaply and he thinks this is a regression in Firefox and not a webcompat issue.

Comment 1

[Mike Kaply [:mkaply] (PTO until Sept 14)]

I did a mozregression bisection and it pointed to:

https://bugzilla.mozilla.org/show_bug.cgi?id=1373536

which doesn't seem to make sense.

Larger set was:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=768a1255610d1225ce412533da4379f664fe7c9c&tochange=842a147e06b82c77e5523b80b70c84fc221e9e47

But it definitely used to work, regardless of the slash,

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

[Tracking Requested - why for this release]: an important issue that might affect more than this website.

Marking the dependency to bug 1373536 until we hear otherwise. The content type we get is text/plain according to the network panel in devtools. Mike, does a build with those patches reverted work?

Comment 3

[Andreas Farre [:farre]]

I also did a mozregression and I got the pushlog https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=eafa5fdcb76713b716f4ada6dc96fb83b92b3a57&tochange=be145d9c3ff461644e2a082e2bf2b884e1037533

The last commit in that set looks more like a likely culprit, and after building b4878203823c I can verify that it does in fact introduce the issue.

Comment 4

[Mike Kaply [:mkaply] (PTO until Sept 14)]

Thanks Andreas. I hadn't had a chance to do that yet. I'm surprised this isn't being hit more on the general web.

Comment 5

[Marcia Knous [:marcia]]

Tracking 58+ based on Comment 2. Do we have any idea how widespread this could be (I would be worried about 57).

Comment 6

[Mike Kaply [:mkaply] (PTO until Sept 14)]

I think the key to knowing that is understanding why this site is serving the source in this case. That would tell us if this is serious or just some weird server config.

Comparing the Curl outputs between the two, the difference is clear:

23723:Downloads michaelkaply$ curl -I https://www.penfed.org/personal/student-loan-refinance/
HTTP/1.1 200 OK
------> Accept-Ranges: bytes <------
Cache-Control: max-age=0
Cache-control: no-cache="set-cookie"
------> Content-Length: 137996 <------
Date: Mon, 23 Oct 2017 19:37:08 GMT
------> ETag: "1805b0"
Server: Apache <------
Set-Cookie: AWSELB=6721935104A1A175E60E7E22637077EDA89EE388DDFA741520195BC3F0435271297232E068F8B31E25A78F7E969E342BF38534F5244C68C7D7265D862CA8DA92A46E961536;PATH=/;MAX-AGE=900
Vary: Accept-Encoding,Origin,User-Agent
X-Frame-Options: SAMEORIGIN
Connection: keep-alive

23723:Downloads michaelkaply$ curl -I https://www.penfed.org/personal/student-loan-refinance
HTTP/1.1 200 OK
Cache-Control: max-age=0
Cache-control: no-cache="set-cookie"
------> Content-Type: text/html; charset=UTF-8 <------
Date: Mon, 23 Oct 2017 19:37:14 GMT
Server: Apache
Set-Cookie: AWSELB=6721935104A1A175E60E7E22637077EDA89EE388DDFA741520195BC3F0435271297232E068F8B31E25A78F7E969E342BF38534F5244C68C7D7265D862CA8DA92A46E961536;PATH=/;MAX-AGE=900
Vary: Origin,User-Agent
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
Connection: keep-alive

I just don't know why those differences are causing us to display HTML source and if it is widespread.

Comment 7

[Dragana Damjanovic [:dragana]]

Attachment: bug_1410146.patch

Comment 8

[Mike Kaply [:mkaply] (PTO until Sept 14)]

Can you please provide a little more detail on what broke and if we need a test to cover this case?

Comment 9

[Chris More [:cmore]]

Is this just an Apache config issue that Firefox is not handing correctly when the content-type encoding header is not being sent?

Comment 10

[Dragana Damjanovic [:dragana]]

(In reply to Mike Kaply [:mkaply] from comment #8)
> Can you please provide a little more detail on what broke and if we need a
> test to cover this case?

It is a small bug I made while transforming code into a thread safe code (this is more for tsan because we guarantee that we do not run code on different threads at the same time.)

Explanation:

if we do not have content-type (https://www.penfed.org/personal/student-loan-refinance/ does not return content-type header) we use nsUnknownDecoder to try to figure out what data it is. So we look at the first up to 512 bytes of data.

Sometimes content is gzipped so nsUnknownDecoder needs to unzip it to be able to sniff it. We only unzip up to 512 bytes of data.

512 bytes is very little that fits into a packet, except if server send a bit of data make a small pause and send the rest. Maybe data does not fit into first packet(if cookies are huge) but usually server would send the rest immediately so we will not see this problem as well... 

The problem I introduce is when content is gzipped and we do not received all 512 before we start onDataAvailable (this is very rare otherwise we would have this problem more often, it is a timing issue). So if we do not get all 512 bytes, my bug starts overwriting the first data instead of appending :(

This is very timing problem and I am not sure I can make a test easy, because I will need to hack our test server to add delays, the localhost is too fast to reproduce this.

I do not think that we need a test for this.

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Dragana Damjanovic [:dragana] from comment #10)

Thanks for this detailed description.

> This is very timing problem and I am not sure I can make a test easy,
> because I will need to hack our test server to add delays, the localhost is
> too fast to reproduce this.

You can do it with our httpd.js easily (async processing of the response) but I don't require a test for r+

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 8921641 [details] [diff] [review]
bug_1410146.patch

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

sorry I didn't catch this at r :((

please don't forget to uplift to beta

Comment 13

[Chris More [:cmore]]

Thanks for the patch and it make sense and hard to know how many web servers out there are incorrectly set to not include a content-type in the header and have a big enough delay to create this bug.

Comment 14

[Dragana Damjanovic [:dragana]]

Comment on attachment 8921641 [details] [diff] [review]
bug_1410146.patch

Approval Request Comment
[Feature/Bug causing the regression]: bug bug 1373536
[User impact if declined]: In cases when a server does not send content-type header and content is compressed it can happened that we do not sniff data correctly to detect type. (there are additional condition that need to be met that this happens, they are timing related.)
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]:manualy verified and it will be in nightly in a day or two.
[Needs manual test from QE? If yes, steps to reproduce]: The uri is https://www.penfed.org/contact-us/
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: it is a very small patch, that sets one variable instead of leaving it as an EmptyCString.
[String changes made/needed]:none

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e55fa4da0cb
Fix UnknownDecoder. r=mayhemer

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

I'd like to track this as a blocking issue for 57. I am glad this was filed and fixed promptly.

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Liz, fyi, perhaps this is late for 56.0.2 release but this one seems like a dot release driver if the impact was very widespread.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Is there any reason that can't/shouldn't have an automated test?

Comment 19

[Liz Henry (:lizzard)]

I don't think we have any evidence it is a widespread problem - if it were, we'd have lots of duplicate reports.

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8921641 [details] [diff] [review]
bug_1410146.patch

This is a pretty severe issue, glad it's fixed, Beta57+

Comment 21

[Chris More [:cmore]]

Given that this issue is not specific to penfed.org, should we update the summary of this bug be more general? Like "Missing HTTP content-type header in remote request causes Firefox to not render HTML on a website (UnknownDecoder)".

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8e55fa4da0cb

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5f5a87b79586

Comment 24

[Dragana Damjanovic [:dragana]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #18)
> Is there any reason that can't/shouldn't have an automated test?

This was just a small error during converting code. This code is really not changing that often. We could make a test in a separate bug but it is not really critical.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

I think we should get that bug filed and prioritized. Regressions that break the web show a gap in test coverage that we should aim to fix ASAP.