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

RESOLVED FIXED in Firefox 13

Status

()

Core
Canvas: 2D
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: jfkthame)

Tracking

(Blocks: 2 bugs, {assertion, regression, testcase})

Trunk
mozilla14
assertion, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13 verified)

Details

(Whiteboard: [qa+])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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
(Reporter)

Comment 1

5 years ago
Created attachment 615278 [details]
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) !
(Assignee)

Comment 3

5 years ago
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
(Assignee)

Comment 4

5 years ago
Created attachment 615690 [details] [diff] [review]
patch, ensure canvas calls UpdateFontList on the font group before MakeTextRun
Attachment #615690 - Flags: review?(roc)
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.)
(Assignee)

Comment 7

5 years ago
Created attachment 616057 [details] [diff] [review]
ensure canvas calls UpdateFontList on the font group before MakeTextRun, with crashtest

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

Comment 8

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #616057 - Flags: review+
Attachment #616057 - Flags: approval-mozilla-central? → approval-mozilla-central+
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4d9b327cc69
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/a4d9b327cc69
Status: NEW → RESOLVED
Last Resolved: 5 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+
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/0e5698e16053
Target Milestone: mozilla14 → mozilla13

Updated

5 years ago
status-firefox13: --- → fixed
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/
status-firefox13: fixed → verified
You need to log in before you can comment on or make changes to this bug.