| Summary: | [GPUProcess] Add basic GPUProcess crash handling for media playback | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||
| Component: | Media | Assignee: | Chris Dumez <cdumez> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | eric.carlson, ews-watchlist, glenn, jer.noble, peng.liu6, philipj, sergio, webkit-bug-importer | ||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Chris Dumez
2020-11-11 15:40:27 PST
Created attachment 413877 [details]
Patch
Created attachment 413880 [details]
Patch
Comment on attachment 413880 [details]
Patch
Will investigate why API test is failing on iOS.
(In reply to Chris Dumez from comment #3) > Comment on attachment 413880 [details] > Patch > > Will investigate why API test is failing on iOS. It is caused by a flaky GPUProcess crash: Exception Type: EXC_BAD_ACCESS (SIGSEGV) Exception Codes: KERN_INVALID_ADDRESS at 0x0000000000000008 Exception Note: EXC_CORPSE_NOTIFY Termination Signal: Segmentation fault: 11 Termination Reason: Namespace SIGNAL, Code 0xb Terminating Process: exc handler [52931] VM Regions Near 0x8: --> __TEXT 105e71000-105e72000 [ 4K] r-x/r-x SM=COW /Volumes/*/*.Development Application Specific Information: CoreSimulator 732.18 - Device: iPhone 8 (BF967462-6631-4C74-AFFE-1305D10AD782) - Runtime: iOS 14.2 (18B86) - DeviceType: iPhone 8 Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebKit 0x0000000106c71d70 WTF::RetainPtr<CAContext>::operator CAContext*() const + 16 (RetainPtr.h:104) 1 com.apple.WebKit 0x0000000106c71e31 WebKit::LayerHostingContext::setFencePort(unsigned int) + 33 (LayerHostingContext.mm:153) 2 com.apple.WebKit 0x0000000106bd2d09 WebKit::RemoteMediaPlayerProxy::setVideoInlineSizeFenced(WebCore::IntSize const&, WTF::MachSendRight const&) + 73 (RemoteMediaPlayerProxyCocoa.mm:65) 3 com.apple.WebKit 0x0000000106b32829 void IPC::callMemberFunctionImpl<WebKit::RemoteMediaPlayerProxy, void (WebKit::RemoteMediaPlayerProxy::*)(WebCore::IntSize const&, WTF::MachSendRight const&), std::__1::tuple<WebCore::IntSize, WTF::MachSendRight>, 0ul, 1ul>(WebKit::RemoteMediaPlayerProxy*, void (WebKit::RemoteMediaPlayerProxy::*)(WebCore::IntSize const&, WTF::MachSendRight const&), std::__1::tuple<WebCore::IntSize, WTF::MachSendRight>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul>) + 185 (HandleMessage.h:41) 4 com.apple.WebKit 0x0000000106b31bc0 void IPC::callMemberFunction<WebKit::RemoteMediaPlayerProxy, void (WebKit::RemoteMediaPlayerProxy::*)(WebCore::IntSize const&, WTF::MachSendRight const&), std::__1::tuple<WebCore::IntSize, WTF::MachSendRight>, std::__1::integer_sequence<unsigned long, 0ul, 1ul> >(std::__1::tuple<WebCore::IntSize, WTF::MachSendRight>&&, WebKit::RemoteMediaPlayerProxy*, void (WebKit::RemoteMediaPlayerProxy::*)(WebCore::IntSize const&, WTF::MachSendRight const&)) + 112 (HandleMessage.h:47) 5 com.apple.WebKit 0x0000000106b2157d void IPC::handleMessage<Messages::RemoteMediaPlayerProxy::SetVideoInlineSizeFenced, WebKit::RemoteMediaPlayerProxy, void (WebKit::RemoteMediaPlayerProxy::*)(WebCore::IntSize const&, WTF::MachSendRight const&)>(IPC::Decoder&, WebKit::RemoteMediaPlayerProxy*, void (WebKit::RemoteMediaPlayerProxy::*)(WebCore::IntSize const&, WTF::MachSendRight const&)) + 157 (HandleMessage.h:120) 6 com.apple.WebKit 0x0000000106b1f883 WebKit::RemoteMediaPlayerProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder&) + 2675 (RemoteMediaPlayerProxyMessageReceiver.cpp:290) 7 com.apple.WebKit 0x0000000106d1b954 WebKit::RemoteMediaPlayerManagerProxy::didReceivePlayerMessage(IPC::Connection&, IPC::Decoder&) + 100 (RemoteMediaPlayerManagerProxy.cpp:172) 8 com.apple.WebKit 0x0000000106c82ddd WebKit::GPUConnectionToWebProcess::dispatchMessage(IPC::Connection&, IPC::Decoder&) + 205 (GPUConnectionToWebProcess.cpp:352) 9 com.apple.WebKit 0x0000000106b91489 WebKit::GPUConnectionToWebProcess::didReceiveMessage(IPC::Connection&, IPC::Decoder&) + 505 (GPUConnectionToWebProcessMessageReceiver.cpp:92) 10 com.apple.WebKit 0x0000000106538c2a IPC::Connection::dispatchMessage(IPC::Decoder&) + 634 (Connection.cpp:1048) 11 com.apple.WebKit 0x0000000106539d70 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 528 (Connection.cpp:1148) 12 com.apple.WebKit 0x000000010653a3d0 IPC::Connection::dispatchOneIncomingMessage() + 208 (Connection.cpp:1217) 13 com.apple.WebKit 0x000000010655a2b8 IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_8::operator()() + 88 (Connection.cpp:1011) 14 com.apple.WebKit 0x000000010655a1ce WTF::Detail::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_8, void>::call() + 30 (Function.h:52) 15 com.apple.JavaScriptCore 0x000000011d1df602 WTF::Function<void ()>::operator()() const + 130 (Function.h:83) 16 com.apple.JavaScriptCore 0x000000011d2541c5 WTF::RunLoop::performWork() + 341 (RunLoop.cpp:123) 17 com.apple.JavaScriptCore 0x000000011d2579b1 WTF::RunLoop::performWork(void*) + 33 (RunLoopCF.cpp:46) 18 com.apple.CoreFoundation 0x0000000113a8c37a __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17 19 com.apple.CoreFoundation 0x0000000113a8c272 __CFRunLoopDoSource0 + 180 20 com.apple.CoreFoundation 0x0000000113a8b754 __CFRunLoopDoSources0 + 248 21 com.apple.CoreFoundation 0x0000000113a85f1f __CFRunLoopRun + 878 22 com.apple.CoreFoundation 0x0000000113a856c6 CFRunLoopRunSpecific + 567 23 com.apple.Foundation 0x00000001060397b9 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 209 24 com.apple.Foundation 0x00000001060399cd -[NSRunLoop(NSRunLoop) run] + 76 25 libxpc.dylib 0x0000000115395176 _xpc_objc_main + 591 26 libxpc.dylib 0x0000000115397115 xpc_main + 143 27 com.apple.WebKit 0x0000000106f3edd2 WebKit::XPCServiceMain(int, char const**) + 434 (XPCServiceMain.mm:208) 28 com.apple.WebKit 0x000000010838842b WKXPCServiceMain + 27 (WKMain.mm:33) 29 com.apple.WebKit.GPU 0x0000000105e71d42 main + 34 (AuxiliaryProcessMain.cpp:30) 30 libdyld.dylib 0x0000000115046409 start + 1 Created attachment 413941 [details]
Patch
Comment on attachment 413941 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413941&action=review > Source/WebKit/ChangeLog:9 > + as if a network happened and we pause the player. We may be able to do s/network/network error/ > Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:293 > + player->gpuProcessCrashed(); Nit. Probably "restart()" or something similar could be a better name? Comment on attachment 413941 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413941&action=review >> Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:293 >> + player->gpuProcessCrashed(); > > Nit. Probably "restart()" or something similar could be a better name? I think the current name is good, better to be explicit. Created attachment 413958 [details]
Patch
Comment on attachment 413958 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413958&action=review > Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:282 > + for (auto& pair : std::exchange(m_players, { })) Looks a little strange here. So the players will be destroyed and new ones will be created? (In reply to Peng Liu from comment #9) > Comment on attachment 413958 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=413958&action=review > > > Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:282 > > + for (auto& pair : std::exchange(m_players, { })) > > Looks a little strange here. So the players will be destroyed and new ones > will be created? Yes, MediaPlayer::loadWithNextMediaEngine() gets called from MediaPlayer::reloadAndResumePlaybackIfNeeded(). MediaPlayer::loadWithNextMediaEngine() reconstructs m_private here: } else if (m_currentMediaEngine != engine) { m_currentMediaEngine = engine; m_private = engine->createMediaEnginePlayer(this); (In reply to Chris Dumez from comment #10) > (In reply to Peng Liu from comment #9) > > Comment on attachment 413958 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=413958&action=review > > > > > Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:282 > > > + for (auto& pair : std::exchange(m_players, { })) > > > > Looks a little strange here. So the players will be destroyed and new ones > > will be created? > > Yes, MediaPlayer::loadWithNextMediaEngine() gets called from > MediaPlayer::reloadAndResumePlaybackIfNeeded(). > MediaPlayer::loadWithNextMediaEngine() reconstructs m_private here: > } else if (m_currentMediaEngine != engine) { > m_currentMediaEngine = engine; > m_private = engine->createMediaEnginePlayer(this); I don't have to std::exchange() here. If you want I can copy the HashMap instead. The fact is that calling reloadAndResumePlaybackIfNeeded() which destroy the MediaPlayerPrivateRemote objects, which will cause them to get removed from m_players. Created attachment 413969 [details]
Patch
Comment on attachment 413969 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413969&action=review > Source/WebCore/platform/graphics/MediaPlayer.cpp:735 > +void MediaPlayer::seekWhenPossible(const MediaTime& time) Maybe I could merge this logic into seek() instead of adding this new function. I went the conservative way with this new function but I can try tweaking seek() instead if media people think it would be better. > Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerManager.cpp:287 > + ASSERT_WITH_MESSAGE(!player, "reloadAndResumePlaybackIfNeeded should destroy this player and construct a new one"); I went with a HashMap copy and an ASSERT() based on Peng's comment. Hopefully it is clearer. Comment on attachment 413969 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413969&action=review Thanks Chris! >> Source/WebCore/platform/graphics/MediaPlayer.cpp:735 >> +void MediaPlayer::seekWhenPossible(const MediaTime& time) > > Maybe I could merge this logic into seek() instead of adding this new function. I went the conservative way with this new function but I can try tweaking seek() instead if media people think it would be better. I think having this a a separate function is fine. Committed r269750: <https://trac.webkit.org/changeset/269750> All reviewed patches have been landed. Closing bug and clearing flags on attachment 413969 [details]. |