1730456 - Crash in [@ gfxFontGroup::FindFontForChar]

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[blinky]

Steps to reproduce:

1. Open attachment 9221882 [details].
2. Installing/removing the Flags Color World font several times.

Actual results:

The tab crashed.

Crash report: bp-c1a8b382-b6d1-4fd0-89c1-4d50e0210913

Reason: SIGSEGV /SEGV_MAPERR

Top 10 frames of crashing thread:

0 libxul.so gfxFontGroup::FindFontForChar gfx/thebes/gfxTextRun.cpp:3086
1 libxul.so gfxFontGroup::MakeTextRun gfx/thebes/gfxTextRun.cpp:2446
2 libxul.so BuildTextRunsScanner::FlushFrames layout/generic/nsTextFrame.cpp:1661
3 libxul.so BuildTextRunsScanner::ScanFrame layout/generic/nsTextFrame.cpp:2015
4 libxul.so BuildTextRunsScanner::ScanFrame layout/generic/nsTextFrame.cpp:2069
5 libxul.so BuildTextRunsScanner::ScanFrame layout/generic/nsTextFrame.cpp:2069
6 libxul.so BuildTextRunsScanner::ScanFrame layout/generic/nsTextFrame.cpp:2069
7 libxul.so BuildTextRunsScanner::ScanFrame layout/generic/nsTextFrame.cpp:2069
8 libxul.so BuildTextRunsScanner::ScanFrame layout/generic/nsTextFrame.cpp:2069
9 libxul.so nsTextFrame::EnsureTextRun layout/generic/nsTextFrame.cpp:2991

Comment 1

[blinky]

Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=ab80197101b9c7163739f04019ba64fd08770e4b&tochange=d400d0a16ce6c20bfe5addb995f610f4881b31e6

Regressed by: bug 1722487

Comment 2

[blinky]

This similar to bug 1561868.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1722487

Comment 4

[Emilio Cobos Álvarez (:emilio)]

It seems this made bug 1723468 more reproducible. Maybe the same root cause, maybe slightly different...

Comment 5

[Emilio Cobos Álvarez (:emilio)]

I can reliably reproduce this, and we're trying to access unmapped memory, from a quick look at rr.

Jonathan, it seems we reframe correctly, but the font list generation check in here is keeping the old generation around.

Do you have cycles to look at this? If not, do you have pointers? Feel free to bounce the ni? back to me and I can look tomorrow / next week.

Thanks!

Comment 6

[Jonathan Kew [:jfkthame]]

When an extra font is installed (or removed) on the system, the parent process should discard the existing font-list and create it afresh (with an updated generation, so that we know it has changed), using gfxPlatformFontList::InitFontList(). It should then broadcast a notification to all child processes telling them to refresh their font lists as well (dom::ContentParent::NotifyUpdatedFonts(true)).

It looks like what's happening here is that a content process is trying to continue using a shared-memory block after it has been unmapped, presumably because it's got an existing pointer into the old font-list and is trying to use it even after the list has been replaced. So we need to find where it was caching that pointer, and ensure that as part of the font-list rebuild process, it gets discarded. All existing fontlist::Family or fontlist::Face records, and the names, charmaps, etc that they refer to, all become invalid and unsafe to touch when a font-list rebuild happens, because they exist in the shared memory that is entirely released/replaced on a rebuild.

So if you have this in an rr recording, can you identify where the bad pointer comes from and why we still have it around, assuming the content process has handled the updated-fonts notification? Or if that notification hasn't yet been handled, why isn't the old shared memory block still valid at this point? (What generation does the content process think it's looking at, the pre- or post-update one?)

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1730456 - InitPlatformFontList should reframe. r=jfkthame

I don't know why it didn't broadcast the reflow request to child
processes... Perhaps it doesn't have to?

Comment 8

[Pascal Chevrel:pascalc (PTO until April 26)]

Low volume crasher on beta and we are nearing the end of the beta cycle, fix-optional 93.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/454a9b07d164
InitPlatformFontList should reframe. r=jfkthame

Comment 10

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/454a9b07d164

Comment 11

[Jonathan Kew [:jfkthame]]

Comment on attachment 9242125 [details]
Bug 1730456 - InitPlatformFontList should reframe. r=jfkthame

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This fixes a potential use-after-free (or use-after-unmapped), triggered by the system font configuration being changed
• User impact if declined: Potential crash when installing/uninstalling fonts
• Fix Landed on Version: 94
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple change to use existing code to force frame-tree reconstruction
• String or UUID changes made by this patch:

Comment 12

[Jonathan Kew [:jfkthame]]

Comment on attachment 9242125 [details]
Bug 1730456 - InitPlatformFontList should reframe. r=jfkthame

Beta/Release Uplift Approval Request

• User impact if declined: This fixes a potential use-after-free (or use-after-unmapped), triggered by the system font configuration being changed
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple change to use existing code to force frame-tree reconstruction
• String changes made/needed:

Comment 13

[Pascal Chevrel:pascalc (PTO until April 26)]

I am keeping this one on my radar as a ride-along if we have another RC before shipping, thanks.

Comment 14

[Andrei Purice]

Hey blinky,
Can you still reproduce this issue on your side?
I tried reproducing this on Firefox Nightly 94.0a1 (2021-09-13) and the font windows came to a stop (is this where the crash is supposed to happen?) and on Firefox Nightly 95.0a1 (2021-10-08) where this issue didn't occur (the install window was still present).
There was no option to uninstall the font so I just kept opening the install and tried to install over the previous installment. Not sure if this is the correct way to reproduce this issue.

Comment 15

[blinky]

I can not reproduce the crash with the latest Nightly build. Thanks.

Comment 16

[Andrei Purice]

Reproduced the issue in Nightly 94.0a1 (2021-09-13) (20210913095054), using Ubuntu 20.04.
Verified - Fixed on Beta 94.0b3 (20211010185747) and latest Nightly 95.0a1 (2021-10-10) (build id: 20211010214947).

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9242125 [details]
Bug 1730456 - InitPlatformFontList should reframe. r=jfkthame

We're going to disable shared font list on ESR91 instead.