Closed
Bug 1385462
Opened 7 years ago
Closed 7 years ago
Crash in gfxTextRun::AccumulateMetricsForRun when no vector font is installed
Categories
(Core :: Graphics: Text, defect)
Tracking
()
VERIFIED
FIXED
mozilla57
People
(Reporter: bugs, Assigned: jfkthame)
References
Details
(Keywords: crash)
Crash Data
Attachments
(1 file)
947 bytes,
patch
|
lsalzman
:
review+
lizzard
:
approval-mozilla-beta+
jcristau
:
approval-mozilla-esr52-
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-4f718bb7-eafd-403b-a025-7b95d0170728. ============================================================= When no TTF or OTF fonts are installed, going to any Twitter page reproduces that crash. Other web pages and the chrome are using the default bitmap fonts provided with Xorg, as well as the coloured emoji font for numbers. Other web pages don’t seem to trigger that crash.
Assignee | ||
Comment 1•7 years ago
|
||
The crash here occurs because the gfxTextRun measurement/drawing code assumes that when GlyphRunIterator::NextRun returns true, we have a usable glyph run (with a non-null font), but NextRun fails to ensure this is the case. Fixing this prevents the crashes here. I'll file a separate bug for the fact that we end up rendering hexboxes for text that is too far from any of the available bitmap font sizes, in the case where no scalable fonts are installed, which is what leads to twitter.com (and other pages with large headings) hitting this issue.
Attachment #8894452 -
Flags: review?(lsalzman)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jfkthame
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 2•7 years ago
|
||
For additional context, this occurs when the SizeDistance function in gfxFcPlatformFontList.cpp[1] ends up rejecting all the available faces for all the available font families because they're all bitmap sizes, and the requested size is too far away. (If we always pass 'true' for the aForceScalable flag here, the problem doesn't arise, but of course this would change the font matching behavior when both scalable and non-scalable fonts are installed, so needs further consideration.) [1] https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/gfx/thebes/gfxFcPlatformFontList.cpp#1071
Updated•7 years ago
|
Attachment #8894452 -
Flags: review?(lsalzman) → review+
Pushed by jkew@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/551898614c2c Ensure GlyphRunIterator::NextRun returns false immediately if the textrun's only glyphrun does not have a valid font. r=lsalzman
Assignee | ||
Comment 4•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/551898614c2c8e44fd249cf7f924c9e43d3eff01 Bug 1385462 - Ensure GlyphRunIterator::NextRun returns false immediately if the textrun's only glyphrun does not have a valid font. r=lsalzman
Comment 5•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/551898614c2c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee | ||
Comment 6•7 years ago
|
||
Comment on attachment 8894452 [details] [diff] [review] Ensure GlyphRunIterator::NextRun returns false immediately if the textrun's only glyphrun does not have a valid font Approval Request Comment [Feature/Bug causing the regression]: 1350783 [User impact if declined]: potential crash for users with only bitmap fonts [Is this code covered by automated tests?]: normal use of this code is exercised throughout tests (part of the main text-rendering code), but the specific edge case addressed here does not arise (dependent on the system's font configuration) [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: on Linux, uninstall *all* scalable fonts (truetype, opentype, type1), leaving only X11 bitmap fonts enabled; then visit sites that have text in large font sizes [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: just adds a check so the iterator will return false immediately in the degenerate case where no valid glyph run is present [String changes made/needed]: none
Attachment #8894452 -
Flags: approval-mozilla-beta?
Updated•7 years ago
|
status-firefox56:
--- → affected
Comment 7•7 years ago
|
||
Comment on attachment 8894452 [details] [diff] [review] Ensure GlyphRunIterator::NextRun returns false immediately if the textrun's only glyphrun does not have a valid font Seems like an edge case but OK, let's uplift to fix the crash.
Attachment #8894452 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 8•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/e08775132fa2
Updated•7 years ago
|
Flags: qe-verify+
Comment 9•7 years ago
|
||
Given that ESR52/TB52 are also hitting these crashes and the patch is a one-liner, I think we should nominate it for approval there as well.
status-firefox55:
--- → wontfix
status-firefox-esr52:
--- → affected
Flags: needinfo?(jfkthame)
Keywords: crash
Assignee | ||
Comment 10•7 years ago
|
||
Comment on attachment 8894452 [details] [diff] [review] Ensure GlyphRunIterator::NextRun returns false immediately if the textrun's only glyphrun does not have a valid font Trivial patch to fix crash for Linux users with no vector fonts installed; see comment 6, comment 9.
Flags: needinfo?(jfkthame)
Attachment #8894452 -
Flags: approval-mozilla-esr52?
Comment 11•7 years ago
|
||
Doesn't apply to esr52: grafting 415147:551898614c2c "Bug 1385462 - Ensure GlyphRunIterator::NextRun returns false immediately if the textrun's only glyphrun does not have a valid font. r=lsalzman" merging gfx/thebes/gfxTextRun.cpp warning: conflicts while merging gfx/thebes/gfxTextRun.cpp! (edit, then use 'hg resolve --mark') abort: unresolved conflicts, can't continue (use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(jfkthame)
Comment 12•7 years ago
|
||
Looks like this would need a rebase around bug 1360620. Probably not worth it considering the low rate.
Flags: needinfo?(jfkthame)
Updated•7 years ago
|
Attachment #8894452 -
Flags: approval-mozilla-esr52? → approval-mozilla-esr52-
Comment 13•7 years ago
|
||
Reproduced the initial crash using an old Nightly build from 2017-07-28: - bp-84510421-213e-4cac-ba9e-8202a0170911 - bp-a11c8c9b-4d6a-44bf-8f47-4ddeb0170911 Verified that using Firefox 56 beta 10 the crash does not occur anymore on Ubuntu 16.04 64bit.
Flags: qe-verify+
Comment 14•7 years ago
|
||
Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0 Build ID: 20171003100226 Reproduced the initial crash using Nightly build from 2017-07-28: - https://crash-stats.mozilla.com/report/index/fb7958c2-6d21-4ff3-bb7a-8be500171003 Verified as fixed using Nightly 57.0a1 (2017-09-04) and latest Nightly 58.0a1 (20171003100226) on Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•