1833790 - Crash in [@ mozilla::PerformanceCounter::GetTotalDispatchCount]

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect, P3)

Description

[Ryan VanderMeulen [:RyanVM]]

Crash report: https://crash-stats.mozilla.org/report/index/5563b6f2-ae79-4c83-9377-c9be40230517

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  xul.dll  std::_Load_seq_cst_8  /builds/worker/fetches/vs/VC/Tools/MSVC/14.16.27023/include/xatomic.h:1839
0  xul.dll  std::_Atomic_load_8  /builds/worker/fetches/vs/VC/Tools/MSVC/14.16.27023/include/xatomic.h:1898
0  xul.dll  std::atomic_load_explicit  /builds/worker/fetches/vs/VC/Tools/MSVC/14.16.27023/include/xxatomic:495
0  xul.dll  std::_Atomic_ullong::load const  /builds/worker/fetches/vs/VC/Tools/MSVC/14.16.27023/include/xxatomic:629
0  xul.dll  mozilla::detail::IntrinsicMemoryOps<unsigned long long, 2>::load  mfbt/Atomics.h:195
0  xul.dll  mozilla::detail::AtomicBaseIncDec<unsigned long long, 2>::operator unsigned long long const  mfbt/Atomics.h:340
0  xul.dll  mozilla::PerformanceCounter::GetTotalDispatchCount const  xpcom/threads/PerformanceCounter.cpp:65
1  xul.dll  mozilla::dom::FetchStreamReader::Create::<lambda_16>::operator const  dom/fetch/FetchStreamReader.cpp:80
1  xul.dll  fu2::abi_400::detail::invocation::invoke  third_party/function2/include/function2/function2.hpp:222
1  xul.dll  fu2::abi_400::detail::type_erasure::invocation_table::function_trait<void   third_party/function2/include/function2/function2.hpp:628

Comment 1

[Valentin Gosu [:valentin]]

As far as I can tell, this is a null pointer deref:

https://searchfox.org/mozilla-central/rev/a1036b237f2c132ac990c715ae8d20f441657113/toolkit/components/perfmonitoring/PerformanceUtils.cpp#37-38

const RefPtr<WorkerDebugger> debugger = wdm->GetDebuggerAt(i);
promises.AppendElement(debugger->ReportPerformanceInfo());

Either debugger is null here.

https://searchfox.org/mozilla-central/rev/a1036b237f2c132ac990c715ae8d20f441657113/dom/workers/WorkerDebugger.cpp#538-541

const auto& perf = mWorkerPrivate->PerformanceCounterRef();
uint64_t perfId = perf.GetID();
uint16_t count = perf.GetTotalDispatchCount();
uint64_t duration = perf.GetExecutionDuration();

Or mWorkerPrivate is null here.

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

I asked in https://chat.mozilla.org/#/room/#crashreporting:mozilla.org about the stacks we're seeing here with the following message:

I'm dealing with some weird stacks in https://bugzilla.mozilla.org/show\_bug.cgi?id=1833790 that should be impossible. I'm pretty sure in the past it was possible to see weird symbol names because something in the linker pipeline would seem to de-duplicate identical sequences of machine code. Is that (still) possible? If so, is there an easy way to find out the set of symbols that collapsed in the symbol I'm seeing reported?

A particular stack of interest is https://crash-stats.mozilla.org/report/index/f1c20040-6a2c-4b93-b236-3c7f80230529 where frame 0 mozilla::PerformanceCounter::GetTotalDispatchCount() should be an impossible symbol because it's something that can only be called on the main thread (the cool searchfox diagram helps confirm this).

Helping my suspicion is that frame 21 claims to be nsWindowRoot::DispatchEvent which is also not a possible thing because it's also only possible on the main thread. We expect to instead see DOMEventTargetHelper::DispatchEvent's symbol name, although the virtual override's code is identical (albeit slightly differently formatted). And if I download the build symbols from the build dir and look in the C06F8DC52242C4844C4C44205044422E1 "xul.sym" file contents, I can see there is a symbol definition for "nsWindowRoot::DispatchEvent" but not for "DOMEventTargetHelper::DispatchEvent" which would suggest that they've been consolidated.

