1647573 - Crash in [@ gfxFontGroup::GetUnderlineOffset]

Status: RESOLVED FIXED

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

Description

[blinky]

Steps to reproduce:

1. Set gfx.e10s.font-list.shared to true.
2. Restart Firefox.
3. Download Font Loader.
4. Download and extracts the archive.
5. Open this page in seven tabs.
6. Open the Font Loader, Click on the Add Fonts button, Select the extracted font files then click Open.
7. Click on the Load button.
8. Switch between tabs.

See https://youtu.be/RTY2JclTEyA

Actual results:

The tab crashed when clicking on the Load button then switch between tabs.

Crash report: bp-f51f216b-f619-464e-995b-4ddc20200623

Top 10 frames of crashing thread:

0 xul.dll gfxFontGroup::GetUnderlineOffset gfx/thebes/gfxTextRun.cpp:2819
1 xul.dll nsFontMetrics::MaxDescent gfx/src/nsFontMetrics.cpp:238
2 xul.dll nsTextFrame::ReflowText layout/generic/nsTextFrame.cpp:9384
3 xul.dll nsLineLayout::ReflowFrame layout/generic/nsLineLayout.cpp:881
4 xul.dll nsInlineFrame::Reflow layout/generic/nsInlineFrame.cpp:363
5 xul.dll nsLineLayout::ReflowFrame layout/generic/nsLineLayout.cpp:878
6 xul.dll nsBlockFrame::DoReflowInlineFrames layout/generic/nsBlockFrame.cpp:4254
7 xul.dll nsBlockFrame::ReflowDirtyLines layout/generic/nsBlockFrame.cpp:2658
8 xul.dll nsBlockFrame::Reflow layout/generic/nsBlockFrame.cpp:1375
9 xul.dll nsContainerFrame::ReflowChild layout/generic/nsContainerFrame.cpp:1074

Comment 1

[Ryan VanderMeulen [:RyanVM]]

This is restricted to Nightly-only at the moment.

Comment 2

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

gfx.e10s.font-list.shared is not enabled by default yet. S3 because the work around exists.

Comment 3

[Jonathan Kew [:jfkthame]]

I can reproduce this pretty consistently following the reporter's steps.

What's happening is that although we flush the various font caches when the font-list is rebuilt (because existing font entries are no longer safe to use, as they have pointers into the shared memory that has been discarded), in this case we end up using a font reference that was found via an existing textrun attached to the frame tree, not from the (flushed) font caches.

To avoid the risk of this, we should forcibly reconstruct frames when the font-list is reinitialized; it's not sufficient or safe to just restyle everything.

blinky, there's a tryserver build at https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbe1bb1b5d3c589c29da64d51708335fbfed63d2 with a proof-of-concept patch to address this; if you could confirm whether this prevents the crash, that would be really helpful -- thanks!

Comment 4

[Jonathan Kew [:jfkthame]]

Incidentally, this means the existing behavior (without the shared font list enabled) is already buggy, inasmuch as the reflow that happens after the font-list has been changed can continue using metrics from obsolete font entries that shouldn't be available to layout any more. But the results (e.g. relying on an underline-position metric from a font that's supposed to have been disabled) would be scarcely noticeable in normal cases, whereas with the shared font-list the error suddenly becomes a crash-bug.

Comment 5

[Jonathan Kew [:jfkthame]]

The other thing I noticed while auditing code to understand what was happening here is that we have some places where we use the pointer returned by FamilyFace::FontEntry() without checking for null. This is normally fine but it is possible, at least in theory, for this to fail -- that didn't turn out to be the root cause of the crashes here, but nevertheless we should add the missing null-checks.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1647573 - Always check FamilyFace::FontEntry() for null before trying to use the pointer, as it can fail. r=jwatt

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1647573 - On font-list reinitialization, reconstruct frames rather than just restyling. r=jwatt

As a bonus, this also fixes a pre-existing bug that we never cleared the
mPrefChangePendingNeedsReflow flag after using it, so future pref changes
that shouldn't need a reflow would still trigger one.

Depends on D84518

Comment 8

[blinky]

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/13159d59b7f6
Always check FamilyFace::FontEntry() for null before trying to use the pointer, as it can fail. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/51da3f1e63df
On font-list reinitialization, reconstruct frames rather than just restyling. r=jwatt

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/13159d59b7f6
https://hg.mozilla.org/mozilla-central/rev/51da3f1e63df

Comment 11

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

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.