Bug 232179 – RemoteRenderingBackend should not send IPC in the middle of destruction

• Component: WebKit2
• Status: RESOLVED FIXED

[Wenson Hsieh]

It looks like there's still a flaky GPU process crash, found while running layout tests in EWS: https://ews-build.s3-us-west-2.amazonaws.com/macOS-Catalina-Release-WK2-Tests-EWS/r442191-21289/com.apple.WebKit.GPU.Development-10292-crash-log.txt

Main thread:

Thread 0:: Dispatch queue: com.apple.main-thread
0   libsystem_kernel.dylib        	0x00007fff700c355e __ulock_wait + 10
1   libsystem_pthread.dylib       	0x00007fff7018a5c2 _pthread_join + 347
2   com.apple.JavaScriptCore      	0x00000001102a69f4 WTF::Thread::waitForCompletion() + 68
3   com.apple.WebKit              	0x0000000107c66d35 IPC::StreamConnectionWorkQueue::stopAndWaitForCompletion() + 81
4   com.apple.WebKit              	0x0000000107cd2de9 WebKit::RemoteRenderingBackend::~RemoteRenderingBackend() + 309
5   com.apple.WebKit              	0x0000000107cd2f59 non-virtual thunk to WebKit::RemoteRenderingBackend::~RemoteRenderingBackend() + 21
6   com.apple.WebKit              	0x0000000107cd47a1 IPC::ScopedActiveMessageReceiveQueue<WebKit::RemoteRenderingBackend, WTF::RefPtr<WebKit::RemoteRenderingBackend, WTF::RawPtrTraits<WebKit::RemoteRenderingBackend>, WTF::DefaultRefDerefTraits<WebKit::RemoteRenderingBackend> > >::~ScopedActiveMessageReceiveQueue() + 23
7   com.apple.WebKit              	0x0000000107cd7293 WTF::HashTable<WTF::ObjectIdentifier<WebKit::RenderingBackendIdentifierType>, WTF::KeyValuePair<WTF::ObjectIdentifier<WebKit::RenderingBackendIdentifierType>, IPC::ScopedActiveMessageReceiveQueue<WebKit::RemoteRenderingBackend, WTF::RefPtr<WebKit::RemoteRenderingBackend, WTF::RawPtrTraits<WebKit::RemoteRenderingBackend>, WTF::DefaultRefDerefTraits<WebKit::RemoteRenderingBackend> > > >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WTF::ObjectIdentifier<WebKit::RenderingBackendIdentifierType>, IPC::ScopedActiveMessageReceiveQueue<WebKit::RemoteRenderingBackend, WTF::RefPtr<WebKit::RemoteRenderingBackend, WTF::RawPtrTraits<WebKit::RemoteRenderingBackend>, WTF::DefaultRefDerefTraits<WebKit::RemoteRenderingBackend> > > > >, WTF::DefaultHash<WTF::ObjectIdentifier<WebKit::RenderingBackendIdentifierType> >, WTF::HashMap<WTF::ObjectIdentifier<WebKit::RenderingBackendIdentifierType>, IPC::ScopedActiveMessageReceiveQueue<WebKit::RemoteRenderingBackend, WTF::RefPtr<WebKit::RemoteRenderingBackend, WTF::RawPtrTraits<WebKit::RemoteRenderingBackend>, WTF::DefaultRefDerefTraits<WebKit::RemoteRenderingBackend> > >, WTF::DefaultHash<WTF::ObjectIdentifier<WebKit::RenderingBackendIdentifierType> >, WTF::HashTraits<WTF::ObjectIdentifier<WebKit::RenderingBackendIdentifierType> >, WTF::HashTraits<IPC::ScopedActiveMessageReceiveQueue<WebKit::RemoteRenderingBackend, WTF::RefPtr<WebKit::RemoteRenderingBackend, WTF::RawPtrTraits<WebKit::RemoteRenderingBackend>, WTF::DefaultRefDerefTraits<WebKit::RemoteRenderingBackend> > > >, WTF::HashTableTraits>::KeyValuePairTraits, WTF::HashTraits<WTF::ObjectIdentifier<WebKit::RenderingBackendIdentifierType> > >::remove(WTF::KeyValuePair<WTF::ObjectIdentifier<WebKit::RenderingBackendIdentifierType>, IPC::ScopedActiveMessageReceiveQueue<WebKit::RemoteRenderingBackend, WTF::RefPtr<WebKit::RemoteRenderingBackend, WTF::RawPtrTraits<WebKit::RemoteRenderingBackend>, WTF::DefaultRefDerefTraits<WebKit::RemoteRenderingBackend> > > >*) + 25
8   com.apple.WebKit              	0x0000000107ccd495 WebKit::GPUConnectionToWebProcess::releaseRenderingBackend(WTF::ObjectIdentifier<WebKit::RenderingBackendIdentifierType>) + 105
9   com.apple.WebKit              	0x0000000107c9c0dd WebKit::GPUConnectionToWebProcess::didReceiveMessage(IPC::Connection&, IPC::Decoder&) + 567
10  com.apple.WebKit              	0x0000000107b951f1 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 221
11  com.apple.WebKit              	0x0000000107b91b42 IPC::Connection::SyncMessageState::ConnectionAndIncomingMessage::dispatch() + 42
12  com.apple.WebKit              	0x0000000107b91eb8 IPC::Connection::SyncMessageState::dispatchMessagesAndResetDidScheduleDispatchMessagesForConnection(IPC::Connection&) + 848
13  com.apple.JavaScriptCore      	0x0000000110289e01 WTF::RunLoop::performWork() + 513
14  com.apple.JavaScriptCore      	0x000000011028a712 WTF::RunLoop::performWork(void*) + 34
15  com.apple.CoreFoundation      	0x00007fff35e5e884 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
16  com.apple.CoreFoundation      	0x00007fff35e5e823 __CFRunLoopDoSource0 + 103
17  com.apple.CoreFoundation      	0x00007fff35e5e63d __CFRunLoopDoSources0 + 209
18  com.apple.CoreFoundation      	0x00007fff35e5d359 __CFRunLoopRun + 937
19  com.apple.CoreFoundation      	0x00007fff35e5c953 CFRunLoopRunSpecific + 466
20  com.apple.Foundation          	0x00007fff3851a1c8 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 212
21  com.apple.Foundation          	0x00007fff385ccc6f -[NSRunLoop(NSRunLoop) run] + 76
22  libxpc.dylib                  	0x00007fff701d24ea _xpc_objc_main.cold.4 + 49
23  libxpc.dylib                  	0x00007fff701d2430 _xpc_objc_main + 559
24  libxpc.dylib                  	0x00007fff701d1f63 xpc_main + 377
25  com.apple.WebKit              	0x0000000107d74b77 WebKit::XPCServiceMain(int, char const**) + 266
26  libdyld.dylib                 	0x00007fff6ff80cc9 start + 1

