Steps to reproduce:

1. Download and install the BigPartyO2Green font.
2. Open this page.
3. Installing/removing the ColorTube font several times.

See https://youtu.be/6d5B2H_21vo

Actual results:

The tab crashed.

Crash report: bp-aef1bf70-ff94-4237-ab4a-2c99f0210802

Reason: SIGSEGV /SEGV_MAPERR

Top 10 frames of crashing thread:

0 libxul.so gfxFontGroup::FindFontForChar gfx/thebes/gfxTextRun.cpp:3120
1 libxul.so void gfxFontGroup::InitScriptRun<char16_t> gfx/thebes/gfxTextRun.cpp:2704
2 libxul.so gfxFontGroup::MakeTextRun gfx/thebes/gfxTextRun.cpp:2503
3 libxul.so BuildTextRunsScanner::FlushFrames layout/generic/nsTextFrame.cpp:1661
4 libxul.so BuildTextRunsScanner::ScanFrame layout/generic/nsTextFrame.cpp:2015
5 libxul.so BuildTextRunsScanner::ScanFrame layout/generic/nsTextFrame.cpp:2069
6 libxul.so BuildTextRunsScanner::ScanFrame layout/generic/nsTextFrame.cpp:2069
7 libxul.so nsTextFrame::EnsureTextRun layout/generic/nsTextFrame.cpp:2991
8 libxul.so nsTextFrame::ReflowText layout/generic/nsTextFrame.cpp:9263
9 libxul.so nsLineLayout::ReflowFrame layout/generic/nsLineLayout.cpp:878

Set release status flags based on info from the regressing bug 1694174

Jonathan, you might be interested in poking at this shared-font-list regression? It doesn't seem super-urgent given the STR, but given it seems reproducible it might be interesting.

Did you mean to needinfo me rather than yourself...? :)

I tried to reproduce this last week, but so far didn't manage to get it to crash. I don't have any doubt it's a real issue, but the exact STR are probably dependent on various aspects of the specific system -- installed/default fonts, how timing works out across both the parent and content processes, etc -- so finding STR that work for me locally (and ideally can be captured in rr) can prove difficult.

I'll poke at it some more and see if I can get it to happen here....

Err, yes :)

I get a slightly different stack in a local build:

#0  gfxFontGroup::FindFallbackFaceForChar at gfx/thebes/gfxTextRun.cpp:2947
#1  0x00007fab73ed6b15 in gfxFontGroup::FindFallbackFaceForChar at gfx/thebes/gfxTextRun.cpp:2967
#2  0x00007fab73ed58c2 in gfxFontGroup::FindFontForChar at gfx/thebes/gfxTextRun.cpp:3120
#3  0x00007fab73eecf89 in gfxFontGroup::ComputeRanges<char16_t> at gfx/thebes/gfxTextRun.cpp:3477
#4  0x00007fab73eeba08 in gfxFontGroup::InitScriptRun<char16_t>

aFamily is:

$4 = {
  {
    mOwnedFamily = 0x7fab6391f1d8,
    mSharedFamily = 0x7fab6391f1d8
  },
  {
    mFont = 0x7fab6500c640,
    mFontEntry = 0x7fab6500c640,
    mSharedFace = 0x7fab6500c640
  },
  mGeneric = mozilla::StyleGenericFontFamily::None,
  mFontCreated = true,
  mLoading = false,
  mInvalid = false,
  mCheckForFallbackFaces = false,
  mIsSharedFamily = true,
  mHasFontEntry = false
}

and it appears that mSharedFamily is pointing to an unmapped memory region:

(rr) p $4.mSharedFamily
$5 = (mozilla::fontlist::Family *) 0x7fab6391f1d8
(rr) p *$4.mSharedFamily
Cannot access memory at address 0x7fab6391f1d8

Changing severity to S2 and flagging as security-sensitive because it looks like we're accessing random memory, per the crash report in comment 0 (Crash Address 0x00007f32ed03a0f4) and comment 5.

re-upping the jfkthame needinfo -- in comment 3 you were unable to reproduce, though maybe you can chat with mats to find out what worked for him to make it repro on his end (in comment 5)?

The fix that Emilio has just landed in bug 1730456 may help here; let's see if we can still reproduce this, or confirm the fix.

This bug has been fixed in bug 1722487.

Pushlog for fix range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=951d5d88fe92f14213b100760808e1b1cae09b62&tochange=5235f051d95235e351d59f61d5ce19cf0cb9d3f7

blinky: thanks for your testing/confirmation - very much appreciated.

What should we do about ESR here?

301 Jonathan, if he doesn't mind, as I don't know of the top of my head the status of the shared font list on ESR.

It's been on-by-default since 89, so that means it's enabled on esr91.

Although blinky points to bug 1722487 in comment 8, I suspect that just affected timing such that the exact testcase no longer crashed for them, but it was not the fundamental fix. I believe the most important fix referred to here is actually bug 1730456, which addressed a potential use-after-free in scenarios like this where the font-list is updated during the session.

Uplifting both bug 1730456 and bug 1722487 would make sense, IMO, but especially 1730456.

Jonathan, do you want to request uplift to 93 in bug 1730456? I think that this buf being a sec-moderate, uplifting to ESR can wait next cycle. Thanks

Yeah, it looks like the supporting patches will need some rebasing for ESR91, so let's defer that until the next cycle to avoid putting the 91.2esr RC build at risk.

I'm changing the flags for this one.
Bug 1730456 is the real fix and it's in 94 but not 93.
Bug 1722487 is a masking fix and it's in 93.

So I'm saying it's fixed in 94 but not 93.

I took a look at the ESR91 rebase today but ended up in a position of also needing to take bug 1718755 to make things work smoothly. That bug has a number of other dependencies that would make the uplift more complicated. After discussing with Emilio and Jonathan, we felt that the safer option is to pref off shared font list on ESR91 since it's not as important there anyway.

Comment on attachment 9245625 [details]
Bug 1723468 - Disable shared font list on ESR91. r=jfkthame

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: The primary motivation for the shared font-list work is to reduce the cost (per-process memory use and startup) of Fission. So as long as we're not shipping Fission on ESR, there's no compelling reason to enable it there. Given that the shared font list is still a little raw on the edges, disabling it avoids exposing ESR users to the added risk.
• User impact if declined: Possible crashes, particularly associated with installing/removing fonts while Firefox is running.
• Fix Landed on Version: n/a
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just turns off the new implementation and reverts to longstanding code. Some possible risk in that the old code paths are no longer being regularly tested on trunk; but they will still be exercised by CI tests on ESR so any breakage should be visible there.
• String or UUID changes made by this patch:

Comment on attachment 9245625 [details]
Bug 1723468 - Disable shared font list on ESR91. r=jfkthame

Approved for 91.3esr.

https://hg.mozilla.org/releases/mozilla-esr91/rev/6b6cd7f6837b

Although this is memory corruption, it's probably not a vulnerability if a user has to installing their own fonts to trigger this. The mainline non-ESR version of this was not flagged as a sec bug.