Bug 224017 – Crash under GPUProcessProxy::getGPUProcessConnection()

• Component: Canvas
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

<rdar://75146104>

[Said Abou-Hallawa]

Created attachment 424823 [details]
Patch

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 424859 [details]
Patch

[EWS]

Committed r275349: <https://commits.webkit.org/r275349>

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

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

<rdar://76794645>

[Said Abou-Hallawa]

Created attachment 427210 [details]
Patch

[Chris Dumez]

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

[Chris Dumez]

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.

[Chris Dumez]

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.

[Said Abou-Hallawa]

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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 427217 [details]
Patch

[Chris Dumez]

Created attachment 427225 [details]
Patch

[Chris Dumez]

Created attachment 427227 [details]
Patch

[Chris Dumez]

Created attachment 427231 [details]
Patch

[Chris Dumez]

Created attachment 427232 [details]
Patch

[EWS]

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].