GMPParent::ActorDestroy should not re-enter itself

RESOLVED FIXED in mozilla33

Status

()

Core
WebRTC
--
critical
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Benjamin Smedberg, Assigned: Benjamin Smedberg)

Tracking

(Blocks: 1 bug)

unspecified
mozilla33
Points:
2
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
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.
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+
(Assignee)

Comment 3

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad1513416811
Target Milestone: --- → mozilla33
(Assignee)

Updated

4 years ago
QA Whiteboard: [qa-]

Updated

4 years ago
Status: NEW → ASSIGNED
Iteration: --- → 33.3
https://hg.mozilla.org/mozilla-central/rev/ad1513416811
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Flags: firefox-backlog+
You need to log in before you can comment on or make changes to this bug.