638318 - Page starting with lots of NUL chars is incorrectly sniffed as UTF-16BE (with HTML5 parser enabled)

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P1)

Description

[Gary [:streetwolf52]]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b13pre) Gecko/20110302 Firefox/4.0b13pre
Build Identifier: 20110302123843

With the HTML5 Parser enabled the page renders in what looks like Asian characters.  With the Parser disabled it renders correctly.

Reproducible: Always

Steps to Reproduce:
1.Go to http://www.rinconrojo.net/podium/index.php?showforum=4
2.
3.
Actual Results:  
All Asian characters appear.

Expected Results:  
A normal looking page.

Here's another page with the same problem: http://www.creators.com/featurepages/11_comics_speed-bump.html?name=bmp

Comment 1

[Leman Bennett [Omega]]

Something is causing the parser to trip the wrong encoding. Using the encoding menu, setting the option back to iso-8859-1 fixes the issue.

Comment 2

[xan K]

Thank you Dexter for filing the bug. It's been happening since quite some time.
All I thought was a problem with the page. Until today I received email feeds
from the reported forum and I opened it with IE but still couldn't render well
in FF4.

Comment 3

[Gary [:streetwolf52]]

(In reply to comment #1)
> Something is causing the parser to trip the wrong encoding. Using the encoding
> menu, setting the option back to iso-8859-1 fixes the issue.

It is set on iso-8859-1 with the Parser enabled.  Maybe I am not understanding your comment.

Comment 4

[Boris Zbarsky [:bzbarsky]]

The page starts with a bunch of null bytes (12280 of them, in fact) before any content.  So the <meta> tag is ignored, since it's too late in the document.

So yes, there is very much a problem with the page!

Comment 5

[Gary [:streetwolf52]]

Without the Parser enabled it look's like the encoding used is UTF-16BE.

With the Parser enabled it's using iso-8859-1.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Henri, SniffBOMlessUTF16BasicLatin looks like it'll treat all null bytes as leaving both elements of byteNonZero as false.  But then the code at the bottom of the function assumes that one of them must be true....

Comment 7

[Boris Zbarsky [:bzbarsky]]

> With the Parser enabled it's using iso-8859-1.

No.  Without the parser enabled (or with the bug fixed), this page will use the user's default encoding, which is locale-dependent.  So it'll still be broken for many users; it really needs to be fixed in its own right.

Comment 8

[Ed Morley [:emorley]]

Changing platform to x86; since WOW64 in user agent indicates 32bit build of Firefox (platform refers to browser build, not OS).

Comment 9

[Gary [:streetwolf52]]

(In reply to comment #8)
> Changing platform to x86; since WOW64 in user agent indicates 32bit build of
> Firefox (platform refers to browser build, not OS).

I missed this.  Thanks for correcting.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

I'm not quote sure how to write a test for this, since the detected charset will be locale-dependent...  Can I just assume that it'll be ISO-8859-1 anywhere where we run tests?

Comment 11

[mattburles]

when will this land so it doesnt become a hardblocker? (hopefully it doesnt become one)

Comment 12

[Boris Zbarsky [:bzbarsky]]

It needs review first.

Comment 13

[Henri Sivonen (:hsivonen)]

Comment on attachment 516491 [details] [diff] [review]
Fix

r=me.

Comment 14

[Henri Sivonen (:hsivonen)]

Hmm. Actually, won't this patch still sniff as UTF-16 if there's, say 500 zero bytes? A safer follow-up patch coming up.

Comment 15

[Henri Sivonen (:hsivonen)]

Comment on attachment 516491 [details] [diff] [review]
Fix

Clearing review. Will post another patch after testing it.

Comment 16

[Henri Sivonen (:hsivonen)]

Attachment: Safer fix

Mochitests coming up in another patch separately.

Comment 17

[Henri Sivonen (:hsivonen)]

(In reply to comment #14)
> Hmm. Actually, won't this patch still sniff as UTF-16 if there's, say 500 zero
> bytes? A safer follow-up patch coming up.

Rather, the problem I'm trying to fix by making this "safer" is where there are runs of zeros and non-zeros are all even or odd. It seems safer to trigger the UTF-16 stuff only if we the an alternation of zero and non-zero all the way.

Comment 18

[Henri Sivonen (:hsivonen)]

Attachment: Mochitest

The file in the binary patch part has a long run of zeros at the start but one non-zero byte among them within the first 1024 bytes. Then after the 1024-byte limit, there's ASCII text.

Comment 19

[Henri Sivonen (:hsivonen)]

About blockingness: I think this bug is worse from the Web compat point of view than bug 631751 (whose fix caused this regression). If this fix doesn't make it to Firefox 4.0, I think we should take the fix for 4.0.1 or, failing that, back out bug 631751 for 4.0.1.

Comment 20

[Henri Sivonen (:hsivonen)]

Attachment: Patch for backing out the change (bug 631751) that caused the regression

Here's a patch that reverses the fix for bug 631751 in case we want to do that instead.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 516517 [details] [diff] [review]
Safer fix

OK, yeah.  This would be the other way to deal.  r=me

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 516520 [details] [diff] [review]
Mochitest

r=me.

Please request approval for this?

For drivers: I think this is very safe, in the sense that it turns off some sniffing that we added recently in cases when there are lots of null bytes at the beginning of the file, because the sniffing is not reliable in that situation.

Comment 23

[Henri Sivonen (:hsivonen)]

Comment on attachment 516517 [details] [diff] [review]
Safer fix

Setting the approval request per bz's comment.

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 516517 [details] [diff] [review]
Safer fix

a=beltzner

Comment 25

[Mike Beltzner [:beltzner, not reading bugmail]]

Can someone land this nowrightnow?

Comment 26

[Henri Sivonen (:hsivonen)]

I'll land.

Comment 27

[Johnny Stenback (:jst)]

Landed:

http://hg.mozilla.org/mozilla-central/rev/ed9c526f93b5

Comment 28

[Henri Sivonen (:hsivonen)]

Test landed:
http://hg.mozilla.org/mozilla-central/rev/770e2709cff5

Comment 29

[jag (Peter Annema)]

Attachment: Test case, UTF 16 BE with leading zeros

Comment 30

[jag (Peter Annema)]

I think the first fix was better. Or maybe the second fix needs to distinguish between 0x0000 characters before and after having found a non-zero byte.

The attached test case renders differently in current trunk than in Fx 4.0 b12 and IE 9.

Comment 31

[Henri Sivonen (:hsivonen)]

(In reply to comment #29)
> Created attachment 516644 [details]
> Test case, UTF 16 BE with leading zeros

Has this case been seen in the wild?

Comment 32

[jag (Peter Annema)]

Not that I know of. New bug, target Fx 5?

Comment 33

[Henri Sivonen (:hsivonen)]

(In reply to comment #32)
> Not that I know of. New bug, target Fx 5?

Let's not go further down the slippery slope here.

Supporting imaginable bogosity is not a goal. The point is supporting the real-world bogosity seen in bug 631751 to the extent it works in both IE9 and Firefox 3.6--and only to that extent. Note that Chrome and Safari don't support that bogosity anyway.

Comment 34

[Boris Zbarsky [:bzbarsky]]

> The attached test case renders differently

That was rather the point of Henri's approach, yes.