Bug 91506 – Typo in FontCacheWin.cpp causes return value from getCachedFontData() in getLastResortFallbackFont() to be ignored

• Component: Platform
• Status: RESOLVED FIXED

[Mark Salisbury]

Created attachment 152770 [details]
Page which I used to reproduce bug

In FontCacheWin.cpp, getLastResortFallbackFont(), currently line 340, I believe there is a typo that causes the return from getCachedFontData() to be ignored.  I'm a little surprised the compiler even allows this.  The problem is that the closing parenthesis is in the wrong place:

338    SimpleFontData* simpleFont;
339    for (size_t i = 0; i < WTF_ARRAY_LENGTH(fallbackFonts); ++i) {
340        if (simpleFont = getCachedFontData(fontDescription, fallbackFonts[i]), false, shouldRetain) {
341            fallbackFontName = fallbackFonts[i];
342            return simpleFont;
343        }
344    }

It should look like this:

340        if (simpleFont = getCachedFontData(fontDescription, fallbackFonts[i], false, shouldRetain)) {

There is more 'fallback' code after this section; it checks for "a DEFAULT_GUI_FONT is no known Unicode font is available".  Because of this a fallback font will be found.  It looks like this was introduced last year (http://trac.webkit.org/changeset/93140) when the shouldRetain parameter was added to getCachedFontData().

I discovered this by porting this code to Windows CE, which does not support GetStockObject(DEFAULT_GUI_FONT).  After binding out this section of code near the bottom of the function, I hit the ASSERT_NOT_REACHED.  Moving the parenthesis fixes the crash.

Steps to reproduce:
1) Load a page which does not specify a font (I attached the page I found the bug with).
2) Specify in preferences that the webkit standard font is a font which your system does not have (so that the last resort fallback code will be invoked).

Set a breakpoint in getLastResortFallbackFont().  You'll notice that even though it invokes getCachedFontData() on line 340, and the function returns a valid SimpleFontData *, the value is ignored; the code loops through all the fonts then it hits the "last last" resort fallback font code.

[Mark Salisbury]

I plan to submit a trivial patch for this, hopefully later today; it'll be a first for me.  I'm working through the process right now as documented at http://www.webkit.org/coding/contributing.html.

[Joseph Pecoraro]

(In reply to comment #0)
> Created an attachment (id=152770) [details]
> Page which I used to reproduce bug
> 
> In FontCacheWin.cpp, getLastResortFallbackFont(), currently line 340, I believe there is a typo that causes the return from getCachedFontData() to be ignored.  I'm a little surprised the compiler even allows this.  The problem is that the closing parenthesis is in the wrong place:

Wow, great catch!

The compiler is probably fine with it because getCachedFontData has optional parameters (arg!), and this then becomes a comma expression, really only intended for the initializer section for a for loop.  Like: "for (int i=0, j=0; ...)".
<http://publib.boulder.ibm.com/infocenter/comphelp/v8v101/index.jsp?topic=%2Fcom.ibm.xlcpp8a.doc%2Flanguage%2Fref%2Fco.htm>

[Joseph Pecoraro]

(In reply to comment #1)
> I plan to submit a trivial patch for this, hopefully later today; it'll be a first for me.  I'm working through the process right now as documented at http://www.webkit.org/coding/contributing.html.

It has been a few days now. Let me know if you have any questions with the contribution process.

[Mark Salisbury]

Thanks Joe for following up to see where I am.

I got run-webkit-tests running against a wincairo build, then saw some fails that were unexpected so I built the standard apple "release" build yesterday... It's running now but TONS of tests are failing.  I expect it has nothing to do with my code change and more something being wrong in my environment.

I am still working on getting the patch.  It will be a really trivial patch, I'm just trying to follow the process...

This brings up a good question though.  Do you think it's possible/reasonable to create a test for this fix?

I'm not sure how we could create a test for this.  The "fallback fallback" code ends up creating a font, so you always get a fallback font even with this code bug.  The default font (WebCore::Settings) needs to be changed to one that's not available on the system during the testing (and I think that is possible using the internals testing object).  However, the exact font that we would then fallback to depends on user preferences (system control panel settings).  This is a very platform specific issue.  It would be easier to create a test that would show this on WinCE, since it doesn't have the final fallback code, but we don't have layout tests for WinCE at this point in time.

[Joseph Pecoraro]

(In reply to comment #4)

> I am still working on getting the patch.  It will be a really trivial patch, I'm just trying to follow the process...

Thanks for taking the time to do this!


> This brings up a good question though.  Do you think it's possible/reasonable to create a test for this fix?

I think it would be possible, but I'm not sure how reasonable it is. This was just an unfortunate typo, and the fix is straight forward. This is akin to a build fix.

Another interesting aspect of this is that we might be "leaking" these fonts, because the default value for getCachedFontData is to retain the font. So they are probably getting abandoned in the FontCache forever. For these kind of performance issues / improvements we normally don't require tests if they were shown to fix an issue.

And maybe this is already covered by some tests and we just have incorrect expected results for windows? You could attach a simple patch + changelog, and we can run that patch on the "Early Warning System" bots and see if it affects any tests. There is a Windows EWS bot.

[Mark Salisbury]

Created attachment 153573 [details]
Proposed fix

[Mark Salisbury]

I attached a patch and clicked the EWS button.  The results look good, though the few chromium EWS regressions appear flaky to me.  Any regressions should be related to fonts/pixel diffs/layout size, not HTTP failures/tests timing out.

I'm just waiting for a reviewer to take a look now.

Chromium DRT fails:

  http/tests/cache/post-redirect-get.php -> unexpected no expected result found
  http/tests/cache/post-with-cached-subresources.php -> unexpected no expected result found
  editing/spelling/grammar-markers.html -> pixel hash failed (but pixel test still passes)
  fast/events/dispatch-message-string-data.html -> unexpected test timed out
  fast/forms/range/slider-delete-while-dragging-thumb.html -> unexpected text diff mismatch
  fast/forms/range/slider-mouse-events.html -> unexpected text diff mismatch
  fast/forms/range/slider-onchange-event.html -> unexpected text diff mismatch
  fast/frames/cached-frame-counter.html -> unexpected test timed out
  http/tests/security/script-crossorigin-loads-correctly.html -> unexpected text diff mismatch
  fast/loader/loadInProgress.html -> unexpected text diff mismatch
  fast/loader/unload-form-post-about-blank.html -> unexpected test timed out
  http/tests/xmlhttprequest/zero-length-response.html -> unexpected test timed out
  svg/batik/text/smallFonts.svg -> unexpected image mismatch
  svg/batik/text/textLayout2.svg -> unexpected iFailed to run "['xvfb-run', 'Tools/Scripts/new-run-webkit-tests', '--chromium', '--skip-failing-tests', '--no-ne..." exit_code: 3

[Tim Horton]

The Chromium EWS comments in the bug if there are failures; it looks green above.

[Joseph Pecoraro]

Comment on attachment 153573 [details]
Proposed fix

r=me. Thanks!

[WebKit Review Bot]

Comment on attachment 153573 [details]
Proposed fix

Clearing flags on attachment: 153573

Committed r123262: <http://trac.webkit.org/changeset/123262>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.