Closed Bug 1039572 Opened 8 years ago Closed 8 years ago

GMPParent::ActorDestroy should not re-enter itself

Categories

(Core :: WebRTC, defect)

defect
Not set
critical
Points:
2

Tracking

()

RESOLVED FIXED
mozilla33
Iteration:
33.3

People

(Reporter: benjamin, Assigned: benjamin)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
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.
Attachment #8458030 - Flags: review?(rjesup)
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)
Attachment #8458030 - Flags: review?(rjesup) → review+
QA Whiteboard: [qa-]
Status: NEW → ASSIGNED
Iteration: --- → 33.3
https://hg.mozilla.org/mozilla-central/rev/ad1513416811
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Flags: firefox-backlog+
You need to log in before you can comment on or make changes to this bug.