820596 - DASH: Need to defend against nullptr calls to state machine and reentrant monitor

Status: RESOLVED FIXED

(Core :: Networking, enhancement)

Description

[Steve Workman [:sworkman] (INACTIVE)]

Some calls to the state machine and reentrant monitor may happen after those objects are deleted in DASH-WebM. We should avoid this by having nullptr checks in the calling functions.

Comment 1

[Steve Workman [:sworkman] (INACTIVE)]

To clarify, DASHRepDecoder removes the reference to mDecoderStateMachine in its Shutdown function, before calling MediaDecoder::Shutdown. This is so that only DASHDecoder performs cleanup and shutdown on the state machine, since it is shared among DASHDecoder and its sub DASHRepDecoder objects.

Similarly, DASHDecoder::mReentrantMonitor is shared among these objects. It has been observed during testing that DASHRepReader objects which have been waiting on data become unblocked during shutdown, but AFTER DASHDecoder has been deleted. GetReentrantMonitor returns a stale reference in this case and Firefox crashes.

Comment 2

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Defend against nullptr calls to mDecoderStateMachine and mReentrantMonitor

-- Add nullptr checks for mDecoderStateMachine / GetStateMachine(), and return silently or 0 etc. as appropriate.

-- Add a nullptr check for mMainDecoder in DASHRepDecoder::GetReentrantMonitor(), and return DASHRepDecoder's own monitor. This assumes that such a case will only happen after shutdown, when the decoder thread has been waiting on data.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 691101 [details] [diff] [review]
v1.0 Defend against nullptr calls to mDecoderStateMachine and mReentrantMonitor

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

::: content/media/MediaDecoder.cpp
@@ +1433,5 @@
>    return mDecoderStateMachine;
>  }
>  
>  bool MediaDecoder::IsShutdown() const {
> +  NS_ENSURE_TRUE(GetStateMachine(), false);

Should this return true instead of false?

Comment 4

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Chris Pearce (:cpearce) from comment #3)
> Comment on attachment 691101 [details] [diff] [review]
> v1.0 Defend against nullptr calls to mDecoderStateMachine and
> mReentrantMonitor
> 
> Review of attachment 691101 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/MediaDecoder.cpp
> @@ +1433,5 @@
> >    return mDecoderStateMachine;
> >  }
> >  
> >  bool MediaDecoder::IsShutdown() const {
> > +  NS_ENSURE_TRUE(GetStateMachine(), false);
> 
> Should this return true instead of false?

Yes.

Thanks for the review!

Comment 5

[Steve Workman [:sworkman] (INACTIVE)]

https://tbpl.mozilla.org/?tree=Try&rev=fd2f2694f9dd

Comment 6

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 691101 [details] [diff] [review]
v1.0 Defend against nullptr calls to mDecoderStateMachine and mReentrantMonitor

https://hg.mozilla.org/integration/mozilla-inbound/rev/02a0cad51a22

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/02a0cad51a22