Comment 3

[Yannis Juglaret [:yannis]]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #2)

A particular stack of interest is https://crash-stats.mozilla.org/report/index/f1c20040-6a2c-4b93-b236-3c7f80230529 where frame 0 mozilla::PerformanceCounter::GetTotalDispatchCount() should be an impossible symbol because it's something that can only be called on the main thread (the cool searchfox diagram helps confirm this).

I do not have a generic solution to disambiguate cases of code reuse like this one, but in this specific build of xul.dll, any getter function that returns a field with offset 0x10 could end up showing as xul!mozilla::PerformanceCounter::GetTotalDispatchCount after optimization (they could share the same code):

> dt mozilla::PerformanceCounter mTotalDispatchCount
xul!mozilla::PerformanceCounter
   +0x010 mTotalDispatchCount : mozilla::Atomic<unsigned long long,2,void>

> u xul!mozilla::PerformanceCounter::GetTotalDispatchCount
00007ffc`e3b5c370 488b4110        mov     rax,qword ptr [rcx+10h]
00007ffc`e3b5c374 c3              ret

This specific kind of ambiguity can usually be avoided by putting the getters in the class header file so that they get inlined (see e.g. how the call to GetJSContext() got compiled below).

In your case, the offending getter would be this one:

WorkerPrivate* StrongWorkerRef::Private() const {
  NS_ASSERT_OWNINGTHREAD(StrongWorkerRef);
  return mWorkerPrivate;
}

Because the relevant fields are:

> dt mozilla::dom::FetchStreamReader mWorkerRef
xul!mozilla::dom::FetchStreamReader
   +0x028 mWorkerRef : RefPtr<mozilla::dom::StrongWorkerRef>

> dt mozilla::dom::StrongWorkerRef mWorkerPrivate
xul!mozilla::dom::StrongWorkerRef
   +0x010 mWorkerPrivate : Ptr64 mozilla::dom::WorkerPrivate

> dt mozilla::dom::WorkerPrivate mJSContext
xul!mozilla::dom::WorkerPrivate
   +0x480 mJSContext : Ptr64 JSContext

The offending caller code is this one:

streamReader->CloseAndRelease(
    streamReader->mWorkerRef->Private()->GetJSContext(),
   NS_ERROR_DOM_INVALID_STATE_ERR);

Which is compiled as follows (a tail-call to xul!mozilla::dom::FetchStreamReader::CloseAndRelease):

// rcx = streamReader->mWorkerRef
00007ffc`e4ce5e7d 488b4e28        mov     rcx,qword ptr [rsi+28h]

// rax = rcx->Private()
00007ffc`e4ce5e81 e8ea64e7fe      call    xul!mozilla::PerformanceCounter::GetTotalDispatchCount (00007ffc`e3b5c370)

// aCx (rdx) = rax->mJSContext
00007ffc`e4ce5e86 488b9080040000  mov     rdx,qword ptr [rax+480h]

// this (rcx) = streamReader
00007ffc`e4ce5e8d 4889f1          mov     rcx,rsi

// aStatus (r8) = 0x8053000B (NS_ERROR_DOM_INVALID_STATE_ERR)
00007ffc`e4ce5e90 41b80b005380    mov     r8d,8053000Bh

// tail-call
00007ffc`e4ce5e96 4883c420        add     rsp,20h
00007ffc`e4ce5e9a 5e              pop     rsi
00007ffc`e4ce5e9b e9d08bffff      jmp     xul!mozilla::dom::FetchStreamReader::CloseAndRelease (00007ffc`e4cdea70)

So, here, this crash is the non-debug build equivalent of a MOZ_ASSERT(streamReader->mWorkerRef); failure.

