1635338 - Native Allocations create a recursive ProfileChunkedBuffer call

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect, P2)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

With the "Native Allocations" feature, we can get crash stacks like this:

Assertion failure: mOwningThreadId != tid, at c:/mozilla-source/obj-mc-dbg/dist/include\mozilla/BaseProfilerDetail.h:48
#01: profiler_add_marker_for_thread (c:\mozilla-source\mozilla-central\tools\profiler\core\platform.cpp:5020)
#02: profiler_add_native_allocation_marker (c:\mozilla-source\mozilla-central\tools\profiler\core\platform.cpp:4939)
#03: mozilla::profiler::AllocCallback (c:\mozilla-source\mozilla-central\tools\profiler\core\memory_hooks.cpp:389)
#04: replace_malloc (c:\mozilla-source\mozilla-central\tools\profiler\core\memory_hooks.cpp:453)
#05: moz_xmalloc (c:\mozilla-source\mozilla-central\memory\mozalloc\mozalloc.cpp:53)
#06: mozilla::ProfileChunkedBuffer::RequestChunk (c:\mozilla-source\obj-mc-dbg\dist\include\mozilla\ProfileChunkedBuffer.h:1248)
#07: mozilla::ProfileChunkedBuffer::ReserveAndPutRaw<lambda at c:/mozilla-source/obj-mc-dbg/dist/include\mozilla/ProfileChunkedBuffer.h:618:9',lambda at c:/mozilla-source/obj-mc-dbg/dist/include\mozilla/ProfileChunkedBuffer.h:623:9'>::<unnamed-tag>::operator (c:\mozilla-source\obj-mc-dbg\dist\include\mozilla\ProfileChunkedBuffer.h:1467)
#08: mozilla::ProfileChunkedBuffer::ReserveAndPutRaw<lambda at c:/mozilla-source/obj-mc-dbg/dist/include\mozilla/ProfileChunkedBuffer.h:618:9',lambda at c:/mozilla-source/obj-mc-dbg/dist/include\mozilla/ProfileChunkedBuffer.h:623:9'> (c:\mozilla-source\obj-mc-dbg\dist\include\mozilla\ProfileChunkedBuffer.h:1471)
#09: mozilla::ProfileChunkedBuffer::PutObjects<ProfileBufferEntry::Kind,int,mozilla::ProfileBufferUnownedCString,unsigned int,const ProfilerMarkerPayload *,double> (c:\mozilla-source\obj-mc-dbg\dist\include\mozilla\ProfileChunkedBuffer.h:658)
#10: racy_profiler_add_marker (c:\mozilla-source\mozilla-central\tools\profiler\core\platform.cpp:4898)

To summarize: ProfileChunkedBuffer -> malloc -> memory hook -> profiler_add_native_allocation_marker -> ProfileChunkedBuffer, which attemps to lock the buffer mutex again.

To solve this, profiler_add_native_allocation_marker could check whether ProfileChunkedBuffer is already locked in the same thread and avoid calling it, because in that case the allocation is most probably due to ProfileChunkedBuffer itself.
And it's not a problem to lose this data, because the profiler doesn't need to show its own allocations (that would not happen when the profiler is not running).

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Regressed by bug 1630872 (when we switched to ProfileChunkedBuffer), which landed in FF77.

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1635338 - BaseProfilerMutex::IsLockedOnCurrentThread - r?canaltinova,gregtatum

Make BaseProfilerMutex::mOwningThreadId non-optional, and use it in IsLockedOnCurrentThread(), which is similar to the one in Gecko Profiler.
Add related BaseProfilerMaybeMutex::IsActivatedAndLockedOnCurrentThread().

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1635338 - baseprofiler::profiler_is_locked_on_current_thread - r?canaltinova!,gregtatum

Mimic Gecko Profiler's profiler_is_locked_on_current_thread() in Base Profiler.

Depends on D73786

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1635338 - ProfileChunkedBuffer::IsThreadSafeAndLockedOnCurrentThread - r?canaltinova!,gregtatum

Expose ProfileChunkedBuffer's mutex (if present), so that potential callers can avoid recursive calls that would lock or crash.

Depends on D73787

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1635338 - profiler_is_locked_on_current_thread() now also checks for the buffer lock - r?canaltinova!,gregtatum

profiler_add_native_allocation_marker() is the best location to catch profile buffer call recursion due to the buffer's (and friends') own memory operations.
It's acceptable to lose this data, because it's related to the profiler, and therefore would not exist in a non-profiled Firefox session.

Depends on D73788

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/93c3511b9b23
BaseProfilerMutex::IsLockedOnCurrentThread - r=gregtatum,canaltinova
https://hg.mozilla.org/integration/autoland/rev/903b6c75896b
baseprofiler::profiler_is_locked_on_current_thread - r=gregtatum,canaltinova
https://hg.mozilla.org/integration/autoland/rev/3b31d60ad78f
ProfileChunkedBuffer::IsThreadSafeAndLockedOnCurrentThread - r=gregtatum,canaltinova
https://hg.mozilla.org/integration/autoland/rev/b8156e1ef0c5
profiler_is_locked_on_current_thread() now also checks for the buffer lock  - r=gregtatum,canaltinova

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/93c3511b9b23
https://hg.mozilla.org/mozilla-central/rev/903b6c75896b
https://hg.mozilla.org/mozilla-central/rev/3b31d60ad78f
https://hg.mozilla.org/mozilla-central/rev/b8156e1ef0c5

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

"Native Allocations" is only available on Nightly, so we won't fix it on 77 which is now in Beta.