1627605 - Crash in [@ memcmp | nsTHashtable<T>::s_MatchEntry | PLDHashTable::Add | gfxFontGroup::BuildFontList]

Status: RESOLVED FIXED

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

Description

[Calixte Denizet (:calixte)]

This bug is for crash report bp-207aaf1e-abae-462d-9bf8-4c4940200403.

Top 10 frames of crashing thread:

0 vcruntime140.dll memcmp 
1 xul.dll static nsTHashtable<mozilla::dom::LocalStorageManager::LocalStorageCacheHashKey>::s_MatchEntry xpcom/ds/nsTHashtable.h:404
2 xul.dll PLDHashTable::Add xpcom/ds/PLDHashTable.cpp:567
3 xul.dll gfxFontGroup::BuildFontList gfx/thebes/gfxTextRun.cpp:1746
4 xul.dll gfxFontGroup::gfxFontGroup gfx/thebes/gfxTextRun.cpp:1705
5 xul.dll nsFontMetrics::nsFontMetrics gfx/src/nsFontMetrics.cpp:133
6 xul.dll nsDeviceContext::GetMetricsFor gfx/src/nsDeviceContext.cpp:272
7 xul.dll static nsLayoutUtils::GetFontMetricsForFrame layout/base/nsLayoutUtils.cpp:4717
8 xul.dll BuildTextRunsScanner::BuildTextRunForFrames layout/generic/nsTextFrame.cpp:2365
9 xul.dll BuildTextRunsScanner::FlushFrames layout/generic/nsTextFrame.cpp:1647

There are 10 crashes (from 3 installations) in nightly 76 with buildid 20200403063228. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1619349.

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

Comment 1

[Jonathan Kew [:jfkthame]]

Yes, this must be connected to bug 1619349 in some way, as it's crashing in code that was introduced there. It seems possible that it might be related to bug 1627397, which was just fixed; let's see if it continues to happen.

Leaving needinfo in place for now, as a reminder to check on this after a couple of days.

Comment 2

[blinky]

Steps to reproduce:

1. Set gfx.e10s.font-list.shared to true.
2. Restart Firefox.
3. Download Font Loader.
4. Download Franklin Gothic Book Regular.ttf.
5. Open attachment 9089806 [details].
6. Open the Font Loader, Click on the Add Fonts button, Select the font file Franklin Gothic Book Regular.ttf then click Open.
7. Click on the Load button.

Actual results:

Browser crashes.

Crash report: bp-1a3292c0-a648-4d1a-9195-141fa0200407

Top 10 frames of crashing thread:

0 vcruntime140.dll memcmp 
1 xul.dll static nsTHashtable<mozilla::dom::LocalStorageManager::LocalStorageCacheHashKey>::s_MatchEntry xpcom/ds/nsTHashtable.h:493
2 xul.dll PLDHashTable::Add xpcom/ds/PLDHashTable.cpp:567
3 xul.dll gfxFontGroup::BuildFontList gfx/thebes/gfxTextRun.cpp:1749
4 xul.dll gfxFontGroup::gfxFontGroup gfx/thebes/gfxTextRun.cpp:1708
5 xul.dll nsFontMetrics::nsFontMetrics gfx/src/nsFontMetrics.cpp:133
6 xul.dll nsDeviceContext::GetMetricsFor gfx/src/nsDeviceContext.cpp:272
7 xul.dll static nsLayoutUtils::GetFontMetricsForFrame layout/base/nsLayoutUtils.cpp:4717
8 xul.dll mozilla::ScrollFrameHelper::GetLineScrollAmount const layout/generic/nsGfxScrollFrame.cpp:4432
9 xul.dll nsHTMLScrollFrame::GetLineScrollAmount const layout/generic/nsGfxScrollFrame.h:1393

Comment 3

[blinky]

The crash also happens with any page with the steps in comment 2.

Comment 4

[Jonathan Kew [:jfkthame]]

OK, I see what's happening here. The shared font-list AsString() method returns a string whose buffer is in the shared-memory block (to avoid any copying of the actual characters); this is usually fine, but in this case it's biting us, because we then end up with a string in the mFontMatchingStats hashtable that also shares the same buffer. And if you then trigger a font-list rebuild, e.g. by activating a new font, that buffer gets unmapped, but the hashtable doesn't know it's gone away. So then it'll crash on some future hashtable operation such as EnsureInserted that tries to do a key comparison.

I think the safest fix here is to drop the "optimization" of letting AsString return strings that directly wrap character buffers in the shared memory block. I doubt the cost of copying the string data will be significant in practice, and that will eliminate the footgun here.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1627605 - Avoid creating strings with shared-memory buffers that could become unmapped on a font-list refresh. r=jwatt

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/12fc56d7782c
Avoid creating strings with shared-memory buffers that could become unmapped on a font-list refresh. r=jwatt

Comment 7

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/12fc56d7782c

Comment 8

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

The patch landed in nightly and beta is affected.
:jfkthame, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 9138848 [details]
Bug 1627605 - Avoid creating strings with shared-memory buffers that could become unmapped on a font-list refresh. r=jwatt

Beta/Release Uplift Approval Request

• User impact if declined: Potential crash for users who enable the gfx.e10s.font-list.shared pref (if fonts are installed or removed while the browser is running)
• 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): Just removes unsafe (mis)use of "literal" string API for what is not truly a literal. Only affects code that is preffed-off by default.
• String changes made/needed:

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9138848 [details]
Bug 1627605 - Avoid creating strings with shared-memory buffers that could become unmapped on a font-list refresh. r=jwatt

Fixes a crash for users with the shared font list enabled. Approved for 76.0b5.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7b3a4776fc35