451426 - Incorrect rendering of some Japanese characters when using downloadable fonts (@font-face)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Akira Yoshihara]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a2pre) Gecko/20080818032051 Minefield/3.1a2pre
Build Identifier: 

Some Japanese characters were rendered incorrectly when using downloadble fonts. 

This is how downloadable fonts work. At first, when the fonts are not downloaded, text will be rendered using default font. But once the download finishes, they will be rendered again using the downloaded font. The problem is that on this second rendering, some Japanese characters are rendered incorrectly, when downloaded fonts does not contain Japanese characters within it. Japanese characters should be rendered using default Japanese fonts (since the downloaded font does not contain Japanese characters), but it seems that they are using Chinese fonts instead of using Japanese fonts.
It would be easier to tell the difference if you compare this text with the same text without applying the downloadable fonts.

This only happens when you applied the patch v.0.8b for bug 441473 to the build tree.

Reproducible: Always

Steps to Reproduce:
1. Apply patch v.0.8b from bug 441473 and build
2. Make an HTML file in Japanese encoding (Shift_JIS, EUC-jp)
3. Write a text that contains Japanese characters in between Chinese characters and apply downloadable font to the text (make sure the font you download does not contain Japanese characters)
4. Open the HTML file from browser, and when the download is done, Japanese characters would be rendered incorrectly
Actual Results:  
Japanese characters are rendered incorrectly, probably using Chinese fonts.

Expected Results:  
Japanese characters should be rendered using Japanese fonts.

Comment 1

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

I'm not sure whether we generally want bugs against stuff you get when applying patches in other bugs, but for something this big it may be useful.

Comment 2

[cmtalbert]

