1758286 - Crash in [@ gfxFcPlatformFontList::FindAndAddFamilies]

Status: RESOLVED FIXED

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

Description

[Calixte Denizet (:calixte)]

Maybe Fission related. (DOMFissionEnabled=1)

Crash report: https://crash-stats.mozilla.org/report/index/24865c55-7104-4d7c-a9a4-931580220306

MOZ_CRASH Reason: MOZ_RELEASE_ASSERT(!HasEntry())

Top 10 frames of crashing thread:

0 libxul.so gfxFcPlatformFontList::FindAndAddFamilies gfx/thebes/gfxFcPlatformFontList.cpp:2116
1 libxul.so gfxFontGroup::BuildFontList gfx/thebes/gfxTextRun.cpp:1887
2 libxul.so nsFontCache::GetMetricsFor gfx/src/nsFontCache.cpp:92
3 libxul.so BuildTextRunsScanner::FlushFrames layout/generic/nsTextFrame.cpp:1750
4 libxul.so nsTextFrame::ReflowText layout/generic/nsTextFrame.cpp:9561
5 libxul.so nsLineLayout::ReflowFrame layout/generic/nsLineLayout.cpp:873
6 libxul.so nsBlockFrame::ReflowLine layout/generic/nsBlockFrame.cpp:3227
7 libxul.so nsBlockFrame::Reflow layout/generic/nsBlockFrame.cpp:1394
8 libxul.so nsContainerFrame::ReflowChild layout/generic/nsContainerFrame.cpp:1005
9 libxul.so nsHTMLScrollFrame::ReflowScrolledFrame layout/generic/nsGfxScrollFrame.cpp:838

There are 5 crashes (from 2 installations) in nightly 99 with buildid 20220306094505. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1756400.

[1] https://hg.mozilla.org/mozilla-central/rev?node=4f8f857f310d

Comment 1

[Donal Meehan [:dmeehan]]

This may start to spike now that 99 is in Beta.

Comment 2

[Jonathan Kew [:jfkthame]]

The patches in bug 1756400, aside from the actual functional change, also aimed to optimize the performance here by replacing the separate Lookup() potentially followed by InsertOrUpdate() calls when working with the mFcSubstituteCache hashtable with a single LookupOrInsertWith() call.

It appears that when InsertInternal() is used within the LookupOrInsertWith() method, we're hitting a MOZ_RELEASE_ASSERT within the hashtable code here, although I haven't figured out how that could happen. (And have no idea how to reproduce this locally, for debugging.)

As an experiment, I propose to undo the restructuring of the code to use LookupOrInsertWith(), so that we return to the original code structure (but retaining the improved language handling), to see if this avoids the issue here.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1758286 - Revert to separate Lookup and InsertOrUpdate calls when working with mFcSubstituteCache. r=lsalzman

This shouldn't change behavior at all, afaict, but by reverting to the original
code structure I'm hoping it may avoid the release-assert that has showed up
in a few crash reports.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Donal Meehan [:dmeehan] from comment #1)

This may start to spike now that 99 is in Beta.

(ni=jfkthame as a reminder that we'll need a beta-uplift-request ASAP, once the patch has hit Nightly and we're confident that it avoids the release-assert.)

Comment 5

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

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2378d74d6ae5
Revert to separate Lookup and InsertOrUpdate calls when working with mFcSubstituteCache. r=lsalzman

Comment 7

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/2378d74d6ae5

Comment 8

[dector9]

This issue is reproducible for me on 99.0b1 and 99.0b2 (including safe mode). Around 95% of websites that I'm visiting are affected (gmail or youtube are not crashing instantly but after some scrolling).

Issue IS NOT reproducible on nightly build downloaded on Mar 11.

Any chances to have fix in 99.0b3? Thanks!

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 9266900 [details]
Bug 1758286 - Revert to separate Lookup and InsertOrUpdate calls when working with mFcSubstituteCache. r=lsalzman

Beta/Release Uplift Approval Request

• User impact if declined: Frequent crashes for some users on Linux
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce: (No known STR, but crash-stats and an affected user's report in comment 8 suggest this avoids the issue.)
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Reverting to earlier, established code structure that does not involve calling LookupOrInsertWith(), where the failure is apparently happening.
• String changes made/needed:

Comment 10

[Donal Meehan [:dmeehan]]

Comment on attachment 9266900 [details]
Bug 1758286 - Revert to separate Lookup and InsertOrUpdate calls when working with mFcSubstituteCache. r=lsalzman

Approved for 99.0b3. Thanks.

Comment 11

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/09fd7a924898