Closed Bug 745699 Opened 9 years ago Closed 9 years ago
"ASSERTION: Whoever was caching this font group should have called Update
Font List on it"
449 bytes, text/html
10.49 KB, text/plain
3.82 KB, patch
|Details | Diff | Splinter Review|
###!!! ASSERTION: Whoever was caching this font group should have called UpdateFontList on it: '!mUserFontSet || mCurrGeneration == GetGeneration()', file gfx/thebes/gfxFont.h, line 2957
(In reply to Jesse Ruderman from comment #0) > Created attachment 615277 [details] > testcase > > ###!!! ASSERTION: Whoever was caching this font group should have called > UpdateFontList on it: '!mUserFontSet || mCurrGeneration == GetGeneration()', > file gfx/thebes/gfxFont.h, line 2957 This should really be a crashing assertion (at least NS_ABORT_IF_FALSE) !
I don't think this needs to be anything more drastic than NS_ASSERTION - the worst effect I can think of would be that we use the wrong font face for something. This is (presumably) a regression from bug 703100; the canvas code used to go through gfxTextRunCache to create the textrun it needs for measurement/drawing, and that took care of calling UpdateFontList on the font group. But that was removed and canvas now calls gfxFontGroup::MakeTextRun directly, so it needs to call UpdateFontList first in case of changes to the user font set.
Comment on attachment 615690 [details] [diff] [review] patch, ensure canvas calls UpdateFontList on the font group before MakeTextRun Review of attachment 615690 [details] [diff] [review]: ----------------------------------------------------------------- Please add a test.
Attachment #615690 - Flags: review?(roc) → review+
(A test that just triggers the assertion would be fine.)
Added crashtest based on the original testcase here - fires multiple assertions if the patch is not present. Carrying forward r=roc. Requesting approval to land for mozilla-14. This fixes a minor regression from bug 703100. Patch is very safe, just ensures the fontgroup is refreshed if necessary to avoid risk of canvas text APIs using the wrong face.
Hmm, didn't really mean to request approval-aurora until the patch landed on m-c, but since the flag got set - I do think we should uplift this to ff-13 as well, given the minimal risk, and the possibility that the bug could introduce hard-to-reproduce canvas text misbehavior.
Attachment #616057 - Flags: approval-mozilla-central? → approval-mozilla-central+
Target Milestone: --- → mozilla14
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment on attachment 616057 [details] [diff] [review] ensure canvas calls UpdateFontList on the font group before MakeTextRun, with crashtest [Triage Comment] Recent regression from FF12 - approved for Aurora 13.
Attachment #616057 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Target Milestone: mozilla14 → mozilla13
Verified with Linux beta debug build [*] using attachment test case. No assertions displayed. Could previously reproduce with a build from the report date. [*] http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-05-18-mozilla-beta-debug/
You need to log in before you can comment on or make changes to this bug.