1811950 - crash at null in [@ Destroy]

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Found while fuzzing m-c 20221124-e12f31999d33 (--enable-address-sanitizer --enable-fuzzing)

To reproduce via Grizzly Replay:

$ pip install fuzzfetch grizzly-framework
$ python -m fuzzfetch -a --fuzzing -n firefox
$ python -m grizzly.replay ./firefox/firefox testcase.html

==171659==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f56886b317c bp 0x7f565fb2dfd0 sp 0x7f565fb2dfb0 T25)
==171659==The signal is caused by a READ memory access.
==171659==Hint: address points to the zero page.
    #0 0x7f56886b317c in Destroy /builds/worker/checkouts/gecko/layout/style/FontFaceSet.cpp:143:38
    #1 0x7f56886b317c in mozilla::dom::FontFaceSet::~FontFaceSet() /builds/worker/checkouts/gecko/layout/style/FontFaceSet.cpp:107:3
    #2 0x7f56886b331d in mozilla::dom::FontFaceSet::~FontFaceSet() /builds/worker/checkouts/gecko/layout/style/FontFaceSet.cpp:102:29
    #3 0x7f567f57e492 in SnowWhiteKiller::~SnowWhiteKiller() /builds/worker/checkouts/gecko/xpcom/base/nsCycleCollector.cpp:2431:7
    #4 0x7f567f57da2e in nsCycleCollector::FreeSnowWhite(bool) /builds/worker/checkouts/gecko/xpcom/base/nsCycleCollector.cpp:2621:3
    #5 0x7f567f585866 in nsCycleCollector::BeginCollection(mozilla::CCReason, ccIsManual, nsICycleCollectorListener*) /builds/worker/checkouts/gecko/xpcom/base/nsCycleCollector.cpp:3610:3
    #6 0x7f567f584e91 in nsCycleCollector::Collect(mozilla::CCReason, ccIsManual, js::SliceBudget&, nsICycleCollectorListener*, bool) /builds/worker/checkouts/gecko/xpcom/base/nsCycleCollector.cpp:3437:9
    #7 0x7f567f58853e in nsCycleCollector_collect(mozilla::CCReason, nsICycleCollectorListener*) /builds/worker/checkouts/gecko/xpcom/base/nsCycleCollector.cpp:3945:28
    #8 0x7f56878ba0cd in mozilla::dom::workerinternals::(anonymous namespace)::WorkerJSRuntime::CustomGCCallback(JSGCStatus) /builds/worker/checkouts/gecko/dom/workers/RuntimeService.cpp:817:11
    #9 0x7f567f53df36 in mozilla::CycleCollectedJSRuntime::OnGC(JSContext*, JSGCStatus, JS::GCReason) /builds/worker/checkouts/gecko/xpcom/base/CycleCollectedJSRuntime.cpp:1884:3
    #10 0x7f568e20cbed in js::gc::GCRuntime::maybeCallGCCallback(JSGCStatus, JS::GCReason) /builds/worker/checkouts/gecko/js/src/gc/GC.cpp:4046:3
    #11 0x7f568e20dd1d in ~AutoCallGCCallbacks /builds/worker/checkouts/gecko/js/src/gc/GC.cpp:4019:32
    #12 0x7f568e20dd1d in js::gc::GCRuntime::gcCycle(bool, js::SliceBudget const&, JS::GCReason) /builds/worker/checkouts/gecko/js/src/gc/GC.cpp:4136:1
    #13 0x7f568e20f3e0 in js::gc::GCRuntime::collect(bool, js::SliceBudget const&, JS::GCReason) /builds/worker/checkouts/gecko/js/src/gc/GC.cpp:4318:9
    #14 0x7f568e1d75bb in js::gc::GCRuntime::gc(JS::GCOptions, JS::GCReason) /builds/worker/checkouts/gecko/js/src/gc/GC.cpp:4395:3
    #15 0x7f56878b8b99 in mozilla::dom::workerinternals::(anonymous namespace)::WorkerThreadPrimaryRunnable::Run() /builds/worker/checkouts/gecko/dom/workers/RuntimeService.cpp:2090:9
    #16 0x7f567f75e58b in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1191:16
    #17 0x7f567f768064 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:477:10
    #18 0x7f5680ede064 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:300:20
    #19 0x7f5680d5bf57 in RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:381:10
    #20 0x7f5680d5bf57 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:374:3
    #21 0x7f5680d5bf57 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:356:3
    #22 0x7f567f756065 in nsThread::ThreadFunc(void*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:383:10
    #23 0x7f56a17eb628 in _pt_root /builds/worker/checkouts/gecko/nsprpub/pr/src/pthreads/ptthread.c:201:5
    #24 0x7f56a1f6ab42 in start_thread nptl/pthread_create.c:442:8
    #25 0x7f56a1ffc9ff  misc/../sysdeps/unix/sysv/linux/x86_64/clone3.S:81

