650653 - test_ident_escaping.html and test_parse_ident.html fail if Firefox is built on DBCS locales

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Masatoshi Kimura [:emk]]

Microsoft C++ compiler doesn't support UTF-8 without BOM.
So nsCssScanner.cpp is treated as Shift_JIS source on Japanese locale.
Shift_JIS converter will eat second byte regardless the second byte is valid or not.
So lines 117, 119, 121 of nsCssScanner.cpp will be "commented out" because line terminators are eaten by last charcters in the previous lines.
https://hg.mozilla.org/mozilla-central/annotate/af907a37dcf0/layout/style/nsCSSScanner.cpp#l116
For example, 'ß' is encoded 'C3 9F' in UTF-8. '9F' is a valid first byte of Shift_JIS. so it eats next byte (0A). Therefore the next line is treated as if it is a part of the previous comment line.
This causes test_ident_escaping.html and test_parse_ident.html test failure.

I suppose this will also occur on Other DBCS locales.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: patch

Add dummy characters to prevent line terminators from being eaten.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 526606 [details] [diff] [review]
patch

I'd prefer if you remove the non-ASCII literals entirely, perhaps by replacing them with numbers like this:

// `   a   b   c   d   e   f   g   h   i   j   k   l   m   n   o
   0,  XSI,XSI,XSI,XSI,XSI,XSI,SI, SI, SI, SI, SI, SI, SI, SI, SI,
// p   q   r   s   t   u   v   w   x   y   z   {   |   }   ~
   SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, 0,  0,  0,  0,  0,
// U+008*
   0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,
// U+009*
   0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,
// U+00A*
   SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI,
// U+00B*
   SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI,
// U+00C*
   SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI,
// U+00D*
   SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI,
// U+00E*
   SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI,
// U+00F*
   SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI, SI,

But r=dbaron either way.  Thanks for figuring this one out.  (Thankfully, I don't think this should affect our official builds -- only builds that developers build on Japanese (etc.) systems.)

Comment 3

[Masatoshi Kimura [:emk]]

Attachment: patch

Added comments and static asserts

Comment 4

[Masatoshi Kimura [:emk]]

Sorry, mid-air collision occured. I'll fix a patch in accordance with review comment.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The PR_STATIC_ASSERT (and the removal of the explicit length) is a good idea -- keep that.

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: patch for check in

Comment 7

[Masatoshi Kimura [:emk]]

(In reply to comment #2)
> (Thankfully, I
> don't think this should affect our official builds -- only builds that
> developers build on Japanese (etc.) systems.)
Right. It's the reason this bug was not caught by official automated test.

Comment 8

[Mounir Lamouri (:mounir)]

Pushed in cedar:
http://hg.mozilla.org/projects/cedar/rev/aada30c4e0d3

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #7)
> (In reply to comment #2)
> > (Thankfully, I
> > don't think this should affect our official builds -- only builds that
> > developers build on Japanese (etc.) systems.)
> Right. It's the reason this bug was not caught by official automated test.

Well, locale-specific bugs that depend on the locale the user is *running* the build on (such as bug 646882 or bug 416581 comment 6) also aren't caught by our tests.

Comment 10

[Mounir Lamouri (:mounir)]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/aada30c4e0d3

Comment 11

[Zack Weinberg (:zwol)]

It doesn't really matter here, but it would be nice to not have to worry about putting non-ASCII characters in comments.  Is there a way we could force MSVC++ to treat all our C++ source files as UTF-8 regardless of the OS locale in use?  (From inside the build system, presumably.)

Comment 12

[Masatoshi Kimura [:emk]]

The only way is inserting a BOM which will make non-MS compilers unhappy :(
http://connect.microsoft.com/VisualStudio/feedback/details/341454/