Bug 218825 – [GPUProcess] Add basic GPUProcess crash handling for media playback

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Chris Dumez]

Add basic GPUProcess crash handling for media playback.

[Chris Dumez]

Created attachment 413877 [details]
Patch

[Chris Dumez]

Created attachment 413880 [details]
Patch

[Chris Dumez]

Comment on attachment 413880 [details]
Patch

Will investigate why API test is failing on iOS.

[Chris Dumez]

(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

[Chris Dumez]

Created attachment 413941 [details]
Patch

[Peng Liu]

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?

[Eric Carlson]

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.

[Chris Dumez]

Created attachment 413958 [details]
Patch

[Peng Liu]

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?

[Chris Dumez]

(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);

[Chris Dumez]

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

[Chris Dumez]

Created attachment 413969 [details]
Patch

[Chris Dumez]

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.

[Eric Carlson]

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.

[EWS]

Committed r269750: <https://trac.webkit.org/changeset/269750>

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

[Radar WebKit Bug Importer]

<rdar://problem/71343235>