Closed Bug 1561868 Opened 3 years ago Closed 1 year ago

Crash in [@ gfxFontGroup::FindFontForChar]

Categories

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

69 Branch
defect

Tracking

()

RESOLVED FIXED
91 Branch
Tracking Status
firefox-esr78 --- wontfix
firefox69 --- wontfix
firefox88 --- wontfix
firefox89 --- wontfix
firefox90 --- fixed
firefox91 --- fixed

People

(Reporter: whimboo, Assigned: jfkthame)

References

(Regressed 1 open bug)

Details

(Keywords: crash)

Crash Data

Attachments

(3 files)

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:69.0) Gecko/20100101 Firefox/69.0 ID:20190623215020

This bug is for crash report bp-15b512de-721e-4c00-b5b5-789f60190627.

Top 10 frames of crashing thread:

0 XUL gfxFontGroup::FindFontForChar gfx/thebes/gfxTextRun.cpp:3013
1 XUL void gfxFontGroup::InitScriptRun<char16_t> gfx/thebes/gfxTextRun.cpp:2510
2 XUL gfxFontGroup::MakeTextRun gfx/thebes/gfxTextRun.cpp:2304
3 XUL BuildTextRunsScanner::FlushFrames layout/generic/nsTextFrame.cpp:1633
4 XUL BuildTextRunsScanner::ScanFrame layout/generic/nsTextFrame.cpp:1957
5 XUL BuildTextRunsScanner::ScanFrame layout/generic/nsTextFrame.cpp:2011
6 XUL nsTextFrame::EnsureTextRun layout/generic/nsTextFrame.cpp:2891
7 XUL nsTextFrame::AddInlineMinISizeForFlow layout/generic/nsTextFrame.cpp:8084
8 XUL nsTextFrame::AddInlineMinISize layout/generic/nsTextFrame.cpp:8267
9 XUL nsContainerFrame::DoInlineIntrinsicISize layout/generic/nsContainerFrame.cpp:759

I hit this crash only once while kinda fast scrolling through a Slack channel.

Priority: -- → P3

We've seen a few of these in Nightly over the last few weeks, and some in Release as well.

Flags: needinfo?(lsalzman)
Flags: needinfo?(lsalzman) → needinfo?(jfkthame)
Component: Graphics: Text → Layout: Text and Fonts

Some Nightly reports of this look like they may have been bug 1667977, for which a fix recently landed; not all reports look like that, though, so there may be multiple underlying issues. It'll be interesting to see how the crash rate looks in post-1667977 builds...

(Nightly and Release are not at all comparable here, because the gfx.e10s.font-list.shared pref is currently enabled in Nightly but disabled in Release, and so there may be quite different code backing this method. But the pref is set to ride to Release in Fx89, so that factor will be going away.)

Flags: needinfo?(jfkthame)

Steps to reproduce:

  1. Open this page.
  2. Installing/removing the Flags Color World font several times.

See https://youtu.be/LX_AUVt9dGQ

Crash report: bp-3c15fc5d-ae47-422b-9ecb-7c87f0210421

Reason: SIGSEGV /SEGV_MAPERR

Top 10 frames of crashing thread:

0 libxul.so gfxFontGroup::FindFontForChar gfx/thebes/gfxTextRun.cpp:3119
1 libxul.so gfxFontGroup::MakeTextRun gfx/thebes/gfxTextRun.cpp:2470
2 libxul.so BuildTextRunsScanner::BuildTextRunForFrames layout/generic/nsTextFrame.cpp:2562
3 libxul.so BuildTextRunsScanner::FlushFrames layout/generic/nsTextFrame.cpp:1660
4 libxul.so BuildTextRunsScanner::ScanFrame layout/generic/nsTextFrame.cpp:2023
5 libxul.so nsTextFrame::EnsureTextRun layout/generic/nsTextFrame.cpp:2999
6 libxul.so nsTextFrame::ReflowText layout/generic/nsTextFrame.cpp:9264
7 libxul.so nsLineLayout::ReflowFrame layout/generic/nsLineLayout.cpp:878
8 libxul.so nsBlockFrame::DoReflowInlineFrames layout/generic/nsBlockFrame.cpp:4336
9 libxul.so nsBlockFrame::Reflow layout/generic/nsBlockFrame.cpp:1375
Flags: needinfo?(jfkthame)

