[webkit-reviews] review granted: [Bug 222293] [GPUP] Use a WeakPtr of GPUConnectionToWebProcess instead of a reference in some media related objects : [Attachment 421258] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Peng Liu
<peng.liu6 at apple.com>'s request for review:
Bug 222293: [GPUP] Use a WeakPtr of GPUConnectionToWebProcess instead of a
reference in some media related objects
https://bugs.webkit.org/show_bug.cgi?id=222293

Attachment 421258: Patch

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




--- Comment #3 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 421258
  --> https://bugs.webkit.org/attachment.cgi?id=421258
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