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).

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.

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

The reason for that is, in the first load we're constructing a textrun for the whole string "abcdef &#x2007;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 "&#x2007;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.

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.

(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?

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?

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

(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.

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?

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

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)

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.

Checked in that debug-only dumping code.

Checked in patch for this on Monday.

Should this be marked as fixed?

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

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

(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.