Bug 224017

Summary: Crash under GPUProcessProxy::getGPUProcessConnection()
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: CanvasAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, cdumez, darin, dino, ggaren, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=225486
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch none

Description Said Abou-Hallawa 2021-03-31 14:12:51 PDT
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.
Comment 1 Said Abou-Hallawa 2021-03-31 14:35:53 PDT
<rdar://75146104>
Comment 2 Said Abou-Hallawa 2021-03-31 15:00:34 PDT
Created attachment 424823 [details]
Patch
Comment 3 Said Abou-Hallawa 2021-03-31 18:21:24 PDT
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.
Comment 4 Said Abou-Hallawa 2021-03-31 18:46:11 PDT
Created attachment 424859 [details]
Patch
Comment 5 EWS 2021-04-01 02:04:19 PDT
Committed r275349: <https://commits.webkit.org/r275349>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424859 [details].
Comment 6 Said Abou-Hallawa 2021-04-27 15:53:27 PDT
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.
Comment 7 Said Abou-Hallawa 2021-04-27 15:54:07 PDT
<rdar://76794645>
Comment 8 Said Abou-Hallawa 2021-04-27 16:14:20 PDT
Created attachment 427210 [details]
Patch
Comment 9 Chris Dumez 2021-04-27 16:21:11 PDT
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 10 Chris Dumez 2021-04-27 16:22:40 PDT
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 11 Chris Dumez 2021-04-27 16:30:05 PDT
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 12 Said Abou-Hallawa 2021-04-27 16:31:56 PDT
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 13 Chris Dumez 2021-04-27 16:45:42 PDT
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.
Comment 14 Chris Dumez 2021-04-27 17:12:48 PDT
Created attachment 427217 [details]
Patch
Comment 15 Chris Dumez 2021-04-27 18:37:14 PDT
Created attachment 427225 [details]
Patch
Comment 16 Chris Dumez 2021-04-27 19:11:26 PDT
Created attachment 427227 [details]
Patch
Comment 17 Chris Dumez 2021-04-27 20:07:41 PDT
Created attachment 427231 [details]
Patch
Comment 18 Chris Dumez 2021-04-27 20:20:42 PDT
Created attachment 427232 [details]
Patch
Comment 19 EWS 2021-04-28 08:20:57 PDT
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].