Bug 182474 – Multiple bmalloc scavenger threads is unexpected

• Component: bmalloc
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Multiple bmalloc scavenger threads is unexpected

Steps to reproduce:
1. Run with trunk
2. Load a webpage
3. `sample <pid>` the com.apple.WebKit.WebContent process
  => Multiple bmalloc Scavenger threads

Sample:
...
    830 Thread_242317
    + 830 thread_start  (in libsystem_pthread.dylib) + 13  [0x7fff636c3bf9]
    +   830 _pthread_start  (in libsystem_pthread.dylib) + 377  [0x7fff636c450d]
    +     830 _pthread_body  (in libsystem_pthread.dylib) + 340  [0x7fff636c4661]
    +       830 void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(bmalloc::Scavenger*), bmalloc::Scavenger*> >(void*)  (in JavaScriptCore) + 40  [0x7fff3e941b48]
    +         830 bmalloc::Scavenger::threadEntryPoint(bmalloc::Scavenger*)  (in JavaScriptCore) + 9  [0x7fff3e941809]
    +           830 bmalloc::Scavenger::threadRunLoop()  (in JavaScriptCore) + 251  [0x7fff3e941a0b]
    +             830 void std::__1::condition_variable_any::wait<std::__1::unique_lock<bmalloc::Mutex> >(std::__1::unique_lock<bmalloc::Mutex>&)  (in JavaScriptCore) + 86  [0x7fff3e941d56]
    +               830 std::__1::condition_variable::wait(std::__1::unique_lock<std::__1::mutex>&)  (in libc++.1.dylib) + 18  [0x7fff61317cb0]
    +                 830 _pthread_cond_wait  (in libsystem_pthread.dylib) + 732  [0x7fff636c5589]
    +                   830 __psynch_cvwait  (in libsystem_kernel.dylib) + 10  [0x7fff63508a1e]
...
    830 Thread_244724
    + 830 thread_start  (in libsystem_pthread.dylib) + 13  [0x7fff636c3bf9]
    +   830 _pthread_start  (in libsystem_pthread.dylib) + 377  [0x7fff636c450d]
    +     830 _pthread_body  (in libsystem_pthread.dylib) + 340  [0x7fff636c4661]
    +       830 void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(bmalloc::Scavenger*), bmalloc::Scavenger*> >(void*)  (in JavaScriptCore) + 40  [0x7fff3e941b48]
    +         830 bmalloc::Scavenger::threadEntryPoint(bmalloc::Scavenger*)  (in JavaScriptCore) + 9  [0x7fff3e941809]
    +           830 bmalloc::Scavenger::threadRunLoop()  (in JavaScriptCore) + 251  [0x7fff3e941a0b]
    +             830 void std::__1::condition_variable_any::wait<std::__1::unique_lock<bmalloc::Mutex> >(std::__1::unique_lock<bmalloc::Mutex>&)  (in JavaScriptCore) + 86  [0x7fff3e941d56]
    +               830 std::__1::condition_variable::wait(std::__1::unique_lock<std::__1::mutex>&)  (in libc++.1.dylib) + 18  [0x7fff61317cb0]
    +                 830 _pthread_cond_wait  (in libsystem_pthread.dylib) + 732  [0x7fff636c5589]
    +                   830 __psynch_cvwait  (in libsystem_kernel.dylib) + 10  [0x7fff63508a1e]

Notes:

- One is created via JavaScriptCore with PerProcess<Scavenger> for normal allocations

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x00007fff3e941810 JavaScriptCore`bmalloc::Scavenger::Scavenger(std::__1::lock_guard<bmalloc::StaticMutex>&)
    frame #1: 0x00007fff3e70e198 JavaScriptCore`bmalloc::PerProcess<bmalloc::Scavenger>::getSlowCase() + 72
    frame #2: 0x00007fff3e946db4 JavaScriptCore`bmalloc::Heap::Heap(bmalloc::HeapKind, std::__1::lock_guard<bmalloc::StaticMutex>&) + 1636
    ...

- Another is created later on via WebCore with PerProcess<Scavenger> for isoheap allocations

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x00007fff3e941810 JavaScriptCore`bmalloc::Scavenger::Scavenger(std::__1::lock_guard<bmalloc::StaticMutex>&)
    frame #1: 0x00007fff4922da88 WebCore`bmalloc::PerProcess<bmalloc::Scavenger>::getSlowCase() + 72
    frame #2: 0x00007fff49374400 WebCore`bmalloc::IsoDirectory<bmalloc::IsoConfig<1184u>, 32u>::takeFirstEligible() + 80
    ...

It seems these should be the same instance, but were unexpectedly different instances.

[Joseph Pecoraro]

<rdar://problem/37175526>

[Joseph Pecoraro]

Created attachment 333062 [details]
[PATCH] Proposed Fix

I didn't convert all PerProcess<T> => SafePerProcess<T> because it requires up-front knowledge of the type specializations. This falls over for IsoTLSDeallocatorEntry, which is constructed with many different configurations:

    IsoTLSDeallocatorEntry<IsoConfig<32>>
    IsoTLSDeallocatorEntry<IsoConfig<40>>
    IsoTLSDeallocatorEntry<IsoConfig<56>>
    IsoTLSDeallocatorEntry<IsoConfig<72>>
    IsoTLSDeallocatorEntry<IsoConfig<88>>
    IsoTLSDeallocatorEntry<IsoConfig<96>>
    IsoTLSDeallocatorEntry<IsoConfig<104>>
    IsoTLSDeallocatorEntry<IsoConfig<120>>
    ...

This also presents a case where there is another PerProcess<T> cross image issue. JavaScriptCore and WebCore each produce their own `IsoTLSDeallocatorEntry<IsoConfig<40>>` instance. So we may have to abandon the PerProcess<T> templated approach here.

[Joseph Pecoraro]

> This also presents a case where there is another PerProcess<T> cross image
> issue. JavaScriptCore and WebCore each produce their own
> `IsoTLSDeallocatorEntry<IsoConfig<40>>` instance. So we may have to abandon
> the PerProcess<T> templated approach here.

Or someone can figure out how to somehow export the templated symbols. A few of us looked at this and couldn't figure out how to make it happen generically, and fell back to the duplicate class above which explicitly creates the symbols and exports them.

[WebKit Commit Bot]

Comment on attachment 333062 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 333062

Committed r228107: <https://trac.webkit.org/changeset/228107>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

Comment on attachment 333062 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=333062&action=review

> Source/bmalloc/bmalloc/PerProcess.h:41
> +// WARNING: PerProcess<T> does not export its storage. So in actuality when

Maybe we should rename PerProcess?

[Joseph Pecoraro]

(In reply to Saam Barati from comment #6)
> Comment on attachment 333062 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333062&action=review
> 
> > Source/bmalloc/bmalloc/PerProcess.h:41
> > +// WARNING: PerProcess<T> does not export its storage. So in actuality when
> 
> Maybe we should rename PerProcess?

Yeah, I think any follow-ups can/should happen with bug 182496.