Closed Bug 859706 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
1.1 QE3 (26jun)
Tracking Status
firefox24 --- unaffected
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: leo.bugzilla.gecko, Assigned: hsivonen)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Is this where this bug should live?
Component: Gaia::Browser → CSS Parsing and Computation
Product: Boot2Gecko → Core
Um...  So this bug is fixed, no?

Is this a request to backport the fix to b2g18 or something?
If it's fixed please feel free to mark it as fixed or dupe it, I didn't realise, not expecting anything special :)
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?
Flags: needinfo?(leo.bugzilla.gecko)
Hi Boris,

The problem persists in b2g.
Can you please backport the fix to the b2g18?
Thanks.
Flags: needinfo?(leo.bugzilla.gecko)
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?
Depends on: 796882
Flags: needinfo?(leo.bugzilla.gecko)
Flags: needinfo?(hsivonen)
Flags: needinfo?(andre.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.
Flags: needinfo?(leo.bugzilla.gecko)
Flags: needinfo?(andre.graziani)
Summary: CSS file using charset iso-10646-ucs-2 doesn't work properly → CSS file using |@charset "iso-10646-ucs-2"| doesn't just ignore that rule
Bug 796882 depends on bug 801402, which caused a lot of regressions. I don't think it's practical to backport all of them.
(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".
Masatoshi-san, are you willing to write up a patch for that?
Flags: needinfo?(VYV03354)
To clarify: would this new encoding label be a temporary work-around until a BOM takes precedence?
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.
(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.
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.
Flags: needinfo?(VYV03354)
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....
(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.
Flags: needinfo?(hsivonen)
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Attachment #759076 - Flags: review?(bzbarsky)
Attachment #759077 - Flags: review+
Henri, why is comment 9 not an issue?
Flags: needinfo?(hsivonen)
Comment on attachment 759076 [details] [diff] [review]
Part 1: Backport EncodingUtils::FindEncodingForLabel signatures

r=me
Attachment #759076 - Flags: review?(bzbarsky) → review+
(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
Flags: needinfo?(hsivonen)
Attached patch Folded patchSplinter Review
(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.)
Attachment #759076 - Attachment is obsolete: true
Attachment #759077 - Attachment is obsolete: true
Attachment #759714 - Flags: review+
Attachment #759714 - Flags: approval-mozilla-b2g18?
Attachment #759714 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
checkin-needed to b2g18.
Whiteboard: checkin-needed
https://hg.mozilla.org/releases/mozilla-b2g18/rev/64d9a4a4ca85
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: checkin-needed
Target Milestone: --- → 1.1 QE3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: