The completion handler of some messages may reference members of GPUProcessProxy or WebProcessPool even though they may be already destroyed. A crash tracer may look like this: WebKit: WebKit::GPUProcessProxy::getGPUProcessConnection(WebKit::WebProcessProxy&, WTF::CompletionHandler<void (WebKit::GPUProcessConnectionInfo const&)>&&) <== WebKit: WTF::Detail::CallableWrapper<WebKit::WebProcessPool::getGPUProcessConnection(WebKit::WebProcessProxy&, WTF::CompletionHandler<void (WebKit::GPUProcessConnectionInfo const&)>&&)::$_15, void, WebKit::GPUProcessConnectionInfo const&>::call(WebKit::GPUProcessConnectionInfo const&) WebKit: WTF::Detail::CallableWrapper<WebKit::WebProcessPool::getGPUProcessConnection(WebKit::WebProcessProxy&, WTF::CompletionHandler<void (WebKit::GPUProcessConnectionInfo const&)>&&)::$_15, void, WebKit::GPUProcessConnectionInfo const&>::call(WebKit::GPUProcessConnectionInfo const&) WebKit: WTF::Detail::CallableWrapper<WebKit::GPUProcessProxy::getGPUProcessConnection(WebKit::WebProcessProxy&, WTF::CompletionHandler<void (WebKit::GPUProcessConnectionInfo const&)>&&)::$_3, void, WTF::Optional<IPC::Attachment>&&>::call(WTF::Optional<IPC::Attachment>&&) WebKit: WTF::Detail::CallableWrapper<unsigned long long WebKit::AuxiliaryProcessProxy::sendWithAsyncReply<Messages::GPUProcess::CreateGPUConnectionToWebProcess, WebKit::GPUProcessProxy::getGPUProcessConnection(WebKit::WebProcessProxy&, WTF::CompletionHandler<void (WebKit::GPUProcessConnectionInfo const&)>&&)::$_3>(Messages::GPUProcess::CreateGPUConnectionToWebProcess&&, WebKit::GPUProcessProxy::getGPUProcessConnection(WebKit::WebProcessProxy&, WTF::CompletionHandler<void (WebKit::GPUProcessConnectionInfo const&)>&&)::$_3&&, unsigned long long, WTF::OptionSet<IPC::SendOption>, WebKit::AuxiliaryProcessProxy::ShouldStartProcessThrottlerActivity)::'lambda'(IPC::Decoder*), void, IPC::Decoder*>::call(IPC::Decoder*) WebKit: WTF::Detail::CallableWrapper<WebKit::AuxiliaryProcessProxy::sendMessage(std::__1::unique_ptr<IPC::Encoder, std::__1::default_delete<IPC::Encoder> >, WTF::OptionSet<IPC::SendOption>, WTF::Optional<std::__1::pair<WTF::CompletionHandler<void (IPC::Decoder*)>, unsigned long long> >&&, WebKit::AuxiliaryProcessProxy::ShouldStartProcessThrottlerActivity)::$_0, void, IPC::Decoder*>::call(IPC::Decoder*) WebKit: WebKit::AuxiliaryProcessProxy::~AuxiliaryProcessProxy() WebKit: WTF::RefCounted<WebKit::GPUProcessProxy, std::__1::default_delete<WebKit::GPUProcessProxy> >::deref() const WebKit: WebKit::WebProcessPool::~WebProcessPool() WebKit: -[WKProcessPool dealloc] WebKit: WebKit::WebProcessProxy::~WebProcessProxy() WebKit: WebKit::WebProcessProxy::~WebProcessProxy() WebKit: WTF::Detail::CallableWrapper<WebKit::WebPageProxy::close()::$_6, void>::~CallableWrapper() JavaScriptCore: WTF::RunLoop::performWork() JavaScriptCore: WTF::RunLoop::performWork(void*) GPUProcessProxy is being called although its destruction has already begun since we are inside AuxiliaryProcessProxy::~AuxiliaryProcessProxy(). This means we can't deal with this object as a super class of AuxiliaryProcessProxy anymore.
<rdar://75146104>
Created attachment 424823 [details] Patch
Comment on attachment 424823 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=424823&action=review > Source/WebKit/UIProcess/WebProcessPool.cpp:365 > + if (m_gpuProcess) > + m_gpuProcess->shutDownProcess(); I was wrong about this change. The life cycle of m_gpuProcess is different from the WebProcesses which are owned by the WebProcessPool. I made this assertion in this code #if ENABLE(GPU_PROCESS) if (m_gpuProcess) { ASSERT(m_gpuProcess->hasOneRef()); m_gpuProcess->shutDownProcess(); } #endif And I ran the canvas layout tests and this assertion fires multiple times. I think the assertion fires because there is a singleton GPU Process for all the WebProcesses. If there are multiple WebProcessPools, i.e. WebProcessPool::allProcessPools() has more than one item, m_gpuProcess will only be destroyed when the last WebProcessPool is destroyed.
Created attachment 424859 [details] Patch
Committed r275349: <https://commits.webkit.org/r275349> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424859 [details].
r275349 does not guarantee all the pending messages of GPUProcessProxy will be processed before starting the deletion of WebProcessPool. Some messages may be processed inside the AuxiliaryProcessProxy destructor.
<rdar://76794645>
Created attachment 427210 [details] Patch
Comment on attachment 427210 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427210&action=review > Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp:199 > + replyToPendingMessages(); ~AuxiliaryProcessProxy() already does that. So why does ~GPUProcessProxy() need to do it too? > Source/WebKit/UIProcess/WebProcessPool.cpp:364 > + // Force calling the destrsuctor of GPUProcessProxy to ensure the completion typo: destrsuctor
Comment on attachment 427210 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427210&action=review > Source/WebKit/UIProcess/WebProcessPool.cpp:366 > + m_gpuProcess = nullptr; Note that this does not necessarily destroy the GPUProcessProxy since a single GPUProcessProxy may be shared by several WebProcessPool objects.
Comment on attachment 427210 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427210&action=review > Source/WebKit/UIProcess/WebProcessPool.cpp:-364 > - if (m_gpuProcess) Note that this previous code was wrong too since a GPUProcessProxy can be shared by several process pool. Having a single WebProcessPool getting destroyed should not cause pending messages from other process pool to get replied to eagerly.
Comment on attachment 427210 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427210&action=review >> Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp:199 >> + replyToPendingMessages(); > > ~AuxiliaryProcessProxy() already does that. So why does ~GPUProcessProxy() need to do it too? See the completion handler in GPUProcessProxy::getGPUProcessConnection(). We capture this as a WeakPtr and if we reply the pending messages from ~AuxiliaryProcessProxy(), GPUProcessProxy will be invalid and a call like this->connection() will crash. >> Source/WebKit/UIProcess/WebProcessPool.cpp:366 >> + m_gpuProcess = nullptr; > > Note that this does not necessarily destroy the GPUProcessProxy since a single GPUProcessProxy may be shared by several WebProcessPool objects. So this may not be needed then.
Comment on attachment 427210 [details] Patch I think the safest fix here is to protect the WebProcessPool in the lambda in WebProcessPool::getGPUProcessConnection(), instead of using a WeakPtr.
Created attachment 427217 [details] Patch
Created attachment 427225 [details] Patch
Created attachment 427227 [details] Patch
Created attachment 427231 [details] Patch
Created attachment 427232 [details] Patch
Committed r276712 (237117@main): <https://commits.webkit.org/237117@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 427232 [details].