Edit: If you suspect an ambiguity due to code reuse, you can use the ln (List Nearest Symbols) command in WinDbg to double check that there are multiple symbols at the same address. If that is the case it will show multiple entries as Exact matches like below. You will still have to then solve the ambiguity with the kind of analysis shown above though. Here this command reveals that 80 (!) different functions use the code found at xul!mozilla::PerformanceCounter::GetTotalDispatchCount, and, as you suspected, 2 use the code found at xul!nsWindowRoot::DispatchEvent:

> ln xul!mozilla::PerformanceCounter::GetTotalDispatchCount

(00007ffc`e3b5c370)   xul!mozilla::PerformanceCounter::GetTotalDispatchCount   |  (00007ffc`e3b5c380)   xul!mozilla::PerformanceCounter::GetDispatchCount
Exact matches:
    xul!mozilla::extensions::ExtensionMockAPI::GetGlobalObject (void)
    xul!icu_72::TimeZoneTransition::getFrom (void)
    xul!SkDWriteFontFileStream::getPosition (void)
    xul!SkRescaleAndReadPixels::Result::rowBytes (int)
    // ...
    xul!mozilla::dom::StrongWorkerRef::Private (void)
    xul!webrtc::`anonymous namespace'::WrappedYuvBuffer<webrtc::I420BufferInterface>::DataY (void)
    xul!ne_buffer_tell (void *)
    xul!mozilla::ProfileBufferChunkManagerWithLocalLimit::MaxTotalSize (void)
    xul!neqo_transport::cc::classic_cc::impl$2::cwnd<neqo_transport::cc::cubic::Cubic> (struct neqo_transport::cc::classic_cc::ClassicCongestionControl<neqo_transport::cc::cubic::Cubic> *)

> ln xul!nsWindowRoot::DispatchEvent

 [/builds/worker/checkouts/gecko/dom/base/nsWindowRoot.cpp @ 80] (00007ffc`e243c260)   xul!nsWindowRoot::DispatchEvent   |  (00007ffc`e243c2d0)   xul!nsWindowRoot::GetOrCreateListenerManager
Exact matches:
    xul!nsWindowRoot::DispatchEvent (class mozilla::dom::Event *, mozilla::dom::CallerType, class mozilla::ErrorResult *)
    xul!mozilla::DOMEventTargetHelper::DispatchEvent (class mozilla::dom::Event *, mozilla::dom::CallerType, class mozilla::ErrorResult *)

Comment 4

[Jens Stutte [:jstutte]]

Based on comment 3 this looks like a regression from bug 1770630 then.

I wonder if we see a situation where we already called CloseAndRelease and set mWorkerRef to nullptr but for whatever reason this did not immediately free the worker reference? In fact there can be situations where we have two RefPtr to the same worker ref active, so maybe the callback should account for mAsyncWaitWorkerRef, too?

Comment 5

[Jens Stutte [:jstutte]]

Attachment: Bug 1833790 - Ignore StrongWorkerRef callback if mWorkerRef is already cleared. r?#dom-worker-reviewers

Comment 6

[BugBot [:suhaib / :marco]]

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b93f6512bbbc
Ignore StrongWorkerRef callback if mWorkerRef is already cleared. r=dom-worker-reviewers,asuth

Comment 8

[Norisz Fay [:noriszfay]]

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

Comment 9

[BugBot [:suhaib / :marco]]

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

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

For more information, please visit BugBot documentation.

Comment 10

[Jens Stutte [:jstutte]]

Comment on attachment 9337647 [details]
Bug 1833790 - Ignore StrongWorkerRef callback if mWorkerRef is already cleared. r?#dom-worker-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: Users can experience crashes, apparently only in content processes.
• 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): The fix is very simple and straight forward wrt the crash stacks.
• String changes made/needed:
• Is Android affected?: Yes

Comment 11

[Donal Meehan [:dmeehan]]

Comment on attachment 9337647 [details]
Bug 1833790 - Ignore StrongWorkerRef callback if mWorkerRef is already cleared. r?#dom-worker-reviewers

Approved for 115.0b3.

Comment 12

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bd9ba4216df4

Comment 14

[BugBot [:suhaib / :marco]]

Copying crash signatures from duplicate bugs.