1580690 - Crash in [@ gfxFontEntry::HasCharacter]

Status: RESOLVED FIXED

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

Description

[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 https://www.gftrails.ca/contact.html.
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:

The tab crashed.

Crash report: bp-aa701a87-d74e-40c9-a7c2-0d81c0190912

Top 10 frames of crashing thread:

0 xul.dll gfxFontEntry::HasCharacter gfx/thebes/gfxFontEntry.h:214
1 xul.dll gfxFontGroup::ComputeRanges<unsigned char> gfx/thebes/gfxTextRun.cpp:3077
2 xul.dll gfxFontGroup::InitScriptRun<unsigned char> gfx/thebes/gfxTextRun.cpp:2486
3 xul.dll gfxFontGroup::InitTextRun<unsigned char> gfx/thebes/gfxTextRun.cpp:2362
4 xul.dll gfxFontGroup::MakeTextRun gfx/thebes/gfxTextRun.cpp:2252
5 xul.dll BuildTextRunsScanner::BuildTextRunForFrames layout/generic/nsTextFrame.cpp:2499
6 xul.dll BuildTextRunsScanner::FlushFrames layout/generic/nsTextFrame.cpp:1640
7 xul.dll nsTextFrame::EnsureTextRun layout/generic/nsTextFrame.cpp:2937
8 xul.dll nsTextFrame::ReflowText layout/generic/nsTextFrame.cpp:9152
9 xul.dll nsLineLayout::ReflowFrame layout/generic/nsLineLayout.cpp:880

Comment 1

[blinky]

Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c24c083425d0e543bd59076d675c2efcb3b7c73c&tochange=81ac850f683c3e895886bbcae79a3ad285f6a56f

Regressed by: bug 1545177

Comment 2

[Jonathan Kew [:jfkthame]]

Looking into this - I suspect it may have the same cause as one of the other shared-font-list crashes that's currently being addressed.

Comment 3

[Jonathan Kew [:jfkthame]]

blinky: Does this still crash for you with the latest Nightly, since recent font-list fixes landed? I'm not able to reproduce this crash on my Win10 machine at the moment.

Comment 4

[blinky]

I can reproduce with the latest Nightly build.

bp-dcfb8796-1b00-4353-a61c-5bbe20190915
bp-f48ad1ec-9268-4eea-99fa-0b3620190915

Comment 5

[blinky]

It can be reproduced in https://graphemica.com/unicode/characters with the steps in comment 0.

Note the crash occurs because of the G character which is on the left side at the top of the page.

Comment 6

[blinky]

I can also reproduce on https://bugzilla.mozilla.org/attachment.cgi?id=594419 with the steps in comment 0.

Comment 7

[Jonathan Kew [:jfkthame]]

Aha, with the bugzilla attachment 594419 [details], this finally reproduced for me as well -- thanks! (The graphemica.com site doesn't reproduce it for me. I expect differences in the system font configuration are probably a factor.)

Comment 8

[Jonathan Kew [:jfkthame]]

Do you have the Montserrat Bold font (or the whole Montserrat family) installed locally on your machine? The crash I'm seeing seems to be related to src:local(...) rules, and if you have Montserrat installed, that would explain why you see it on the sites in comment 0 and comment 5.

Comment 9

[blinky]

Yes, the Montserrat font family are locally installed.

Comment 10

[Jonathan Kew [:jfkthame]]

OK, I think I understand what's happening here. When a new font is installed (whether manually, or via Font Loader or similar), we discard the existing shared font list and build a new one. Therefore, we have to flush the various font caches, because all the existing font entries, character maps, etc., will be invalid.

(In the non-shared font list case, old font entries would most likely continue to work, although they might not precisely reflect the fonts that are supposed to be available to the browser after the system configuration change; however, in the shared font list case, the font entries and character maps include pointers into the shmem blocks, which will have been unmapped during the font list refresh, so trying to use them is likely to crash.)

We're mostly handling this OK, but missed a case hidden in the user font set. We don't want to throw away all downloaded font resources in response to an installed font change; but the user font set may also have entries that were loaded via src:local("name"). These refer to platform font entries, and those entries are no longer valid after a platform font list refresh. So we need to discard them and re-do the src:local() lookups to get fresh font entries.

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1580690 - Ensure src:local() entries in the user font set are refreshed if the platform font list is rebuilt. r?jwatt

Comment 12

[Jonathan Kew [:jfkthame]]

blinky, I've started a tryserver run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=129a971e8102f622e749c3297785ccafa05c2c3a with this patch. When that build is ready, if you're able to test and confirm whether it resolves this crashes for you (in my local testing, I can no longer reproduce this with the patch applied), that would be great. Thanks!

Comment 13

[blinky]

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ed7c109c239
Ensure src:local() entries in the user font set are refreshed if the platform font list is rebuilt. r=jwatt

Comment 15

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/8ed7c109c239