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

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Whoever was caching this font group should have called UpdateFontList on it: '!mUserFontSet || mCurrGeneration == GetGeneration()', file gfx/thebes/gfxFont.h, line 2957

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

(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) !

Comment 3

[Jonathan Kew [:jfkthame]]

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 4

[Jonathan Kew [:jfkthame]]

Attachment: patch, ensure canvas calls UpdateFontList on the font group before MakeTextRun

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(A test that just triggers the assertion would be fine.)

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: 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.

Comment 8

[Jonathan Kew [:jfkthame]]

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.

Comment 9

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4d9b327cc69

Comment 10

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/a4d9b327cc69

Comment 11

[Alex Keybl [:akeybl]]

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.

Comment 12

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0e5698e16053

Comment 13

[Virgil Dicu [:virgil] [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/