GPUBuffer seems to be ref'd / deref'd from multiple thread concurrently but is not ThreadSafeRefCounted

[Chris Dumez]

GPUBuffer seems to be ref'd / deref'd from multiple thread concurrently but is not ThreadSafeRefCounted:
stderr:
2   0x21a25e471 WTF::RefCountedBase::derefBase() const
3   0x21aa4aa2f WTF::RefCounted<WebCore::GPUObjectBase, std::__1::default_delete<WebCore::GPUObjectBase> >::deref() const
4   0x21aa8c451 void WTF::derefIfNotNull<WebCore::GPUBuffer>(WebCore::GPUBuffer*)
5   0x21aa8c419 WTF::RefPtr<WebCore::GPUBuffer, WTF::DumbPtrTraits<WebCore::GPUBuffer> >::~RefPtr()
6   0x21aa79695 WTF::RefPtr<WebCore::GPUBuffer, WTF::DumbPtrTraits<WebCore::GPUBuffer> >::~RefPtr()
7   0x21aa79679 __destroy_helper_block_e8_32c63_ZTSN3WTF6RefPtrIN7WebCore9GPUBufferENS_13DumbPtrTraitsIS2_EEEE
8   0x7fff737a7bf3 _Block_release
9   0x7fff4c38571a _doMTLDispatch
10  0x7fff4c385c04 -[_MTLCommandBuffer didCompleteWithStartTime:endTime:error:]
11  0x7fff4c3859fe -[MTLIOAccelCommandBuffer didCompleteWithStartTime:endTime:error:]
12  0x7fff4c3858ed -[_MTLCommandQueue commandBufferDidComplete:startTime:completionTime:error:]
13  0x7fff64b786d2 ioAccelCommandQueueBlockFenceCallback
14  0x7fff4a10f7ab IODispatchCalloutFromCFMessage
15  0x7fff4a10f62d _IODispatchCalloutWithDispatch
16  0x7fff73727bde dispatch_mig_server
17  0x7fff7371163d _dispatch_client_callout
18  0x7fff73713de6 _dispatch_continuation_pop
19  0x7fff73722f42 _dispatch_source_invoke
20  0x7fff73717792 _dispatch_lane_serial_drain
21  0x7fff73718396 _dispatch_lane_invoke
22  0x7fff737206ed _dispatch_workloop_worker_thread
23  0x7fff73951611 _pthread_wqthread
24  0x7fff739513fd start_wqthread
LEAK: 1 WebPageProxy

[Chris Dumez]

GPUBuffer gets deref'd on a background Metal thread even though the main threads holds references to it:
Thread 11 Crashed:: Dispatch queue: com.Metal.CompletionQueueDispatch
0   com.apple.JavaScriptCore      	0x0000000777d472be WTFCrash + 14 (Assertions.cpp:305)
1   com.apple.WebCore             	0x0000000760017271 WTF::RefCountedBase::derefBase() const + 209 (RefCounted.h:136)
2   com.apple.WebCore             	0x000000076079651f WTF::RefCounted<WebCore::GPUObjectBase, std::__1::default_delete<WebCore::GPUObjectBase> >::deref() const + 31 (RefCounted.h:196)
3   com.apple.WebCore             	0x00000007607d50f1 void WTF::derefIfNotNull<WebCore::GPUBuffer>(WebCore::GPUBuffer*) + 49 (RefPtr.h:45)
4   com.apple.WebCore             	0x00000007607d50b9 WTF::RefPtr<WebCore::GPUBuffer, WTF::DumbPtrTraits<WebCore::GPUBuffer> >::~RefPtr() + 41 (RefPtr.h:69)
5   com.apple.WebCore             	0x00000007607c3395 WTF::RefPtr<WebCore::GPUBuffer, WTF::DumbPtrTraits<WebCore::GPUBuffer> >::~RefPtr() + 21 (RefPtr.h:69)
6   com.apple.WebCore             	0x00000007607c3379 __destroy_helper_block_e8_32c63_ZTSN3WTF6RefPtrIN7WebCore9GPUBufferENS_13DumbPtrTraitsIS2_EEEE + 25 (GPUBufferMetal.mm:155)
7   libsystem_blocks.dylib        	0x00007fff6e46ebed _Block_release + 101
8   com.apple.Metal               	0x00007fff3c80dc14 _doMTLDispatch + 43
9   com.apple.Metal               	0x00007fff3c80dfa1 -[_MTLCommandBuffer didCompleteWithStartTime:endTime:error:] + 378
10  com.apple.Metal               	0x00007fff3c80de06 -[MTLIOAccelCommandBuffer didCompleteWithStartTime:endTime:error:] + 88
11  com.apple.Metal               	0x00007fff3c80dcf5 -[_MTLCommandQueue commandBufferDidComplete:startTime:completionTime:error:] + 197
12  com.apple.IOAccelerator       	0x00007fff5af0db89 ioAccelCommandQueueBlockFenceCallback + 44
13  com.apple.framework.IOKit     	0x00007fff39e3c72b IODispatchCalloutFromCFMessage + 364
14  com.apple.framework.IOKit     	0x00007fff39e3c5ac _IODispatchCalloutWithDispatch + 33
15  libdispatch.dylib             	0x00007fff6e3de367 dispatch_mig_server + 356
16  libdispatch.dylib             	0x00007fff6e3c75ee _dispatch_client_callout + 8
17  libdispatch.dylib             	0x00007fff6e3c97a0 _dispatch_continuation_pop + 414
18  libdispatch.dylib             	0x00007fff6e3d9440 _dispatch_source_invoke + 2084
19  libdispatch.dylib             	0x00007fff6e3cca60 _dispatch_lane_serial_drain + 263
20  libdispatch.dylib             	0x00007fff6e3cd532 _dispatch_lane_invoke + 363
21  libdispatch.dylib             	0x00007fff6e3d6ba1 _dispatch_workloop_worker_thread + 582
22  libsystem_pthread.dylib       	0x00007fff6e620773 _pthread_wqthread + 290
23  libsystem_pthread.dylib       	0x00007fff6e6205cf start_wqthread + 15

[Chris Dumez]

This code does not look safe:
#if USE(METAL)
void GPUBuffer::commandBufferCommitted(MTLCommandBuffer *commandBuffer)
{
    ++m_numScheduledCommandBuffers;

    auto protectedThis = makeRefPtr(this);
    BEGIN_BLOCK_OBJC_EXCEPTIONS;
    [commandBuffer addCompletedHandler:^(id<MTLCommandBuffer>) {
        protectedThis->commandBufferCompleted();
    }];
    END_BLOCK_OBJC_EXCEPTIONS;
}

void GPUBuffer::commandBufferCompleted()
{
    ASSERT(m_numScheduledCommandBuffers);

    if (m_numScheduledCommandBuffers == 1 && state() == State::Mapped) {
        callOnMainThread([this, protectedThis = makeRef(*this)] () {
            runMappingCallback();
        });
    }

    --m_numScheduledCommandBuffers;
}
#endif // USE(METAL)

given that this involves multiple threads, refs/derefs |this| and |this| is a GPUBuffer which subclasses RefCounted.

[Chris Dumez]

Created attachment 376067 [details]
Patch

[Geoffrey Garen]

Comment on attachment 376067 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 376067 [details]
Patch

Clearing flags on attachment: 376067

Committed r248532: <https://trac.webkit.org/changeset/248532>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54214916>