(In reply to comment #1)
> I'm not sure whether we generally want bugs against stuff you get when applying
> patches in other bugs, but for something this big it may be useful.
> 
Akira and I hemmed and hawed over just that issue until J. Daggett asked us to write it up: https://bugzilla.mozilla.org/show_bug.cgi?id=441473#c46

Comment 3

[John Daggett (:jtd)]

Yeah, normally I wouldn't ask that these be separated but the patch is fairly large and I didn't want these issues to get lost in the noise.

Comment 4

[John Daggett (:jtd)]

Akira, could you attach a testcase for this?

Comment 5

[Akira Yoshihara]

Attachment: test case - incorrect rendering of some Japanese characters

Last 8 characters of each sentence are Chinese characters. Among them, 3rd, 7th and 8th character are rendered thinner compared to other characters. It would be more obvious if you can compare the same sentence using .test { font-family: "Comic Sans MS"; } instead of using downloadable fonts.

Also, once you loaded this page, please focus on some other window. If you focus again of firefox window after about 1 minute, Japanese characters (フォントの拡張子って) in the middle of the sentence will suddenly be rendered in a different way. This only happens to the first sentence which is using downloadable font.

Comment 6

[John Daggett (:jtd)]

Akira, not sure I see the problem here.  If this problem still occurs, could you post a screenshot, along with the exact set of steps you went through to get to that state?  

I see one line rendered in Comic Sans, one line rendered in the default font.  In both cases fallback occurs for Japanese, Chinese and Korean characters.  Given the defined styles, this appears correct.

Comment 7

[Akira Yoshihara]

(In reply to comment #6)
I cannot reproduce the bug right now. That is because I've finished my internship and I don't have a Mac OS build now (I forgot to mention, but this problem only occurs on Mac OS). I'm sorry that I cannot attach the screenshot.

If you could compare the two following test pages using reftest, the bug should reproduce. I'm sure it reproduced on patch v.0.9h. I hope you can see the difference.

Test case for @font-face rules v.0.6 (bug 452780)
https://bugzilla.mozilla.org/attachment.cgi?id=338203

In this attached test suite, you can find the following test cases.

downloadable-font-japanese.html - using downloadable font (since it's using url:local(), it uses the system font as a consequence)
downloadable-font-japanese-ref.html - using system font (ComicSansMS)

> I see one line rendered in Comic Sans, one line rendered in the default font. 
> In both cases fallback occurs for Japanese, Chinese and Korean characters. 
> Given the defined styles, this appears correct.
Yes, the fallback should occur. The point is at the last 8 characters (ABCDEFGH) in the sentence using Comic Sans. The 3rd, 7th and 8th characters should be rendered using Japanese fonts, since the encoding is specified as Shift_JIS. But when you use the @font-face rules, they are rendered using Chinese fonts. When you use the normal .test { font-family: "Comic Sans MS"; } rules, like in downloadable-font-japanese-ref.html, these characters are rendered using Japanese fonts.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: screenshot showing the incorrect rendering

Screenshot showing the incorrect rendering of Japanese characters, created with current Minefield build on Mac OS X (10.5.5). The first line uses @font-face to access Bitstream Vera Sans; the 3rd, 7th and 8th Japanese characters are rendered from a different fallback font than the rest. The second line of the sample specifies Bitstream Vera Sans directly using font-family: "Bitstream Vera Sans", and does not show the problem. (The font difference is most obvious in the final character.)

I observed that if I use Zoom In or Zoom Out to change the size of the displayed text, both lines render correctly; subsequently using Reload to refresh the page brings back the problem with those three characters on line 1.

The exact set of characters that are rendered incorrectly is variable; on some occasions, especially when switching back into Minefield after activating another application for some time, I have seen a different fallback font used for the first Japanese character as well. This behavior seems a bit unpredictable, however.

Safari on the same machine consistently renders both lines correctly (matching line 2 of the Minefield display).

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Font selection happens in gfxFontGroup::ComputeRanges:
http://mxr.mozilla.org/mozilla-central/search?string=gfxfontgroup%3A%3Acomputeranges

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: fix for uninitialized variable that caused premature return when resolving fonts

Due to an uninitialized flag, there is a strong probability that in the case where a font is found in the mUserFontSet, gfxFontGroup::ForEachFontInternal will exit early while resolving font families (at line 1033 of gfxFont.cpp).

In the test case given, this was leading to a truncated FontList containing only the downloaded font, and not the language-appropriate font for the generic sans-serif family; subsequently, gfxFontGroup::FindFontForChar would fall back to WhichPrefFontSupportsChar() rather than finding the appropriate Japanese font from the active font group.

With this patch, which ensures that  aborted  is set to PR_FALSE until specifically used, the test document renders correctly and consistently. (An uninitialized local variable being the cause is also consistent with the observation that the behavior was not 100% predictable, and could vary with changing focus and redrawing of the frame even within a single run of the program.)

NB: while investigating this, I was also puzzled by lines 1124-1126 of gfxFont.cpp:

    // if match, return
    if (selectedFont)
        return selectedFont.forget();

I don't see how  selectedFont  can have any useful value at this point in the function. Might this be an obsolete fragment of code that ought to be removed?

Comment 11

[John Daggett (:jtd)]

Patch looks good.  

> NB: while investigating this, I was also puzzled by lines 1124-1126 of
> gfxFont.cpp:
> 
>     // if match, return
>     if (selectedFont)
>         return selectedFont.forget();
> 
> I don't see how  selectedFont  can have any useful value at this point in the
> function. Might this be an obsolete fragment of code that ought to be removed?

This looks obsolete, feel free to trim it out.

One small nit, could you modify your .hgrc file to include the lines below, as noted in the Mercurial FAQ https://developer.mozilla.org/en/Mercurial_FAQ 

[diff]
git = 1

[defaults]
diff=-p -U 8

This will give your diff's more context.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 346261 [details] [diff] [review]
fix for uninitialized variable that caused premature return when resolving fonts

great!

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: revised patch - more context, also removes obsolete code

Revised patch using preferred diff settings. Also eliminated the obsolete code mentioned in comment #10, although I believe that code was harmless in practice because of nsRefPtr's initialization to null.

Comment 14

[Jonathan Kew [:jfkthame]]

See bug 452870#c17 for reftest related to this bug.

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: updated patch with username and commit message, ready for checkin

Comment 17

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

Landed on mozilla-central (and thus also on mozilla-1.9.1):
http://hg.mozilla.org/mozilla-central/rev/77825d347650

It turns out that this made a bunch of reftests unexpectedly pass, showing that this fixed bug 465409:
http://hg.mozilla.org/mozilla-central/rev/a036f2285660

I still need to investigate whether it also fixed bug 465408.

Comment 19

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

It also fixed bug 465408 and the test that was random on x86-Mac, so I adjusted the reftest.list appropriately; there are now no longer any random tests in reftests/font-face/reftest.list :
http://hg.mozilla.org/mozilla-central/rev/e29be5f4f686