Comment 1

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20230123233224-5b25444c8911.
The bug appears to have been introduced in the following build range:

Start: 14bbdad41ca8f9cbe874b8e1adf10a701ff81517 (20220711154729)
End: 0fa881dedc30efcf53f1419737a9f3d7d2fc5521 (20220711143742)
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=14bbdad41ca8f9cbe874b8e1adf10a701ff81517&tochange=0fa881dedc30efcf53f1419737a9f3d7d2fc5521

Comment 2

[Tyson Smith [:tsmith]]

A Pernosco session is available here: https://pernos.co/debug/1h1aAqK5uanrWuA21t2stg/index.html

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

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

:aosmond, since you are the author of the regressor, bug 1746110, could you take a look? Also, could you set the severity field?

For more information, please visit auto_nag documentation.

Comment 4

[Daniel Holbert [:dholbert]]

In Pernosco, it looks like this FontFaceSet was created via this codepath:
https://searchfox.org/mozilla-central/rev/7a9e3bbab8f81c2cbc72a394047f948da9cfef9a/layout/style/FontFaceSet.cpp#127-136

/* static */ already_AddRefed<FontFaceSet> FontFaceSet::CreateForWorker(
    nsIGlobalObject* aParent, WorkerPrivate* aWorkerPrivate) {
  RefPtr<FontFaceSet> set = new FontFaceSet(aParent);
  RefPtr<FontFaceSetWorkerImpl> impl = new FontFaceSetWorkerImpl(set);
  if (NS_WARN_IF(!impl->Initialize(aWorkerPrivate))) {
    return nullptr;
  }
  set->mImpl = std::move(impl);
  return set.forget();
}

And we're taking that return nullptr case (impl->Initialize() is failing) which means we never reach the assignment for set->mImpl. So mImpl remains nullptr, which is a problem because the destructor unconditionally dereferences it.

Why is impl->Initialize() failing? It's because internally our WorkerPrivate has mStatus set to Canceling, which it got set to earlier via a call to CancelWorkersForWindow. Presumably that's associated with the fact that the testcase is reloading itself.

Here's the backtrace where this FontFaceSet code ends up stumbling over our WorkerPrivate::mStatus of Canceling and hit the return false statement in WorkerPrivate::AddWorkerRef:

