Crash in [@ memcmp | nsTHashtable<T>::s_MatchEntry | PLDHashTable::Add | gfxFontGroup::BuildFontList]
Categories
(Core :: Layout: Text and Fonts, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr68 | --- | unaffected |
firefox74 | --- | unaffected |
firefox75 | --- | unaffected |
firefox76 | --- | fixed |
firefox77 | --- | fixed |
People
(Reporter: calixte, Assigned: jfkthame)
References
(Blocks 1 open bug, Regression)
Details
(Keywords: crash, regression)
Crash Data
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details |
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
Assignee | ||
Comment 1•5 years ago
|
||
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.
Steps to reproduce:
- Set
gfx.e10s.font-list.shared
totrue
. - Restart Firefox.
- Download Font Loader.
- Download Franklin Gothic Book Regular.ttf.
- Open attachment 9089806 [details].
- Open the Font Loader, Click on the Add Fonts button, Select the font file Franklin Gothic Book Regular.ttf then click Open.
- 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
Assignee | ||
Comment 4•5 years ago
|
||
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.
Assignee | ||
Comment 5•5 years ago
|
||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 7•5 years ago
|
||
bugherder |
Comment 8•5 years ago
|
||
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.
Assignee | ||
Comment 9•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
bugherder uplift |
Updated•5 years ago
|
Description
•