Closed Bug 1635338 Opened 9 months ago Closed 8 months ago

Native Allocations create a recursive ProfileChunkedBuffer call

Categories

(Core :: Gecko Profiler, defect, P2)

77 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla78
Tracking Status
firefox-esr68 --- unaffected
firefox75 --- unaffected
firefox76 --- unaffected
firefox77 --- wontfix
firefox78 --- fixed

People

(Reporter: gerald, Assigned: gerald)

References

(Regression)

Details

(Keywords: regression)

Attachments

(4 files)

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).

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

Keywords: regression
Regressed by: 1630872
No longer regressed by: 1622179

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

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

Depends on D73786

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

Depends on D73787

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

Attachment #9145701 - Attachment description: Bug 1635338 - BaseProfilerMutex::IsLockedOnCurrentThread - r?canaltinova!,gregtatum → Bug 1635338 - BaseProfilerMutex::IsLockedOnCurrentThread - r?canaltinova,gregtatum
Attachment #9145704 - Attachment description: Bug 1635338 - Avoid recursion in profiler_add_native_allocation_marker - r?canaltinova!,gregtatum → Bug 1635338 - profiler_is_locked_on_current_thread() now also checks for the buffer lock - r?canaltinova!,gregtatum
Pushed by gsquelart@mozilla.com:
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

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

Regressions: 1637895
You need to log in before you can comment on or make changes to this bug.