1730460 - Crash in [@ mozilla::fontlist::Family::FindAllFacesForStyleInternal]

Status: VERIFIED FIXED

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

Description

[Gabriele Svelto [:gsvelto]]

Crash report: https://crash-stats.mozilla.org/report/index/1f0359f8-c800-4506-9edd-79c360210913

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 xul.dll mozilla::fontlist::Family::FindAllFacesForStyleInternal const gfx/thebes/SharedFontList.cpp:239
1 xul.dll mozilla::fontlist::Family::FindFaceForStyle const gfx/thebes/SharedFontList.cpp:442
2 xul.dll gfxFontGroup::FindFontForChar gfx/thebes/gfxTextRun.cpp:3354
3 xul.dll gfxFontGroup::InitScriptRun<char16_t> gfx/thebes/gfxTextRun.cpp:2674
4 xul.dll gfxFontGroup::MakeTextRun gfx/thebes/gfxTextRun.cpp:2473
5 xul.dll BuildTextRunsScanner::FlushFrames layout/generic/nsTextFrame.cpp:1661
6 xul.dll nsTextFrame::EnsureTextRun layout/generic/nsTextFrame.cpp:2991
7 xul.dll nsTextFrame::ReflowText layout/generic/nsTextFrame.cpp:9362
8 xul.dll nsLineLayout::ReflowFrame layout/generic/nsLineLayout.cpp:878
9 xul.dll nsBlockFrame::DoReflowInlineFrames layout/generic/nsBlockFrame.cpp:4355

This just popped up in nightly, only a handful of crashes. I'm unsure what's going on because of the inlining, I'll probably have to look at a minidump in VS.

Comment 1

[Daniel Holbert [:dholbert]]

Jonathan, looks like a crash in some shared-font-list code; do you know what might be going on here?

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I reproduced a crash having same signature with attachment 9241130 [details].

Comment 3

[Jonathan Kew [:jfkthame]]

This could be another manifestation of the underlying issue in bug 1730456; let's see if that fix (just landed) makes it stop.

Comment 4

[blinky]

Steps to reproduce:

1. Download Font Loader.
2. Download Franklin Gothic Book Regular.ttf.
3. Open this page.
4. Open the Font Loader, Click on the Add Fonts button, Select the font file Franklin Gothic Book Regular.ttf then click Open.
5. Click on the Load button then Unload several times.

Actual results:

The tab crashed.

Crash report: bp-afb88c21-8d0c-4784-a60d-be07e0210928

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 xul.dll mozilla::fontlist::Family::FindAllFacesForStyleInternal const gfx/thebes/SharedFontList.cpp:239
1 xul.dll mozilla::fontlist::Family::FindFaceForStyle const gfx/thebes/SharedFontList.cpp:442
2 xul.dll gfxFontGroup::FindFontForChar gfx/thebes/gfxTextRun.cpp:3355
3 xul.dll gfxFontGroup::InitScriptRun<char16_t> gfx/thebes/gfxTextRun.cpp:2675
4 xul.dll gfxFontGroup::MakeTextRun gfx/thebes/gfxTextRun.cpp:2474
5 xul.dll BuildTextRunsScanner::FlushFrames layout/generic/nsTextFrame.cpp:1661
6 xul.dll nsTextFrame::EnsureTextRun layout/generic/nsTextFrame.cpp:2991
7 xul.dll nsTextFrame::ReflowText layout/generic/nsTextFrame.cpp:9378
8 xul.dll nsLineLayout::ReflowFrame layout/generic/nsLineLayout.cpp:878
9 xul.dll nsInlineFrame::Reflow layout/generic/nsInlineFrame.cpp:359

Comment 5

[blinky]

Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7518e48eae5786a363fca3ae42955003a2cd5138&tochange=fff777ea5637c5f0aac0795b20e37fe86b2ac163

Regressed by: bug 1715501

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Jonathan Kew (:jfkthame) from comment #3)

This could be another manifestation of the underlying issue in bug 1730456; let's see if that fix (just landed) makes it stop.

Looks like it didn't make it stop, unfortunately. Comment 4's crash report is with Nightly 20210927153454 (from a few days after bug 1730456 landed).

Thanks to blinky for the regression range and STR! Putting this back in jfkthame's needinfo queue. :)

Comment 7

[Jonathan Kew [:jfkthame]]

Just encountered this on my macOS nightly: https://crash-stats.mozilla.org/report/index/a326beda-d12a-4362-903b-9fcf50211009. The tab that crashed was viewing a treeherder page, so nothing out of the ordinary in terms of fonts, etc.

The regression range pointing to bug 1715501 is interesting, though I'm a bit suspicious it could also be misleading -- it's possible the real issue was pre-existing, and 1715501 just altered timing in a way that affected the exact circumstances that can hit it.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Jonathan, do you have access to a Windows machine to try debugging with the STR from comment 4?

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1730460 - Ensure cached ReplacementCharacter family records are cleared when the font-list is reinitialized. r=emilio

Prior to bug 1715501, we were relying on the UpdateFontVisibility() method
to take care of this, but since making this a per-visibility-setting array,
that no longer happens so we need to explicitly clear them here.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91166fa9642a
Ensure cached ReplacementCharacter family records are cleared when the font-list is reinitialized. r=emilio

Comment 11

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/91166fa9642a

Comment 12

[Jonathan Kew [:jfkthame]]

Comment on attachment 9246495 [details]
Bug 1730460 - Ensure cached ReplacementCharacter family records are cleared when the font-list is reinitialized. r=emilio

Beta/Release Uplift Approval Request

• User impact if declined: Potential crash on pages with encoding errors (being displayed as U+FFFD ) if the system font configuration is modified.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment 4.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just ensures cached font records are cleared during reinitialization.
• String changes made/needed:

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9246495 [details]
Bug 1730460 - Ensure cached ReplacementCharacter family records are cleared when the font-list is reinitialized. r=emilio

Approved for 94.0b8, let's see if this catches the main cases we're hitting.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5e2101fa011f

Comment 15

[Alexandru Trif, Desktop QA [:atrif]]

Reproduced the issue with Firefox 94.0a1 (20210928213647) on Windows 10x64 and STR from comment 4.
The issue is verified fixed with Firefox 94.0b8 (20211019190240) and 95.0a1 (20211020093007) on Windows 10x64. Tab is no longer crashing after following STR from comment 4.