#0  mozilla::dom::WorkerPrivate::AddWorkerRef (this=0x61b0000c8680, aWorkerRef=0x607000094050, aFailStatus=mozilla::dom::Canceling) at /builds/worker/checkouts/gecko/dom/workers/WorkerPrivate.cpp:4040
#1  0x00007baff414fedc in mozilla::dom::WorkerRef::HoldWorker (this=0x607000094050, aStatus=mozilla::dom::Canceling) at /builds/worker/checkouts/gecko/dom/workers/WorkerRef.cpp:79
#2  0x00007baff4150860 in mozilla::dom::StrongWorkerRef::CreateImpl (aWorkerPrivate=0x61b0000c8680, aName=0x7bafdd8617c0 <str> "FontFaceSetWorkerImpl", aFailStatus=mozilla::dom::Canceling) at /builds/worker/checkouts/gecko/dom/workers/WorkerRef.cpp:174
#3  0x00007baff4127af7 in mozilla::dom::StrongWorkerRef::Create(mozilla::dom::WorkerPrivate*, char const*, fu2::abi_400::detail::function<fu2::abi_400::detail::config<true, false, fu2::capacity_fixed<16ul, 8ul> >, fu2::abi_400::detail::property<false, false, void ()> >&&) (aWorkerPrivate=0x61b0000c8680, aName=0x7bafdd8617c0 <str> "FontFaceSetWorkerImpl", aCallback=...) at /builds/worker/checkouts/gecko/dom/workers/WorkerRef.cpp:153
#4  0x00007baff574bd16 in mozilla::dom::FontFaceSetWorkerImpl::Initialize (this=0x6120001bcdc0, aWorkerPrivate=0x61b0000c8680) at /builds/worker/checkouts/gecko/layout/style/FontFaceSetWorkerImpl.cpp:40
#5  0x00007baff5714fa8 in mozilla::dom::FontFaceSet::CreateForWorker (aParent=0x6140001002c8, aWorkerPrivate=0x61b0000c8680) at /builds/worker/checkouts/gecko/layout/style/FontFaceSet.cpp:131

Probably the simplest fix is to just make the destructor gracefully handle a null mImpl pointer.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The component has been changed since the backlog priority was decided, so we're resetting it.
For more information, please visit auto_nag documentation.

Comment 6

[Daniel Holbert [:dholbert]]

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

Probably the simplest fix is to just make the destructor gracefully handle a null mImpl pointer.

All sorts of other code assumes mImpl is non-null too, BTW. We can pretty much guarantee that none of those APIs will be called here since we're immediately dropping the sole ref -- but the cycle collector does hold a persistent reference for a little while (via setting the NS_IS_PURPLE flag), so we need to be sure the cycle collection APIs (e.g. traverse) can handle a null mImpl value as well.

One other approach that might work: maybe we could instantiate & initialize the FontFaceSetWorkerImpl (and handle failure) before we instantiate the FontFaceSet object -- that way, we could indeed guarantee that a FontFaceSet always has a non-null mImpl pointer. There's a bit of a chicken-and-egg problem since the FontFaceSetWorkerImpl constructor wants a pointer to the FontFaceSet object, but at first glance I'm not sure it actually needs it?

Comment 7

[Daniel Holbert [:dholbert]]

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

One other approach that might work: maybe we could instantiate & initialize the FontFaceSetWorkerImpl (and handle failure) before we instantiate the FontFaceSet object -- that way, we could indeed guarantee that a FontFaceSet always has a non-null mImpl pointer. There's a bit of a chicken-and-egg problem since the FontFaceSetWorkerImpl constructor wants a pointer to the FontFaceSet object, but at first glance I'm not sure it actually needs it?

I started poking at this^ approach and didn't end up liking it; it essentially would mean that FontFaceSetImpl and its subclasses would have a more complex-to-reason-about 3-stage initialization process ((1) constructor, (2) subclass-specific & potentially-fallible initialization, (3) a SetOwner() step to inform them of their FontFaceSet owner). This ends up feeling a bit more delicate than the original approach that I mentioned.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: Bug 1811950: Make FontFaceSet's destruction and cycle-collection code robust against a null mImpl pointer. r?aosmond

Comment 9

[Daniel Holbert [:dholbert]]

oh hmm, even with that fix we still trip a fatal assertion:

Assertion failure: mFontFaceSet, at dom/workers/WorkerScope.cpp:519

For this code:https://searchfox.org/mozilla-central/rev/7a9e3bbab8f81c2cbc72a394047f948da9cfef9a/dom/workers/WorkerScope.cpp#514-519

