Bug 222293 – [GPUP] Use a WeakPtr of GPUConnectionToWebProcess instead of a reference in some media related objects

• Component: Media
• Status: RESOLVED FIXED

[Peng Liu]

[GPUP] Use a WeakPtr of GPUConnectionToWebProcess instead of a reference in some media related objects

[Peng Liu]

<rdar://74560577>

[Peng Liu]

Created attachment 421258 [details]
Patch

[Eric Carlson]

Comment on attachment 421258 [details]
Patch

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

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerManagerProxy.cpp:204
>  const Logger& RemoteMediaPlayerManagerProxy::logger() const
>  {
> -    return m_gpuConnectionToWebProcess.logger();
> +    ASSERT(m_gpuConnectionToWebProcess);
> +    return m_gpuConnectionToWebProcess->logger();
>  }

This won't go well if the connection to the web process goes away. 

You should probably change it to `RemoteMediaPlayerManagerProxy::loggerPtr()`, and change the log sites that called this to use `*_LOG_IF(loggerPtr(), ...`

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:140
> +    ASSERT(m_manager.gpuConnectionToWebProcess());
> +    m_mediaSourceProxy = adoptRef(*new RemoteMediaSourceProxy(*m_manager.gpuConnectionToWebProcess(), mediaSourceIdentifier, webMParserEnabled, *this));

This will crash in a release build, so you should rearrange the code so you can return a configuration that signals an error if the web process is gone.

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:255
> +    ASSERT(m_manager.gpuConnectionToWebProcess());

This will also crash and needs to be restructured.

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:381
> +    ASSERT(m_manager.gpuConnectionToWebProcess());

Ditto.

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:415
> +    ASSERT(m_manager.gpuConnectionToWebProcess());

Ditto

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:486
> +    ASSERT(m_manager.gpuConnectionToWebProcess());

Ditto

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:508
> +    ASSERT(m_manager.gpuConnectionToWebProcess());

Ditto

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:530
> +    ASSERT(m_manager.gpuConnectionToWebProcess());

Ditto

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:622
> +    ASSERT(m_manager.gpuConnectionToWebProcess());

Ditto

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:820
> +    ASSERT(m_manager.gpuConnectionToWebProcess());

Ditto

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:851
> +    ASSERT(m_manager.gpuConnectionToWebProcess());

Ditto

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:859
> +    ASSERT(m_manager.gpuConnectionToWebProcess());

Ditto

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:867
> +    ASSERT(m_manager.gpuConnectionToWebProcess());

Ditto

> Source/WebKit/GPUProcess/media/RemoteMediaSourceProxy.cpp:55
> +    ASSERT(m_connectionToWebProcess);

Ditto

> Source/WebKit/GPUProcess/media/RemoteSourceBufferProxy.cpp:63
> +    m_connectionToWebProcess->messageReceiverMap().removeMessageReceiver(Messages::RemoteSourceBufferProxy::messageReceiverName(), m_identifier.toUInt64());

Ditto

[Peng Liu]

Comment on attachment 421258 [details]
Patch

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

>> Source/WebKit/GPUProcess/media/RemoteMediaPlayerManagerProxy.cpp:204
>>  }
> 
> This won't go well if the connection to the web process goes away. 
> 
> You should probably change it to `RemoteMediaPlayerManagerProxy::loggerPtr()`, and change the log sites that called this to use `*_LOG_IF(loggerPtr(), ...`

Based on a discussion with Eric, I changed the implementation to create a logger in RemoteMediaPlayerManagerProxy instead of using the logger created by GPUConnectionToWebProcess.

>> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:140
>> +    m_mediaSourceProxy = adoptRef(*new RemoteMediaSourceProxy(*m_manager.gpuConnectionToWebProcess(), mediaSourceIdentifier, webMParserEnabled, *this));
> 
> This will crash in a release build, so you should rearrange the code so you can return a configuration that signals an error if the web process is gone.

Right. In addition, we will need to check whether "m_manager" is destroyed or not. Because m_manager (RemoteMediaPlayerManagerProxy) is owned by GPUConnectionToWebProcess. When the GPUConnectionToWebProcess instance is destroyed, the "m_manager" will be destroyed as well.

We have to keep a WeakPtr of RemoteMediaPlayerManagerProxy in RemoteMediaPlayerProxy to implement that.

>> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:255
>> +    ASSERT(m_manager.gpuConnectionToWebProcess());
> 
> This will also crash and needs to be restructured.

Right. Fixed as described by the above comment.
And the same for following comments.

[Peng Liu]

Created attachment 421465 [details]
Revise the patch based on Eric's comments

[EWS]

commit-queue failed to commit attachment 421465 [details] to WebKit repository. To retry, please set cq+ flag again.

[EWS]

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

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