(In reply to blinky from comment #3)

Steps to reproduce:

  1. Open this page.
  2. Installing/removing the Flags Color World font several times.

Did you also choose the Flags font as the default in Firefox preferences, or something like that? (With default settings, I don't see why it would get used at all.)

Flags: needinfo?(jfkthame)

No, I did not choose the Flags font as the default in Firefox preferences.

I can reproduce the crash with other fonts, but with this font, reproduce it more easily.

Oh, sorry - I misunderstood what I was seeing, it's not a plaintext document but has CSS that specifies the font. OK, that makes sense. I'll try to reproduce and pin down why it's breaking. Thanks!

This crash signature's volume has jumped in Nightly recently: from 2 crashes in Nightly 85.0a1 (and 0 in 86.0a1) to 157 in 89.0a1 and 62 in 90.0a1. So perhaps related to the fix for SearchAllFontsForChar crash bug 1667977 landing in 89.0a1?

But not many users are affected: the 157 crashes from 89.0a1 are only from 18 users and the 62 crashes from 90.0a1 are from only 5 users, so far.

OS: Unspecified → All
Hardware: Unspecified → All
See Also: → 1667977
Attached file lbzlymqrak.html

Attaching blinky's test files to this bug for posterity.

Crash Signature: [@ gfxFontGroup::FindFontForChar] → [@ gfxFontGroup::FindFontForChar] [@ gfxFontGroup::FindFallbackFaceForChar ]

I think the patch just landing in bug 1717595 may also fix this, though I've been having some trouble reproducing just now so not 100% sure.

I have triggered Mac and Linux64 builds on https://treeherder.mozilla.org/jobs?repo=try&revision=79cfc3d309538f10c71cc04b9a4f042b16933d01 which includes that patch; blinky, if you could check whether you can still reproduce this crash with the patched build, that would be great. Thanks!

Flags: needinfo?(over68)

Never mind; I have been able to reproduce this on Linux with a build that includes that patch, so it's clearly not resolved yet.

Flags: needinfo?(over68)

I can reproduce on Windows 10, I don't have Ubuntu right now.

You can try to reproduce on Linux with the steps below.

  1. Open attachment 9221882 [details].
  2. Open this page in new tab.
  3. Installing/removing the Flags Color World font several times.

I figured out at least one scenario where this can happen: if the content process receives a Vsync message that leads to a reflow after the parent has replaced the font list, but before the corresponding RebuildFontList message has been handled. Making the font-list message higher priority, so that it takes precedence over Vsync, seems to enable us to avoid this on my Linux system, at least.

There's a try build in progress at https://treeherder.mozilla.org/jobs?repo=try&revision=d483c1974e7def0169fa8f49b5e1f4cae4ad3462; as always, testing appreciated once it's available.

Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/365a5fd3033b
Give the RebuildFontList message a raised priority, so that it is processed ahead of Vsync. r=lsalzman

Comment on attachment 9229007 [details]
Bug 1561868 - Give the RebuildFontList message a raised priority, so that it is processed ahead of Vsync. r=lsalzman

Beta/Release Uplift Approval Request

  • User impact if declined: Possible crash if the user modifies their OS font configuration while the browser is running
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • 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): Trivial patch just raising priority of the rebuild-font-list IPC message. (This is only used at the (rare) moments when the system font configuration changes dynamically, so there is no relevance to normal browsing activity.)
  • String changes made/needed:
Attachment #9229007 - Flags: approval-mozilla-beta?

I can not reproduce the crash with the build in comment 14. Thanks.

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 91 Branch

Comment on attachment 9229007 [details]
Bug 1561868 - Give the RebuildFontList message a raised priority, so that it is processed ahead of Vsync. r=lsalzman

approved for 90 rc1

Attachment #9229007 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Regressions: 1720068

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

I figured out at least one scenario where this can happen: if the content process receives a Vsync message that leads to a reflow after the parent has replaced the font list, but before the corresponding RebuildFontList message has been handled. Making the font-list message higher priority, so that it takes precedence over Vsync, seems to enable us to avoid this on my Linux system, at least.

Hmm, is this the right fix? Reflows can happen outside vsync events. For example, if the child process is already running a long-running piece of JavaScript code, and then the parent replaces the font list, and then the child JS forces a sync reflow - wouldn't that encounter the same problem?

Flags: needinfo?(jfkthame)

I think you're basically right, this is a mitigation that reduces the chances of encountering problems, but there is still an underlying issue.

In principle, the child process should be holding a reference to the (old) font list until such time as it handles the update message, and so the memory mapping should remain valid as long as that reference is held (shouldn't it?), but what I think I've observed (but can't readily reproduce/debug) is that the shared memory is unmapped before the child has released its handle.

Flags: needinfo?(jfkthame)
See Also: → 1723468
You need to log in before you can comment on or make changes to this bug.