1039572 - GMPParent::ActorDestroy should not re-enter itself

Status: RESOLVED FIXED

Description

[Benjamin Smedberg]

Currently if a plugin crashes, GMPParent::ActorDestroy re-enters itself through this stack:

>	xul.dll!mozilla::gmp::GMPParent::ActorDestroy(mozilla::ipc::IProtocolManager<mozilla::ipc::IProtocol>::ActorDestroyReason aWhy) Line 289	C++
 	xul.dll!mozilla::gmp::PGMPParent::DestroySubtree(mozilla::ipc::IProtocolManager<mozilla::ipc::IProtocol>::ActorDestroyReason why) Line 758	C++
 	xul.dll!mozilla::gmp::PGMPParent::OnChannelError() Line 625	C++
 	xul.dll!mozilla::ipc::MessageChannel::NotifyMaybeChannelError() Line 1558	C++
 	xul.dll!mozilla::ipc::MessageChannel::Close() Line 1660	C++
 	xul.dll!mozilla::gmp::PGMPParent::Close() Line 94	C++
 	xul.dll!mozilla::gmp::GMPParent::UnloadProcess() Line 127	C++
 	xul.dll!mozilla::gmp::GMPParent::ActorDestroy(mozilla::ipc::IProtocolManager<mozilla::ipc::IProtocol>::ActorDestroyReason aWhy) Line 298	C++
 	xul.dll!mozilla::gmp::PGMPParent::DestroySubtree(mozilla::ipc::IProtocolManager<mozilla::ipc::IProtocol>::ActorDestroyReason why) Line 758	C++
 	xul.dll!mozilla::gmp::PGMPParent::OnChannelError() Line 625	C++
 	xul.dll!mozilla::ipc::MessageChannel::NotifyMaybeChannelError() Line 1558	C++
 	xul.dll!mozilla::ipc::MessageChannel::OnNotifyMaybeChannelError() Line 1587	C++
 	xul.dll!DispatchToMethod<mozilla::ipc::MessageChannel,void (__thiscall mozilla::ipc::MessageChannel::*)(void)>(mozilla::ipc::MessageChannel * obj, void (void) * method, const Tuple0 & arg) Line 384	C++

I don't believe we should call Close from within ActorDestroy, because we already know that the actor is closing. I still need to look at a "normal" shutdown sequence to see if we can skip the Close altogether, or only in some cases.

I'm going to take this, since I'm touching stuff in the area.

Comment 1

[Benjamin Smedberg]

Created attachment 8458030 [details] [diff] [review]
bug1039572-actordestroy-reenter

This patch sits on top of bug 1039575: it rejiggers the crash and shutdown sequence so that the state machine of GMPParent is clear and so that ActorDestroy doesn't re-enter.

Comment 2

[Randell Jesup [:jesup]]

Comment on attachment 8458030 [details] [diff] [review]
bug1039572-actordestroy-reenter

Review of attachment 8458030 [details] [diff] [review]:
-----------------------------------------------------------------

Looks quite good!

::: content/media/gmp/GMPParent.cpp
@@ +118,5 @@
> +{
> +  MOZ_ASSERT(GMPThread() == NS_GetCurrentThread());
> +
> +  if (mState == GMPStateNotLoaded || mState == GMPStateClosing) {
> +    MOZ_ASSERT(mVideoDecoders.IsEmpty() && mVideoEncoders.IsEmpty());

cpearce will need to update this for audio, but that will be his patches (in a number of places)

Comment 3

[Benjamin Smedberg]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad1513416811

Comment 4

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/ad1513416811