Bug 164442 – void ThreadTimers::sharedTimerFiredInternal() is not thread safe....

• Component: WebCore JavaScript
• Status: RESOLVED INVALID

I am looking at a driver level bug that backtraces to Webkit, upon inspecting the code in sharedTimerFiredInternal it's obvious that this is not thread safe code

The issue is here...

void ThreadTimers::sharedTimerFiredInternal()

...

    // Do a re-entrancy check.
    if (m_firingTimers)
        return;
    m_firingTimers = true;
    m_pendingSharedTimerFireTime = 0;


Where is the lock on m_firingTimers? on a multicore CPU any numbers of threads could be inspecting m_firingTimers and believe its ok.. there should be a true locked access on m_firingTimers to make it thread safe.



Maybe there are no additional threads but I have 73 instances of the same crash trace and it looks wildly suspicious of a threading issue..



* APPLICATION: com.apple.WebKit.WebContent
* SIGNATURE: com.apple.GeForceMTLDriver: -[NVMTLRenderPipelineState dealloc] + 69
* MORE INFORMATION: https://crashtracer.apple.com/signature/show/52146254?app=com.apple.WebKit.WebContent&build=16A313a&users=external

Summary of a selection of backtraces attributed to this bug. The stack frame considered to be the unique "crash signature" is highlighted ==> like this <==. This frame is used for aggregation when filing these bugs and does not necessarily imply fault.

50 libobjc.A.dylib: objc_msgSend
==> 50 GeForceMTLDriver: -[NVMTLRenderPipelineState dealloc] <==
50 QuartzCore: CA::OGL::metal_delete_state(CA::OGL::MetalPipeState*, CA::OGL::MetalPipeState*, void*)
50 QuartzCore: x_hash_table_foreach
50 QuartzCore: CA::OGL::MetalContext::purge_shaders()
50 QuartzCore: CA::OGL::MetalContext::purge(bool)
50 QuartzCore: CA::CG::Renderer::invalidate_context()
50 QuartzCore: CA::CG::IOSurfaceRenderer::~IOSurfaceRenderer()
50 QuartzCore: CA::CG::IOSurfaceRenderer::~IOSurfaceRenderer()
50 QuartzCore: CA::CG::IOSurfaceRenderer::will_suspend_callback(void*)
50 libdispatch.dylib: _dispatch_client_callout
50 libdispatch.dylib: _dispatch_barrier_sync_f_invoke
34 QuartzCore: CA::Render::post_notification(CA::Render::NotificationName, CA::Render::Object*, void*, bool)
| 34 QuartzCore: CABackingStoreCollectBlocking
| 34 WebCore: WebCore::ThreadTimers::sharedTimerFiredInternal()
| 34 WebCore: WebCore::timerFired(__CFRunLoopTimer*, void*)
| 34 CoreFoundation: __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__
| 34 CoreFoundation: __CFRunLoopDoTimer
| 34 CoreFoundation: __CFRunLoopDoTimers
| 34 CoreFoundation: __CFRunLoopRun
| 34 CoreFoundation: CFRunLoopRunSpecific
| 34 HIToolbox: RunCurrentEventLoopInMode
| 34 HIToolbox: ReceiveNextEventCommon
| 34 HIToolbox: _BlockUntilNextEventMatchingListInModeWithFilter
| 34 AppKit: _DPSNextEvent
| 34 AppKit: -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:]
| 34 AppKit: -[NSApplication run]
| 34 AppKit: NSApplicationMain
| 34 libxpc.dylib: _xpc_objc_main
| 34 libxpc.dylib: xpc_main
| 34 com.apple.WebKit.WebContent: main
| 34 libdyld.dylib: start
16 libdispatch.dylib: _dispatch_barrier_sync_f_slow
16 QuartzCore: CA::Render::post_notification(CA::Render::NotificationName, CA::Render::Object*, void*, bool)
16 QuartzCore: CABackingStoreCollectBlocking
16 WebCore: WebCore::ThreadTimers::sharedTimerFiredInternal()
16 WebCore: WebCore::timerFired(__CFRunLoopTimer*, void*)
16 CoreFoundation: __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__
16 CoreFoundation: __CFRunLoopDoTimer
16 CoreFoundation: __CFRunLoopDoTimers
16 CoreFoundation: __CFRunLoopRun
16 CoreFoundation: CFRunLoopRunSpecific
16 HIToolbox: RunCurrentEventLoopInMode
16 HIToolbox: ReceiveNextEventCommon
16 HIToolbox: _BlockUntilNextEventMatchingListInModeWithFilter
16 AppKit: _DPSNextEvent
16 AppKit: -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:]
16 AppKit: -[NSApplication run]
16 AppKit: NSApplicationMain
16 libxpc.dylib: _xpc_objc_main
16 libxpc.dylib: xpc_main
16 com.apple.WebKit.WebContent: main
16 libdyld.dylib: start

[Alexey Proskuryakov]

There is no need for synchronization in this class, each instance is only used from one thread. Please see this comment in ThreadTimers.cpp:

// Timers are created, started and fired on the same thread, and each thread has its own ThreadTimers
// copy to keep the heap and a set of currently firing timers.

[Alexey Proskuryakov]

For Apple employees, this appears to be about rdar://28248383

Yes that is the radar, we are seeing a double release coming up from webkit, but its now more likely from QuartzCore over releasing a metal context

==> 50 GeForceMTLDriver: -[NVMTLRenderPipelineState dealloc] <==
          50 QuartzCore: CA::OGL::metal_delete_state(CA::OGL::MetalPipeState*, CA::OGL::MetalPipeState*, void*)
            50 QuartzCore: x_hash_table_foreach
              50 QuartzCore: CA::OGL::MetalContext::purge_shaders()
                50 QuartzCore: CA::OGL::MetalContext::purge(bool)
                  50 QuartzCore: CA::CG::Renderer::invalidate_context()
                    50 QuartzCore: CA::CG::IOSurfaceRenderer::~IOSurfaceRenderer()
                      50 QuartzCore: CA::CG::IOSurfaceRenderer::~IOSurfaceRenderer()
                        50 QuartzCore: CA::CG::IOSurfaceRenderer::will_suspend_callback(void*)
                          50 libdispatch.dylib: _dispatch_client_callout
                            50 libdispatch.dylib: _dispatch_barrier_sync_f_invoke
                              34 QuartzCore: CA::Render::post_notification(CA::Render::NotificationName, CA::Render::Object*, void*, bool)
                              | 34 QuartzCore: CABackingStoreCollectBlocking
                              |   34 WebCore: WebCore::ThreadTimers::sharedTimerFiredInternal()
                              |     34 WebCore: WebCore::timerFired(__CFRunLoopTimer*, void*)
                              |       34 CoreFoundation: __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__