889540 - correctly type streams returned by MediaStreamGraph

Status: NEW

(Core :: Audio/Video: MediaStreamGraph, defect, P4)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: correctly type streams returned by MediaStreamGraph

While looking for manual uses of NS_ADDREF, I noticed that
MediaStreamGraph::Create{Source,TrackUnion,AudioNode>Stream all use
NS_ADDREF, but fail to note that the returned pointer has already been
AddRef'd.  Thus when the returned pointer is assigned (e.g. to mStream
in various AudioNodes), it will have a reference count that is too high,
and will leak.

I have no experience with this code, but I couldn't find obvious places
where corresponding NS_RELEASEs would be performed; MOZ_COUNT_CTOR ought
to be catching leaks for at least AudioNodeStream, though.  So I'm puzzled
about what is going on here.  Does the patch look semi-sane?

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd (:froydnj) from comment #1)
> I have no experience with this code, but I couldn't find obvious places
> where corresponding NS_RELEASEs would be performed; MOZ_COUNT_CTOR ought
> to be catching leaks for at least AudioNodeStream, though.  So I'm puzzled
> about what is going on here.

I guess DOMMediaStream::Destroy takes care of stream destruction, regardless of reference count.

Comment 3

[(no longer active)]

Comment on attachment 770370 [details] [diff] [review]
correctly type streams returned by MediaStreamGraph

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

roc is really the owner of this code, and I know that he and I have disagreed about how to do refcounting on a few occasions in the past, so it's best to ask him directly..  :-)

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 770370 [details] [diff] [review]
correctly type streams returned by MediaStreamGraph

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

::: content/media/DOMMediaStream.cpp
@@ +216,4 @@
>  
>    // Setup track listener
>    mListener = new StreamListener(this);
> +  aStream.get()->AddListener(mListener);

mStream->AddListener

::: content/media/MediaStreamGraph.cpp
@@ +2232,3 @@
>    MediaStreamGraphImpl* graph = static_cast<MediaStreamGraphImpl*>(this);
>    stream->SetGraphImpl(graph);
> +  graph->AppendMessage(new CreateMessage(stream.get()));

This will break things, I think.

The way things currently work is that we addref here, and then ownership of this ref travels with the CreateMessage until it reaches MediaStreamGraphImpl::AddStream (usually on a different thread), where we stick it MediaStreamGraphImpl::mStreams without doing another addref. Therefore the matching release happens when the stream is removed from mStreams or mStreams is destroyed. So I don't think we have a leak here.

I think we can make this clearer and remove manual addreffing by having CreateMessage carry an nsRefPtr<MediaStream> as an *extra* field (instead of overloading ControlMessage::mStream, where it's nice it's not an nsRefPtr), and have AddStream take an already_AddRefed.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: make MediaStreamGraph's streams ownership more visible in the type system

This patch adds the AddStream changes requested.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 770773 [details] [diff] [review]
make MediaStreamGraph's streams ownership more visible in the type system

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

::: content/media/DOMMediaStream.cpp
@@ +216,4 @@
>  
>    // Setup track listener
>    mListener = new StreamListener(this);
> +  aStream.get()->AddListener(mListener);

mStream

::: content/media/MediaStreamGraph.cpp
@@ +2269,5 @@
>                                             aEngine->NodeMainThread()->ChannelInterpretationValue());
>    }
> +  already_AddRefed<AudioNodeStream> addRefedStream = stream.forget();
> +  graph->AppendMessage(new CreateMessage(addRefedStream));
> +  return addRefedStream;

This (and above) looks bad! You're passing an already_Addrefed along two different paths, so it will be released twice!

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> Comment on attachment 770773 [details] [diff] [review]
> make MediaStreamGraph's streams ownership more visible in the type system
> 
> Review of attachment 770773 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/DOMMediaStream.cpp
> @@ +216,4 @@
> >  
> >    // Setup track listener
> >    mListener = new StreamListener(this);
> > +  aStream.get()->AddListener(mListener);
> 
> mStream

Doh, you pointed this out earlier.  My mistake.

> ::: content/media/MediaStreamGraph.cpp
> @@ +2269,5 @@
> >                                             aEngine->NodeMainThread()->ChannelInterpretationValue());
> >    }
> > +  already_AddRefed<AudioNodeStream> addRefedStream = stream.forget();
> > +  graph->AppendMessage(new CreateMessage(addRefedStream));
> > +  return addRefedStream;
> 
> This (and above) looks bad! You're passing an already_Addrefed along two
> different paths, so it will be released twice!

But this is effectively what we were doing before, only now we've typed it correctly!

CreateMessage() stuffs it into an nsRefPtr (mRefedPointer), but to get a suitable already_AddRefed for AddStream, we call mRefedPointer.forget(), so there's no extra releasing on the way to AddStream.  So I don't think there's any extra releasing versus the way it was before...but I haven't tryserver'd this.

I agree that this way of doing things is a little odd.  But I don't see a better way of passing an already_AddRefed into AddStream.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This line:
  nsRefPtr<AudioNodeStream> stream = new AudioNodeStream(aEngine, aKind, aSampleRate);
does an addref.

The caller of CreateAudioNodeStream receives an already_AddRefed. Its responsibility to provide a balancing release is eventually discharged by the AudioNode.

The MediaStreamGraph *also* gets an already_AddRefed into CreateMessage, and will eventually balance that with a release when the stream is removed from mStreams.

Sure looks like too-many-releases to me.

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> This line:
>   nsRefPtr<AudioNodeStream> stream = new AudioNodeStream(aEngine, aKind,
> aSampleRate);
> does an addref.
> 
> The caller of CreateAudioNodeStream receives an already_AddRefed. Its
> responsibility to provide a balancing release is eventually discharged by
> the AudioNode.
> 
> The MediaStreamGraph *also* gets an already_AddRefed into CreateMessage, and
> will eventually balance that with a release when the stream is removed from
> mStreams.
> 
> Sure looks like too-many-releases to me.

OK.  And behold, try agrees with you:

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

But I don't understand this try result, which is adding an extra ref before passing into CreateMessage, and results in leaks:

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

Because before, we NS_ADDREF'd, and then CreateMessage effectively took that ref.  Then CreateAudioNodeStream returned a "raw" pointer and its caller(s) added a ref.

But after, we're passing an already_AddRefed into CreateMessage, so that effectively takes a ref.  And we're moving the AddRef from the callers of CreateAudioNodeStream into CreateAudioNodeStream itself.  And that leaks!

I'll have to sit down and think about this a little--I'm supposed to be enjoying the freedom of the U.S. of A. today. ;)

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: make MediaStreamGraph's streams ownership more visible in the type system

The leaking version pushed to try.

Comment 11

[Maire Reavy [:mreavy]]

Hi Nathan  -- Where does this stand from your perspective?

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #11)
> Hi Nathan  -- Where does this stand from your perspective?

Um.  It would be nice to fix, but I obviously haven't done anything on it for a while.  Not any sort of priority.

Comment 13

[Bulk Bug Changes for mreavy's org]

Mass change P3->P4 to align with new Mozilla triage process.