859706 - CSS file using |@charset "iso-10646-ucs-2"| doesn't just ignore that rule

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[leo.bugzilla.gecko]

CSS file using charset iso-10646-ucs-2 doesn't work properly in FFOS (nor Firefox Browser for Windows).

But it does work fine in Firefox Nightly.

For reference test, visit:
http://sqaap1.htc.com.tw/GCF/CSS_Content_V1.1_2007_05_15/wcss_syntax/atcharset_ucs2_le.xhtml

This is specially important for FFOS because some operators require it.

Comment 2

[Ben Francis [:benfrancis]]

Is this where this bug should live?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Um...  So this bug is fixed, no?

Is this a request to backport the fix to b2g18 or something?

Comment 4

[Ben Francis [:benfrancis]]

If it's fixed please feel free to mark it as fixed or dupe it, I didn't realise, not expecting anything special :)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Well, comment 0 says it works in nightly and it works for me in a nightly as well.  ;)

Reporter, what sort of action are you looking for here?

Comment 6

[Andre Graziani (:graziani)]

Hi Boris,

The problem persists in b2g.
Can you please backport the fix to the b2g18?
Thanks.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So for future reference, it's worth doing a bisection (on nightlies to start with) to figure out _what_ needs to be backported.

In this case the fix range is http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a761bfc192b5&tochange=58ebb638a7ea so presumably bug 796882 (which never got marked fixed, why?).

In any case, the change in that bug simply made the @charset rule be ignored in this case.  And then the testcase started working.  Without that it fails because "iso-10646-ucs-2" is not a valid charset name that we know anything about, so it used to be treated as a fatal error when we actually paid attention to the @charset rule.

So what content is this that's using completely bogus @charset rules and why do we care about it, exactly?  And does it actually need a backport of bug 796882 or a specific blacklisting or aliasing of this @charset value?

Henri, how safe would trying to backport the patch from bug 796882 to the gecko 18 branch be?  My gut instinct is "not safe", but maybe there was less churn in the relevant code from 18 to 19 than I think?

Comment 8

[Andre Graziani (:graziani)]

(In reply to Boris Zbarsky (:bz) from comment #7)
> So what content is this that's using completely bogus @charset rules and why
> do we care about it, exactly?
This test case is part of the GCF certification, required in Europe.

> And does it actually need a backport of bug
> 796882 or a specific blacklisting or aliasing of this @charset value?
So far only this specific charset presented problem. If the backport of bug 796882 is considered safe and applied, it would avoid us to face similar issues with other problematic charsets we didn't detect yet.

Comment 9

[Masatoshi Kimura [:emk]]

Bug 796882 depends on bug 801402, which caused a lot of regressions. I don't think it's practical to backport all of them.

Comment 10

[Masatoshi Kimura [:emk]]

(In reply to andre.graziani from comment #8)
> So far only this specific charset presented problem.

Then it would be reasonable to make this specific label an alias of "UTF-16".

Comment 11

[Boris Zbarsky [:bzbarsky]]

Masatoshi-san, are you willing to write up a patch for that?

Comment 12

[Simon Sapin (:SimonSapin)]

To clarify: would this new encoding label be a temporary work-around until a BOM takes precedence?

Comment 13

[Boris Zbarsky [:bzbarsky]]

This new encoding label would be a b2g-18 specific thing, not to be checked into any other branch, basically to make this broken "certification" testcase work.

Comment 14

[Henri Sivonen (:hsivonen)]

(In reply to leo.bugzilla.gecko from comment #0)
> This is specially important for FFOS because some operators require it.

This looks like a test suite for the sake of testing. This doesn't look like something that needs to be backported in order to avoid breaking the Web, so in terms of the risk of breaking something this late on a branch, it doesn't make sense to change the code on the branch, IMO. (The encoding label in question seems to be a WAP-era walled-garden oddity. On the Web, people who have the bad judgment to use UTF-16 at least don't tend to call it UCS-2, AFAIK.)

What happens if b2g18 does not pass this test case? Do we now make changes in order to pass operator test suites that aren't reflective of what's needed to render the real Web out there? That would be sad.

Comment 15

[Masatoshi Kimura [:emk]]

Hm, "iso-10646-ucs-2" was an alias of "UTF-16BE", not "UTF-16LE". The test is completely bogus. Now I'm reluctant to land the *fix* even if it is b2g-only.

Comment 16

[Boris Zbarsky [:bzbarsky]]

How about adding something in the CSS loader that just ignores the @charset in this case?  Would that be good enough?  After all, that's what trunk is doing....

Comment 17

[Henri Sivonen (:hsivonen)]

(In reply to Boris Zbarsky (:bz) from comment #7)
> Henri, how safe would trying to backport the patch from bug 796882 to the
> gecko 18 branch be?  My gut instinct is "not safe", but maybe there was less
> churn in the relevant code from 18 to 19 than I think?

Seems pretty safe, though a try run (https://tbpl.mozilla.org/?tree=Try&rev=2cb4223a9913) just started. The patch needs newer signatures for EncodingUtils::FindEncodingForLabel to be backported.

Comment 18

[Henri Sivonen (:hsivonen)]

Attachment: Part 1: Backport EncodingUtils::FindEncodingForLabel signatures

Comment 19

[Henri Sivonen (:hsivonen)]

Attachment: Part 2: The patch for bug bug 796882

Comment 20

[Boris Zbarsky [:bzbarsky]]

Henri, why is comment 9 not an issue?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 759076 [details] [diff] [review]
Part 1: Backport EncodingUtils::FindEncodingForLabel signatures

r=me

Comment 22

[Henri Sivonen (:hsivonen)]

(In reply to Boris Zbarsky (:bz) from comment #20)
> Henri, why is comment 9 not an issue?

Oh, right. Sorry about not addressing that.

The bug itself: The CSS-relevant part is addressed by the backport.
Bug 860180: XML, irrelevant to CSS
Bug 845743: affects Hong Kong-specific Trad. Chinese characters: likely not an issue for CSS or an issue for Firefox OS launch markets.
Bug 844461: HTML, irrelevant to CSS
Bug 814900: XML, irrelevant to CSS
Bug 813427: I don't understand the relevance.
Bug 847886: Firefox OS-irrelevant UI
Bug 847880: Unfixed on trunk; doesn't need to block.
Bug 846936: WONTFIX
Bug 816849: test suite for old spec is outdated when spec updated

Comment 23

[Henri Sivonen (:hsivonen)]

Attachment: Folded patch

(Folding patches to have a single one for approval.)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Gecko 18 implements CSS 2.1 on this point, but CSS 2.1 makes no sense on this point. The test that this bug was filed over is bogus, but passes when Gecko implements CSS3 on this point (like later versions of Gecko do).

User impact if declined: UTF-16 style sheets (very rare; probably even rarer in the Firefox OS launch markets) that have a bogus @charset rule are rejected. Plus whatever the impact of failing to pass a test on an operator-wanted test suite is.

Testing completed: Baked on trunk for multiple releases.

Risk to taking this patch (and alternatives if risky): The main risk is that the trunk has some change that interacts with this code in an important way but isn't included in the backport. The alternative to taking the patch is keeping the CSS 2.1 behavior for b2g18 and not passing the bogus test.

String or UUID changes made by this patch: None.

(I don't think backporting this was a good use of time, but here's a backport, since figuring out how risky backporting would be basically required doing the backporting. I'd like some guidance for how to deal with bugs arising of test suites of this nature in the future.)

Comment 24

[Henri Sivonen (:hsivonen)]

checkin-needed to b2g18.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/64d9a4a4ca85

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/64d9a4a4ca85