Closed Bug 745699 Opened 9 years ago Closed 9 years ago

"ASSERTION: Whoever was caching this font group should have called UpdateFontList on it"

Categories

(Core :: Canvas: 2D, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14
Tracking Status
firefox13 --- verified

People

(Reporter: jruderman, Assigned: jfkthame)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [qa+])

Attachments

(3 files, 1 obsolete file)

Attached file testcase
###!!! ASSERTION: Whoever was caching this font group should have called UpdateFontList on it: '!mUserFontSet || mCurrGeneration == GetGeneration()', file gfx/thebes/gfxFont.h, line 2957
Attached file stack trace
(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.
Assignee: nobody → jfkthame
Blocks: 703100
Component: Graphics → Canvas: 2D
Keywords: regression
OS: Mac OS X → All
QA Contact: thebes → canvas.2d
Hardware: x86_64 → All
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.
Attachment #615690 - Attachment is obsolete: true
Attachment #616057 - Flags: approval-mozilla-central?
Attachment #616057 - Flags: approval-mozilla-aurora?
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: review+
Attachment #616057 - Flags: approval-mozilla-central? → approval-mozilla-central+
https://hg.mozilla.org/mozilla-central/rev/a4d9b327cc69
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/0e5698e16053
Target Milestone: mozilla14 → mozilla13
Target Milestone: mozilla13 → mozilla14
Whiteboard: [qa+]
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.