Crash in [@ gfxDWriteFontList::CreateFontEntry]
Categories
(Core :: Layout: Text and Fonts, defect, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox71 | --- | verified |
People
(Reporter: over68, Assigned: jfkthame)
References
Details
Crash Data
Attachments
(3 files)
Steps to reproduce:
- Set
gfx.e10s.font-list.sharedtotrue. - Restart Firefox.
- Download and install the font Pinyin1712.ttf.
- Uninstall the font
Pinyin1712.ttf. - Open the testcase.
Actual results:
The tab crashed.
Crash report: bp-c37599fc-704e-4a5b-b54b-332e20190929
Top 10 frames of crashing thread:
0 xul.dll class gfxFontEntry* gfxDWriteFontList::CreateFontEntry gfx/thebes/gfxDWriteFontList.cpp:945
1 xul.dll gfxPlatformFontList::GetOrCreateFontEntry gfx/thebes/gfxPlatformFontList.cpp:1126
2 xul.dll void gfxFontGroup::AddFamilyToFontList gfx/thebes/gfxTextRun.cpp:1815
3 xul.dll gfxFontGroup::BuildFontList gfx/thebes/gfxTextRun.cpp:1738
4 xul.dll gfxFontGroup::gfxFontGroup gfx/thebes/gfxTextRun.cpp:1696
5 xul.dll nsFontMetrics::nsFontMetrics gfx/src/nsFontMetrics.cpp:133
6 xul.dll nsFontCache::GetMetricsFor gfx/src/nsDeviceContext.cpp:178
7 xul.dll nsLayoutUtils::GetFontMetricsForFrame layout/base/nsLayoutUtils.cpp:4645
8 xul.dll BuildTextRunsScanner::BuildTextRunForFrames layout/generic/nsTextFrame.cpp:2327
9 xul.dll BuildTextRunsScanner::FlushFrames layout/generic/nsTextFrame.cpp:1640
Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=87be514024ac53ab6362ffc26610c063d50abe07&tochange=7de7d6a0be86d400ee23ca1ac806eb358555b28d
(In reply to blinky from comment #0)
- Open the testcase.
Open the page locally to reproduce the crash https://onedrive.live.com/download?cid=F96BA52A2AF70D03&resid=F96BA52A2AF70D03%211502&authkey=AINAjZLBHtv7YC8.
|
||
Comment 3•5 years ago
|
||
Bugbug thinks this bug is a regression, but please revert this change in case of error.
|
Assignee | |
Comment 4•5 years ago
|
||
(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #3)
Bugbug thinks this bug is a regression, but please revert this change in case of error.
Bugbug is confused... this isn't a regression, this is crashiness of a not-yet-enabled feature still under development.
|
|
Assignee | |
Comment 5•5 years ago
|
||
This is a weird one. What appears to be happening is that when a font is installed, we get notified (via WM_FONTCHANGE) by Windows, and in response, we rebuild the platform font list. That's fine.... now we have a font list that includes the newly-installed font. As part of rebuilding the list, we get a fresh system font collection via Factory::GetDWriteSystemFonts.
What's not so fine is what happens when a font is uninstalled. We again get notified (good), and so we again rebuild the platform font list. But - at least some of the time - when we call GetSystemFontCollection() after uninstalling a font, it may return the same font collection as it returned prior to the uninstallation. Which means we've got an mSystemFonts collection where one of the fonts (maybe more, if multiple fonts were uninstalled) is not in fact present and usable.
That seems to be benign as far as the parent process is concerned; it kinda looks like Windows may be keeping the "uninstalled" font available to processes that may already have been using it. (At least in Win10; maybe in Win7 it goes away more quickly?)
However, if a new content process is started - e.g. to load a file:// URL, as in comment 2 - it gets a system font collection that does not match the parent's. Because we use indexes into the font collection to communicate between parent and child processes, this mismatch means we may try to refer to the wrong font family, and eventually leads to crashes when we try doing DWrite font operations where we've assumed (rashly) things will not fail.
Adding some missing null-checks in the DWrite font code seems to prevent this crash, at least in my testing so far. I'm not entirely happy with the resulting behavior, as it still sometimes leads to us using the wrong font after such an uninstallation has happened, so I will try to debug that further and figure out if we can detect and correct for the mismatched font collections.
However, the missing error checks should be added in any case to reduce the fragility of the code, so let's do that here.
|
|
Assignee | |
Comment 6•5 years ago
|
||
|
|
Assignee | |
Comment 7•5 years ago
|
||
|
||
Updated•5 years ago
|
|
|
Assignee | |
Comment 8•5 years ago
|
||
Depends on D47788
|
|
Assignee | |
Comment 9•5 years ago
|
||
(In reply to Jonathan Kew (:jfkthame) from comment #5)
I'm not entirely happy with the resulting behavior, as it still sometimes leads to us using the wrong font after such an uninstallation has happened, so I will try to debug that further and figure out if we can detect and correct for the mismatched font collections.
Patch 2 addresses this by checking that the family name of the font we've found (by index) from the collection is what we expect, and if not, falling back to a (slower) by-name search. With this, I no longer see incorrect fonts sometimes being used by the local-file testcase here.
Try build is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a5ef48d7bed69e6c4f55534c228e0e44ba37b76; verifying that this fixes the problem also on Win7 would be much appreciated.
|
|
Assignee | |
Updated•5 years ago
|
|
Reporter | |
Comment 10•5 years ago
|
||
I can not reproduce the crash with the build in comment 9. Thanks.
|
||
Comment 11•5 years ago
|
||
Pushed by jkew@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fcd8b94fdd4b patch 1 - Add some missing null-checks in dwrite font code. r=lsalzman https://hg.mozilla.org/integration/autoland/rev/a7c4367d186d patch 2 - Check that the expected font family was found, and fall back to search by name in case of a font collection mismatch. r=lsalzman
|
||
Comment 12•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/fcd8b94fdd4b
https://hg.mozilla.org/mozilla-central/rev/a7c4367d186d
|
||
Updated•5 years ago
|
|
||
Comment 13•5 years ago
|
||
Description
•