91506 2012-07-17 09:05:02 -0700 Typo in FontCacheWin.cpp causes return value from getCachedFontData() in getLastResortFallbackFont() to be ignored 2012-07-20 14:27:21 -0700 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) PC Windows 7 RESOLVED FIXED P2 Normal --- 0 mark.salisbury webkit-unassigned joepeck mark.salisbury mitz roger_fong thorton webkit.review.bot oldest_to_newest 671305 0 152770 mark.salisbury 2012-07-17 09:05:02 -0700 Created attachment 152770 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. 671309 1 mark.salisbury 2012-07-17 09:15:54 -0700 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. 671364 2 joepeck 2012-07-17 10:29:21 -0700 (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> 673775 3 joepeck 2012-07-19 12:19:40 -0700 (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. 673783 4 mark.salisbury 2012-07-19 12:28:25 -0700 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. 673829 5 joepeck 2012-07-19 12:55:01 -0700 (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. 674869 6 153573 mark.salisbury 2012-07-20 12:48:50 -0700 Created attachment 153573 Proposed fix 674951 7 mark.salisbury 2012-07-20 14:10:27 -0700 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 674955 8 thorton 2012-07-20 14:11:43 -0700 The Chromium EWS comments in the bug if there are failures; it looks green above. 674963 9 153573 joepeck 2012-07-20 14:16:29 -0700 Comment on attachment 153573 Proposed fix r=me. Thanks! 674973 10 153573 webkit.review.bot 2012-07-20 14:27:16 -0700 Comment on attachment 153573 Proposed fix Clearing flags on attachment: 153573 Committed r123262: <http://trac.webkit.org/changeset/123262> 674974 11 webkit.review.bot 2012-07-20 14:27:21 -0700 All reviewed patches have been landed. Closing bug. 152770 2012-07-17 09:05:02 -0700 2012-07-17 09:05:02 -0700 Page which I used to reproduce bug date-time-test.html text/html 340 mark.salisbury PHNjcmlwdD4NCg0KdmFyIHRvZGF5ID0gbmV3IERhdGUoKQ0KdmFyIGQxID0gbmV3IERhdGUoIk9j dG9iZXIgMTMsIDE5NzUgMTE6MTM6MDAiKTsNCnZhciBkMiA9IG5ldyBEYXRlKDc5LDUsMjQpOw0K dmFyIGQzID0gbmV3IERhdGUoNzksNSwyNCwxMSwzMywwKTsNCg0KDQpkb2N1bWVudC53cml0ZSgi PGRpdj5kMSA9ICIgKyBkMS50b1N0cmluZygpICsgIjwvZGl2PiIpOw0KZG9jdW1lbnQud3JpdGUo IjxkaXY+ZDIgPSAiICsgZDIudG9TdHJpbmcoKSArICI8L2Rpdj4iKTsNCmRvY3VtZW50LndyaXRl KCI8ZGl2PmQzID0gIiArIGQzLnRvU3RyaW5nKCkgKyAiPC9kaXY+Iik7DQoNCjwvc2NyaXB0Pg== 153573 2012-07-20 12:48:50 -0700 2012-07-20 14:27:16 -0700 Proposed fix Patch-91506.patch text/plain 1736 mark.salisbury SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZw0KPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQ0KLS0tIFNvdXJjZS9X ZWJDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTIzMjQ3KQ0KKysrIFNvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwkod29ya2luZyBjb3B5KQ0KQEAgLTEsMyArMSwxOCBAQA0KKzIwMTItMDctMjAgIE1h cmsgU2FsaXNidXJ5ICA8bWFyay5zYWxpc2J1cnlAaHAuY29tPgorCisgICAgICAgIFR5cG8gaW4g Rm9udENhY2hlV2luLmNwcCBjYXVzZXMgcmV0dXJuIHZhbHVlIGZyb20gZ2V0Q2FjaGVkRm9udERh dGEoKSBpbiBnZXRMYXN0UmVzb3J0RmFsbGJhY2tGb250KCkgdG8gYmUgaWdub3JlZAorICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9OTE1MDYKKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBObyBuZXcgdGVzdC4gIFRo ZSAiZmFsbGJhY2sgZmFsbGJhY2siIGNvZGUgZW5kcyB1cCBjcmVhdGluZyBhIGZvbnQsIHNvIHlv dSBhbHdheXMKKyAgICAgICAgZ2V0IGEgZmFsbGJhY2sgZm9udCBldmVuIHdpdGggdGhpcyBjb2Rl IGJ1Zy4gIEEgdGVzdCB3b3VsZCBiZSBidWdneSwgYmVpbmcgaGlnaGx5CisgICAgICAgIHBsYXRm b3JtIGRlcGVuZGVudCBvbiB3aGF0IHRoZSAiZmFsbGJhY2sgZmFsbGJhY2siIGZvbnQgaXMgb24g dGhlIHBhcnRpY3VsYXIKKyAgICAgICAgd2luZG93cyBtYWNoaW5lIHRoZSB0ZXN0IHJ1bnMgb24u CisKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy93aW4vRm9udENhY2hlV2luLmNwcDoKKyAg ICAgICAgKFdlYkNvcmU6OkZvbnRDYWNoZTo6Z2V0TGFzdFJlc29ydEZhbGxiYWNrRm9udCk6CisK IDIwMTItMDctMjAgIFJ5b3N1a2UgTml3YSAgPHJuaXdhQHdlYmtpdC5vcmc+CiAKICAgICAgICAg UkVHUkVTU0lPTihyMTIyODczKTogMTUlIHJlZ3Jlc3Npb24gb24gRHJvbWFlby9kb20tYXR0cgpJ bmRleDogU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3Mvd2luL0ZvbnRDYWNoZVdpbi5j cHANCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT0NCi0tLSBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy93aW4v Rm9udENhY2hlV2luLmNwcAkocmV2aXNpb24gMTIyODQ3KQ0KKysrIFNvdXJjZS9XZWJDb3JlL3Bs YXRmb3JtL2dyYXBoaWNzL3dpbi9Gb250Q2FjaGVXaW4uY3BwCSh3b3JraW5nIGNvcHkpDQpAQCAt MzMxLDcgKzMzMSw3IEBADQogICAgIH07CiAgICAgU2ltcGxlRm9udERhdGEqIHNpbXBsZUZvbnQ7 CiAgICAgZm9yIChzaXplX3QgaSA9IDA7IGkgPCBXVEZfQVJSQVlfTEVOR1RIKGZhbGxiYWNrRm9u dHMpOyArK2kpIHsKLSAgICAgICAgaWYgKHNpbXBsZUZvbnQgPSBnZXRDYWNoZWRGb250RGF0YShm b250RGVzY3JpcHRpb24sIGZhbGxiYWNrRm9udHNbaV0pLCBmYWxzZSwgc2hvdWxkUmV0YWluKSB7 CisgICAgICAgIGlmIChzaW1wbGVGb250ID0gZ2V0Q2FjaGVkRm9udERhdGEoZm9udERlc2NyaXB0 aW9uLCBmYWxsYmFja0ZvbnRzW2ldLCBmYWxzZSwgc2hvdWxkUmV0YWluKSkgewogICAgICAgICAg ICAgZmFsbGJhY2tGb250TmFtZSA9IGZhbGxiYWNrRm9udHNbaV07CiAgICAgICAgICAgICByZXR1 cm4gc2ltcGxlRm9udDsKICAgICAgICAgfQo=