1777301 - Crash in [@ mozilla::dom::Document::UnblockOnload]

Status: RESOLVED FIXED

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

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

5 crashes from 5 different installations (both Windows and macOS), all with Firefox 104.0a1 20220629070023

Crash report: https://crash-stats.mozilla.org/report/index/2e35c08e-51ce-4b00-96fc-2825c0220629

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::Document::UnblockOnload dom/base/Document.cpp:11586
1 xul.dll nsFontFaceLoader::Cancel layout/style/nsFontFaceLoader.cpp:347
2 xul.dll mozilla::dom::FontFaceSetImpl::Destroy layout/style/FontFaceSetImpl.cpp:87
3 xul.dll mozilla::dom::FontFaceSetDocumentImpl::Destroy layout/style/FontFaceSetDocumentImpl.cpp:100
4 xul.dll mozilla::dom::FontFaceSet::cycleCollection::Unlink layout/style/FontFaceSet.cpp:81
5 xul.dll nsCycleCollector::Collect xpcom/base/nsCycleCollector.cpp:3440
6 xul.dll nsCycleCollector_collectSlice xpcom/base/nsCycleCollector.cpp:3934
7 xul.dll static mozilla::CCGCScheduler::CCRunnerFired dom/base/nsJSEnvironment.cpp:1609
8 xul.dll virtual bool std::_Func_impl_no_alloc<bool  
9 xul.dll mozilla::IdleTaskRunner::Run xpcom/threads/IdleTaskRunner.cpp:125

Comment 1

[Daniel Holbert [:dholbert]]

Note, Document::UnblockOnload has a handful of callers, so there are some older crashes with this same signature which seem to be unrelated. This bug here is focused on the recent crash reports where nsFontFaceLoader::Cancel is the caller.

For those ones, we're crashing with a null deref on this line:

void Document::UnblockOnload(bool aFireSync) {
  if (mDisplayDocument) {

...implying that this itself is nullptr.

So, going up one level, to nsFontFaceLoader::Cancel(), we have this line:

  mFontFaceSet->Document()->UnblockOnload(false);

...and it looks like mFontFaceSet->Document() must be returning nullptr there.

That Document() API is just a getter for the FontFaceSetDocumentImpl::mDocument pointer, which is set to nullptr here:

void FontFaceSetDocumentImpl::Destroy() {
[...]
  mDocument = nullptr;

https://searchfox.org/mozilla-central/rev/2946e9b450cb35afaf8dad927a8d187975dcd74d/layout/style/FontFaceSetDocumentImpl.cpp#98

So presumably the issue here is that our nsFontFaceLoader's mFontFaceSet has had its Destroy() method already called, which invalidates it (or at least makes its Document() API surprisingly null-returning), and then we trip over that in nsFontFaceLoader::Cancel.

I'm not familiar with this code, so take my thoughts with a grain of salt, but presumably we need to either make sure that mFontFaceSet::Destroy() invalidates the nsFontFaceLoader as well (e.g. disconnecting itself); or maybe we need to make nsFontFaceLoader::Cancel null-check the return value of the Document() function-call here, though that's a little silly given the current infallible naming of that function.

Comment 2

[Daniel Holbert [:dholbert]]

It looks like the void FontFaceSetDocumentImpl::Destroy() was added (renamed/broadened from an older Disconnect method) here:
https://hg.mozilla.org/mozilla-central/rev/3d30a8d83d77cfb25c9d75aae2daa01051810ff0#l9.143

...and the mDocument = nullptr; is newly added in that commit.

So indeed, this is a regression from that commit (as aryx already flagged).

(In reply to Daniel Holbert [:dholbert] from comment #1)

I'm not familiar with this code, so take my thoughts with a grain of salt, but presumably we need to either make sure that mFontFaceSet::Destroy() invalidates the nsFontFaceLoader as well (e.g. disconnecting itself)

This seems like it might be feasible, since it looks like we have a mLoaders member-var which we could iterate over and detach ourselves from somehow?

Anyway, I'm hoping/guessing aosmond will know what to do here. :)

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

Huh I can probably just fix the ordering in the Destroy.

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1777301 - Fix crash in FontFaceSetDocumentImpl::Destroy due to clearing document reference too early.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f9d68808b49b
Fix crash in FontFaceSetDocumentImpl::Destroy due to clearing document reference too early. r=jfkthame

Comment 6

[Sandor Molnar[:smolnar]]

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