[GPUP] Use a WeakPtr of GPUConnectionToWebProcess instead of a reference in some media related objects
<rdar://74560577>
Created attachment 421258 [details] Patch
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
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.
Created attachment 421465 [details] Revise the patch based on Eric's comments
commit-queue failed to commit attachment 421465 [details] to WebKit repository. To retry, please set cq+ flag again.
Committed r273473: <https://commits.webkit.org/r273473> All reviewed patches have been landed. Closing bug and clearing flags on attachment 421465 [details].