412859 - Nondeterministic line height in testcase with U+2007 (FIGURE SPACE)

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect, P3)

Description

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

This is a weird testcase. Reloading the testcase can sometimes make the gap between the two lines differ by one pixel. Repeated reloads generally don't change anything, but if you do something else in the browser for a while and then come back you might see it change. That suggests use of uninitialized memory. It's a problem because my testcase is derived from a reftest (non-breakable-1-ref.html, in particular).

Comment 1

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

Attachment: testcase

Comment 2

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

The difference is in the second line. Sometimes it's

            line 0x272e738: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4100] {0,1152,3107,1236} <
              Text(0)@0x272e6f4[8,8,T]  prev-continuation=0x272382c {0,1152,3107,1140} [state=10600204] SELECTED [content=0x27186f0] sc=0x272e300 pst=:-moz-non-element<
                "\u2007abcdef\n"
              >
            >

After reloading it's

            line 0x4081c138: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4100] {0,1152,3134,1296} <
              Text(0)@0x4081c0f4[8,8,T]  prev-continuation=0x40899a2c {0,1152,3134,1200} [state=10600204] SELECTED [content=0x23918f0] sc=0x4081bd00 pst=:-moz-non-element<
                "\u2007abcdef\n"
              >
            >

Note that it's taller and wider. I wonder if this is nondeterminism in font selection.

Comment 3

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

For the first load we're using DejaVuSerifCondensed for the FIGURE SPACE. After reload we're using Monaco.

Comment 4

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

The reason for that is, in the first load we're constructing a textrun for the whole string "abcdef  abcdef", so the first characters matched Times-Roman and aPrevFont is non-null. Thus Monaco gets a score of 20 for having the glyph plus 2 for having nearly the right weight. In the second load (due to the textrun cache, I guess) we're only constructing a textrun for the string " abcdef", so aPrevFont is null and we give Monaco a score of 20 for having the glpyh and 5 for not being bold or italic.

I'm not sure why this second string isn't hitting in the textrun cache too, it should, so I'll look into that.

Do we want to allow font selection for a character to depend on the font selected for the previous character? Apart from the ranking dependency, there's also this:

    // 3. use fallback fonts
    // -- before searching for something else check the font used for the previous character
    if (!selectedFont && aPrevMatchedFont && aPrevMatchedFont->TestCharacterMap(aCh)) {
        selectedFont = aPrevMatchedFont;
        return selectedFont.forget();
    }

I had thought we were trying to avoid this, apart from the ZWJ special case.

If we want to allow this, then I think after encountering whitespace we should clear mFirstPass in CmapFontMatcher. ("mFirstPass" is a misleading name, BTW.) This will make the font selection results at least independent of the contents of the textrun cache.

Comment 5

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

Also, even if we allow stuff like the code I cited above, I think the font we pass to gfxQuartzFontCache::FindFontForChar for matching should always be the first font in the font group, not the previous font.

Comment 6

[John Daggett (:jtd)]

(In reply to comment #4)

Yeah, using the first font group is better, that's what the Windows code does.  So a change on the line below should fix the problem:

http://mxr.mozilla.org/seamonkey/source/gfx/thebes/src/gfxAtsuiFonts.cpp#804

aPrevMatchedFont should instead be GetFontAt(0), that way the ranking within gfxQuartzFontCache::FindFontForCharProc will be consistent in both cases.

> Do we want to allow font selection for a character to depend on the font
> selected for the previous character?

This is an optimization in both the Windows/Mac font fallback code, we're trying to avoid walking the list of system fonts.  This only applies to the fallback situation, where all the fonts in the font group and in the list of pref fonts has already been tried.  But you're right, this does make the font selection dependent on how the text runs get chopped up.  The characters involved would need to (1) not be in pref fonts and (2) exist in different fonts with different style characteristics (italic/weight) for this dependency to crop up.

> "mFirstPass" is a misleading name, BTW

mFirstChar would be better maybe?

Comment 7

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

mFirstRange?

I can live with the previous-font thing, but we need to make sure it's null after we've processed whitespace. Do you want to write patches here or should I?

I've figured out why we're not hitting the textrun cache for the second line when we reload. Basically every page transition flushes the docshell's devicecontext font metrics cache, so in the new page we have to create a new fontmetrics for the text, and that creates a new fontgroup (fontgroups aren't cached like fonts are). For cases like this where a word contains glyphs from fonts that aren't the first font in the fontgroup, the textrun word cache uses the fontgroup pointer as the cache key, and now the fontgroup pointers are different.