FontFaceSet* WorkerGlobalScope::Fonts() {
  AssertIsOnWorkerThread();

  if (!mFontFaceSet) {
    mFontFaceSet = FontFaceSet::CreateForWorker(this, mWorkerPrivate);
    MOZ_ASSERT(mFontFaceSet);

This^ code seems not-cool with the possibility that CreateForWorker might fail and return nullptr (via the explicit return nullptr statement that I quited in comment 4).

So: it seems we need a bit more thorough invariant-massaging here, to get everything to be coherent. I'll punt back to aosmond; hopefully he can take a look here.

--> Unassigning and abandoning patch, but feel free to use it as inspiration. :)

Comment 10

[Daniel Holbert [:dholbert]]

Adding dependency on bug 1072107 which is where this code was added -- in particular, "part 8" there had CreateForWorker with the return nullptr early-return (the code that I quoted in comment 4):
https://hg.mozilla.org/mozilla-central/rev/d45228a652b44ada5ca749bd36e8b0399153302e#l1.30

...and "part 9" there added a callsite which asserts that CreateForWorker never returns null (the code that I quoted in comment 9) even though it does in fact sometimes return null:
https://hg.mozilla.org/mozilla-central/rev/7eecad7fdee1d234f2d4d77e4b7cedfbe62ed7c6#l6.64

Those need to be harmonized somehow, in addition to making FontFaceSet safely-destructable-with-null-mImpl.

(I'll also revert my naive reclassification to put this under Layout:Text&Fonts, too -- it looks like CSS is the appropriate component, since that matches bug 1072107 where all this code was added. Sorry for the churn.)

Comment 11

[Peter Van der Beken [:peterv]]

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

For this code:https://searchfox.org/mozilla-central/rev/7a9e3bbab8f81c2cbc72a394047f948da9cfef9a/dom/workers/WorkerScope.cpp#514-519

FontFaceSet* WorkerGlobalScope::Fonts() {
  AssertIsOnWorkerThread();

  if (!mFontFaceSet) {
    mFontFaceSet = FontFaceSet::CreateForWorker(this, mWorkerPrivate);
    MOZ_ASSERT(mFontFaceSet);

This^ code seems not-cool with the possibility that CreateForWorker might fail and return nullptr (via the explicit return nullptr statement that I quited in comment 4).

readonly attribute FontFaceSet fonts; (https://searchfox.org/mozilla-central/rev/8e9b4484408154b80d7ede9e1b035819fda48fd2/dom/webidl/FontFaceSource.webidl#16) specifically states that this getter will always return a non-null object. It should probably have been marked as [Throws] so that it can return an error if needed.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1811950 - Allow WorkerGlobalScope.fonts to fail. r=aosmond,jfkthame,#webidl

It can fail when canceling the worker (though I couldn't reproduce the
crash locally). Some things were already accounting for it.

Rename some things for consistency.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1811950 - Test. r=saschanaz

Had to do some frame shenanigans to get it to semi-consistently repro.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/41a7057fb4ee
Allow WorkerGlobalScope.fonts to fail. r=aosmond,webidl,saschanaz

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e216de6bed65
Test. r=saschanaz

Comment 17

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/39479 for changes under testing/web-platform/tests

Comment 18

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/41a7057fb4ee
https://hg.mozilla.org/mozilla-central/rev/e216de6bed65

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit auto_nag documentation.

Comment 20

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 21

[Bugmon [:jkratzer for issues]]

Verified bug as fixed on rev mozilla-central 20230411210653-492ab34039ea.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:emilio, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox113 to wontfix.

For more information, please visit auto_nag documentation.

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9327919 [details]
Bug 1811950 - Allow WorkerGlobalScope.fonts to fail. r=aosmond,jfkthame,#webidl

Beta/Release Uplift Approval Request

• User impact if declined: Fixes a probably rather rare crash on navigation, but fix is low risk.
• 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): Relatively simple patch.
• String changes made/needed: none
• Is Android affected?: Yes

Comment 24

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9327919 [details]
Bug 1811950 - Allow WorkerGlobalScope.fonts to fail. r=aosmond,jfkthame,#webidl

Approved for 113 beta 3, thanks.

Comment 25

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/85b6fc827f70
https://hg.mozilla.org/releases/mozilla-beta/rev/d783ff8b22ad