Rendering backend thread:

Thread 9 Crashed:: RemoteRenderingBackend work queue
0   com.apple.WebKit              	0x0000000107cd1f30 WebKit::RemoteRenderingBackend::didFlush(WTF::ObjectIdentifier<WebCore::GraphicsContextFlushIdentifierType>, WebCore::ProcessQualified<WTF::ObjectIdentifier<WebCore::RenderingResourceIdentifierType> >) + 30
1   com.apple.WebKit              	0x0000000107c6847a IPC::StreamServerConnection::dispatchStreamMessage(IPC::Decoder&&, IPC::StreamMessageReceiver&) + 32
2   com.apple.WebKit              	0x0000000107c680d8 IPC::StreamServerConnection::dispatchStreamMessages(unsigned long) + 374
3   com.apple.WebKit              	0x0000000107c66ed3 IPC::StreamConnectionWorkQueue::processStreams() + 355
4   com.apple.WebKit              	0x0000000107c676b6 WTF::Detail::CallableWrapper<IPC::StreamConnectionWorkQueue::startProcessingThread()::$_0, void>::call() + 46
5   com.apple.JavaScriptCore      	0x00000001102a407c WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) + 124
6   com.apple.JavaScriptCore      	0x00000001102a6909 WTF::wtfThreadEntryPoint(void*) + 9
7   libsystem_pthread.dylib       	0x00007fff70189109 _pthread_start + 148
8   libsystem_pthread.dylib       	0x00007fff70184b8b thread_start + 15

[Wenson Hsieh]

The issue here is that while we're tearing down the stream connection (and running through the remaining IPC stream messages as a result), if one of those stream messages is `RemoteRenderingBackend::didFlush()`, we'll end up trying to send an IPC message (and call into IPC::MessageSender methods) during destruction of RemoteRenderingBackend.

This work needs to happen earlier — probably right after we've torn down the IPC stream connection.

[Wenson Hsieh]

*** Bug 232183 has been marked as a duplicate of this bug. ***

[Wenson Hsieh]

Created attachment 442240 [details]
Patch

[Darin Adler]

Comment on attachment 442240 [details]
Patch

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

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:147
> +            m_remoteDisplayLists.set(renderingResourceIdentifier, remoteDisplayList);

When we know the key is not already in the map, set is just a slower slightly larger version of add. The difference is that after doing an add, set overwrites the value if the key is already in the map. Given that, I would have used add here.

[Wenson Hsieh]

Comment on attachment 442240 [details]
Patch

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

Thanks for the review!

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:147
>> +            m_remoteDisplayLists.set(renderingResourceIdentifier, remoteDisplayList);
> 
> When we know the key is not already in the map, set is just a slower slightly larger version of add. The difference is that after doing an add, set overwrites the value if the key is already in the map. Given that, I would have used add here.

Makes sense, since we should only be adding new entries here — I'll change this to use `add()`.

[Wenson Hsieh]

Created attachment 442317 [details]
Patch for landing

[EWS]

Committed r284768 (243477@main): <https://commits.webkit.org/243477@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 442317 [details].

[Radar WebKit Bug Importer]

<rdar://problem/84596594>