I wonder if it's worth doing a fontgroup cache for 1.9. What do you think Stuart?

Comment 8

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

Attachment: textrun cache dumping code

This is helpful debug dumping code that I wrote, it might as well land.

Comment 9

[John Daggett (:jtd)]

(In reply to comment #7)

> I can live with the previous-font thing, but we need to make sure it's null
> after we've processed whitespace. Do you want to write patches here or should
> I?

Sure, I can set up a patch for this.

Comment 10

[John Daggett (:jtd)]

This is hard to reproduce.  DejaVu is a Windows font?  A Vista font?  Sorry for the trouble, but could you attach a list of ~/Library/Fonts?

Comment 11

[John Daggett (:jtd)]

Attachment: testcase that should show the different font matching without refresh

Comment 12

[philippe (part-time)]

DejaVu is the sort-of successor to the Bitstream Vera fonts
http://dejavu.sourceforge.net/wiki/index.php/Main_Page

Comment 13

[John Daggett (:jtd)]

With DejaVu fonts installed, and DUMP_TEXT_RUN enabled, you get:

InitTextRun 372067b0 fontgroup 4035bd90 (serif) len 14 TEXTRUN "abcdef  abdcef" ENDTEXTRUN
InitTextRun font: Times-Roman
InitTextRun 372067b0 fontgroup 4035bd90 font 40379260 match Times-Roman (1-7)
InitTextRun 372067b0 fontgroup 4035bd90 font 3720c260 match DejaVuSerifCondensed (8-1)
InitTextRun 372067b0 fontgroup 4035bd90 font 40379260 match Times-Roman (9-6)

InitTextRun 3720c9f0 fontgroup 4035bd90 (serif) len 7 TEXTRUN " abcdef" ENDTEXTRUN
InitTextRun font: Times-Roman
InitTextRun 3720c9f0 fontgroup 4035bd90 font 3720cb50 match Monaco (1-1)
InitTextRun 3720c9f0 fontgroup 4035bd90 font 40379260 match Times-Roman (2-6)

Comment 14

[John Daggett (:jtd)]

Attachment: patch to use firstFont instead of prevFont

This should fix the problem.  It doesn't change the previous font rules, I need to fiddle with that a bit more.  And that should be done both on Windows and on the Mac.

Comment 15

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

Checked in that debug-only dumping code.

Comment 16

[John Daggett (:jtd)]

Checked in patch for this on Monday.

Comment 17

[Jesse Ruderman]

Should this be marked as fixed?

Comment 18

[John Daggett (:jtd)]

Nope, not yet.  roc feels we should clear out the "previous font" setting at white-space boundaries, so I still need to implement that.

Comment 19

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

I think this should be higher priority than P4. It leads to nondeterministic rendering, which can cause various hard-to-diagnose problems.

Comment 20

[John Daggett (:jtd)]

(In reply to comment #7)

> I can live with the previous-font thing, but we need to make sure it's null
> after we've processed whitespace.

This relates to this code below.  In this case, "whitespace" really means just the space character, since that's what the word cache code uses to split text runs.

// 3. use fallback fonts
// -- before searching for something else check the font used for the previous character
if (!selectedFont && aPrevMatchedFont && aPrevMatchedFont->TestCharacterMap(aCh)) {
    selectedFont = aPrevMatchedFont;
    return selectedFont.forget();
}
    
http://mxr.mozilla.org/mozilla/source/gfx/thebes/src/gfxAtsuiFonts.cpp#817

After reviewing the code, this is already effectively the case, the character after a space will never fall through to this case.  This is because *all* fonts have a codepoint for the space character, the previous matched font after a space will always be the first font in the font group.  Font group fonts are the first thing tested in gfxAtsuiFontGroup::FindFontForChar so there is no way that aPrevMatchedFont->TestCharacterMap(aCh) will be true at this point in the code for a character after a space.

If I'm missing something here, please reopen.