Last Comment Bug 745699 - "ASSERTION: Whoever was caching this font group should have called UpdateFontList on it"
: "ASSERTION: Whoever was caching this font group should have called UpdateFont...
Status: RESOLVED FIXED
[qa+]
: assertion, regression, testcase
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Jonathan Kew (:jfkthame)
:
:
Mentors:
Depends on:
Blocks: stirdom 379903 703100
  Show dependency treegraph
 
Reported: 2012-04-16 02:18 PDT by Jesse Ruderman
Modified: 2012-05-21 06:03 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified


Attachments
testcase (449 bytes, text/html)
2012-04-16 02:18 PDT, Jesse Ruderman
no flags Details
stack trace (10.49 KB, text/plain)
2012-04-16 02:18 PDT, Jesse Ruderman
no flags Details
patch, ensure canvas calls UpdateFontList on the font group before MakeTextRun (2.81 KB, patch)
2012-04-17 06:32 PDT, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Splinter Review
ensure canvas calls UpdateFontList on the font group before MakeTextRun, with crashtest (3.82 KB, patch)
2012-04-18 02:14 PDT, Jonathan Kew (:jfkthame)
jfkthame: review+
akeybl: approval‑mozilla‑aurora+
blassey.bugs: approval‑mozilla‑central+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-04-16 02:18:12 PDT
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
Comment 1 Jesse Ruderman 2012-04-16 02:18:34 PDT
Created attachment 615278 [details]
stack trace
Comment 2 Benoit Jacob [:bjacob] (mostly away) 2012-04-16 05:11:31 PDT
(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) 2012-04-17 06:31:27 PDT
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) 2012-04-17 06:32:34 PDT
Created attachment 615690 [details] [diff] [review]
patch, ensure canvas calls UpdateFontList on the font group before MakeTextRun
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-17 15:42:53 PDT
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) 2012-04-17 15:43:10 PDT
(A test that just triggers the assertion would be fine.)
Comment 7 Jonathan Kew (:jfkthame) 2012-04-18 02:14:01 PDT
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.
Comment 8 Jonathan Kew (:jfkthame) 2012-04-18 02:16:48 PDT
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 11 Alex Keybl [:akeybl] 2012-04-20 16:06:20 PDT
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) 2012-04-21 00:21:08 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/0e5698e16053
Comment 13 Virgil Dicu [:virgil] [QA] 2012-05-21 06:03:24 PDT
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/

Note You need to log in before you can comment on or make changes to this bug.