1567752 - Crash in [@ mozilla::dom::FontFaceSet::OnFontFaceStatusChanged]

Status: RESOLVED FIXED

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

Description

[[:philipp]]

This bug is for crash report bp-a32fa47d-5cf9-418a-b07c-a9b9b0190720.

Top 10 frames of crashing thread:

0 xul.dll void mozilla::dom::FontFaceSet::OnFontFaceStatusChanged layout/style/FontFaceSet.cpp:1454
1 xul.dll mozilla::dom::FontFace::SetStatus layout/style/FontFace.cpp:428
2 xul.dll void mozilla::dom::FontFace::Entry::SetLoadState layout/style/FontFace.cpp:801
3 xul.dll bool gfxUserFontEntry::LoadPlatformFont gfx/thebes/gfxUserFontSet.cpp:838
4 xul.dll void gfxUserFontEntry::ContinuePlatformFontLoadOnMainThread gfx/thebes/gfxUserFontSet.cpp:955
5 xul.dll nsresult mozilla::detail::RunnableMethodImpl<gfxUserFontEntry*, void  xpcom/threads/nsThreadUtils.h:1176
6 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1225
7 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:486
8 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:88
9 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:308

this crash signature (mostly in the content process) is reappearing during firefox 69.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

I think this is a regression from bug 1490792, though it may be exposing a pre-existing issue. In particular, when unlinking the font face, we remove the font-face-set pointer: https://searchfox.org/mozilla-central/rev/b9041f813de0a05bf6de95a145d4e25004499517/layout/style/FontFace.cpp#84

But we don't modify mInFontFaceSet, which may be an issue.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1567752 - Keep mInFontFaceset and mFontFaceSet in sync. r=heycam,jfkthame

When we unlink, we may null out the mFontFaceSet pointer, but still keep
mInFontFaceset as true. Which can cause null dereference bugs as described in
comment 0.

This is mostly a speculative fix since I think that's a plausible cause for a
bug like that, but it may not be the only cause for the crash.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7abe3bfd993
Keep mInFontFaceset and mFontFaceSet in sync. r=heycam

Comment 4

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/d7abe3bfd993

Comment 5

[[:philipp]]

thank you for the blazingly fast fix. could you nominate the patch for beta approval if it's applicable and you deem fit to do so?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9079626 [details]
Bug 1567752 - Keep mInFontFaceset and mFontFaceSet in sync. r=heycam,jfkthame

Beta/Release Uplift Approval Request

• User impact if declined: Crashes.
• Is this code covered by automated tests?: Yes
• 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): Simple fix to state getting out of sync that is exposed by a change in how we do font sanitization.
• String changes made/needed: none

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9079626 [details]
Bug 1567752 - Keep mInFontFaceset and mFontFaceSet in sync. r=heycam,jfkthame

Approved for 69.0b9 so we can see if the patch helps there too. Thanks for the patch!

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5cfd6b2055a3