Crash in gfxTextRun::AccumulateMetricsForRun when no vector font is installed

VERIFIED FIXED in Firefox 56

Status

()

defect
--
critical
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: bugs, Assigned: jfkthame)

Tracking

({crash})

56 Branch
mozilla57
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox55 wontfix, firefox56 verified, firefox57 verified)

Details

(crash signature)

Attachments

(1 attachment)

Reporter

Description

2 years ago
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

2 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

2 years ago
Assignee: nobody → jfkthame
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee

Comment 2

2 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
Assignee

Updated

2 years ago
Blocks: 1388059
Attachment #8894452 - Flags: review?(lsalzman) → review+

Comment 3

2 years ago
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

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/551898614c2c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee

Comment 6

2 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?
Blocks: 1389553
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+
Flags: qe-verify+
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.
Flags: needinfo?(jfkthame)
Keywords: crash
Assignee

Comment 10

2 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?
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)
Looks like this would need a rebase around bug 1360620.  Probably not worth it considering the low rate.
Flags: needinfo?(jfkthame)
Attachment #8894452 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52-
Depends on: 1390635
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+
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.