Closed Bug 1522416 Opened 5 years ago Closed 5 years ago

Crash in mozilla::dom::FontFace::Entry::GetUserFontSets

Categories

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

66 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- unaffected
firefox65 --- unaffected
firefox66 --- fixed
firefox67 --- fixed

People

(Reporter: philipp, Unassigned)

References

Details

(Keywords: crash, regression)

Crash Data

This bug is for crash report bp-684c5f60-faf8-4b41-afb5-e64360190124.

Top 7 frames of crashing thread:

0 libxul.so mozilla::dom::FontFace::Entry::GetUserFontSets mfbt/RefPtr.h:267
1 libxul.so nsFontFaceLoader::LoadTimerCallback layout/style/nsFontFaceLoader.cpp:171
2 libxul.so nsTimerEvent::Run xpcom/threads/nsTimerImpl.cpp:559
3 libxul.so nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1160
4 libxul.so NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:468
5 libxul.so nsThreadManager::SpinEventLoopUntilInternal xpcom/threads/nsThreadUtils.h:335
6 libxul.so _NS_InvokeByIndex 

crashes with this signature are starting to show up since 66.0a1 build 20190122094123 and they are more common on fennec.cthe changes of bug 1519918 are in the pushlog for that build, could they be related?

Flags: needinfo?(emilio)

Hmm, I don't understand how this crash can happen... The only way GetUserFontSets can crash on a null deref is with a null this pointer, which would make sense, except in LoadTimerCallback we've already dereferenced it a bunch of times...

Tyson, do you or somebody from your team happen to have any test-case that could resemble this signature?

Flags: needinfo?(emilio) → needinfo?(twsmith)

I don't see anything that looks similar at the moment, neither does jkratzer. We will be sure to update this bug if we find something.

Flags: needinfo?(twsmith)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #1)

Hmm, I don't understand how this crash can happen... The only way GetUserFontSets can crash on a null deref is with a null this pointer, which would make sense, except in LoadTimerCallback we've already dereferenced it a bunch of times...

If I had to guess, I think it may actually be at the loader->mUserFontEntry.get() call[1] that we're crashing, but optimization reordering somehow ends up causing the stack to blame the later GetUserFontSets call. The loader's RefPtr<gfxUserFontEntry> mUserFontEntry member is the thing that seems to involve RefPtr in the execution of this code.

Very possibly unrelated, but we have also recently had a couple of nsFontFaceLoader crashes reported recently[2].

Tyson: any testcases with crashes where nsFontFaceLoader members such as the member are null?

  1. https://hg.mozilla.org/mozilla-central/annotate/c60e6c0c2e2318295d1bec96e95ec7eaa9b48087/layout/style/nsFontFaceLoader.cpp#l104

  2. https://crash-stats.mozilla.com/signature/?signature=nsFontFaceLoader%3A%3ALoadTimerCallback#reports

Flags: needinfo?(twsmith)

I don't see any that match that description either. I have checked our code coverage reports and only our css variable-font fuzzer hits this code path atm. I have added some basic support to another more general Layout/DOM fuzzer we have as well. We'll see if that shakes anything out.

:jwatt, :emilio do you have any suggested code patterns or example test cases that might help exercise this code in different ways? I might be able to add those ideas to the fuzzer as well.

:jkratzer Is this something you can add to Dominode?

Flags: needinfo?(twsmith)
Flags: needinfo?(jwatt)
Flags: needinfo?(jkratzer)
Flags: needinfo?(emilio)

Adding valid / invalid / slow @font-face rules should trigger different related codepaths. It's a bit trickier than that because in order to trigger the load you need to have some text on the page using that font, so you need to make sure that happens. Alternatively / additionally using the document.fonts API should also help here, with the same caveat (IIRC).

Flags: needinfo?(emilio)

I'm working on implementing this into Dominode. I should have it running within the next week.

Flags: needinfo?(jkratzer)

the issue looks fixed in 66.0b6.

Status: NEW → RESOLVED
Closed: 5 years ago
Depends on: 1524246
Resolution: --- → FIXED
Flags: needinfo?(jwatt)
You need to log in before you can comment on or make changes to this bug.