Closed Bug 1567752 Opened 6 months ago Closed 6 months ago

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

Categories

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

69 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox68 --- unaffected
firefox69 --- fixed
firefox70 --- fixed

People

(Reporter: philipp, Assigned: emilio)

References

(Regression)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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.

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.

Regressed by: 1490792

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.

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d7abe3bfd993
Keep mInFontFaceset and mFontFaceSet in sync. r=heycam
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Assignee: nobody → emilio

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?

Flags: needinfo?(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
Flags: needinfo?(emilio)
Attachment #9079626 - Flags: approval-mozilla-beta?

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!

Attachment #9079626 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.