STR: 1 Start Firefox with MOZ_LOG=MediaStreamGraph:4 2 Go to https://jsfiddle.net/pehrsons/xaLgt3md/ 3 Click "Start" and allow the capture 4 Click "Add 1000" 5 See in the log that many MediaStreams were created in the graph 6 Click "Clear" 7 In a new tab, go to about:memory and click "GC" and "CC" repeatedly Expected Log should say that MediaStreams were removed from the graph Actual Log doesn't say anything

Jib, thoughts on making a test for this? I'm thinking a chrome-only api for number of streams in MSG, but there's no natural place to put it.

Comment on attachment 8917284 [details] Bug 1407542 - Remove back reference to consumer in MediaStreamTrack. https://reviewboard.mozilla.org/r/188300/#review193524 ::: dom/media/MediaStreamTrack.h:453 (Diff revision 1) > virtual already_AddRefed<MediaStreamTrack> CloneInternal(DOMMediaStream* aOwningStream, > TrackID aTrackID) = 0; > > nsTArray<PrincipalChangeObserver<MediaStreamTrack>*> mPrincipalChangeObservers; > > - nsTArray<RefPtr<MediaStreamTrackConsumer>> mConsumers; > + nsTArray<MediaStreamTrackConsumer*> mConsumers; You want to annotate this as MOZ_NON_OWNING_REF but I don't know how.

For a test, you may like to consider something like http://searchfox.org/mozilla-central/search?q=webaudio-node-demise

(In reply to Andreas Pehrson [:pehrsons] from comment #1) > Jib, thoughts on making a test for this? You could use noGum() to detect garbage collection of all streams. [1] > I'm thinking a chrome-only api for number of streams in MSG, but there's no natural place to put it. A static ChromeOnly method on MediaStream maybe? [1] http://searchfox.org/mozilla-central/rev/ed1d5223adcdc07e9a2589ee20f4e5ee91b4df10/dom/media/tests/mochitest/mediaStreamPlayback.js#243

Comment on attachment 8921438 [details] Bug 1407542 - Implement static chrome-only MediaStream method to get number of MSG-MediaStreams. https://reviewboard.mozilla.org/r/192468/#review197664 ::: dom/media/DOMMediaStream.cpp:621 (Diff revision 1) > + public: > + Counter(MediaStreamGraphImpl* aGraph, > + const RefPtr<Promise>& aPromise) > + : ControlMessage(nullptr) > + , mGraph(aGraph) > + , mPromise(MakeAndAddRef<PtrHolder<Promise>>("DOMMediaStream::Counter::mPromise", aPromise)) I have no idea why we have PtrHolder. Could you use nsMainThreadPtrHolder, which has self-documenting name. ::: dom/media/DOMMediaStream.cpp:630 (Diff revision 1) > + > + void Run() override > + { > + PtrHandle<Promise>& promise = mPromise; > + uint32_t streams = mGraph->mStreams.Length() > + + mGraph->mSuspendedStreams.Length(); I guess this is a coding style which hasn't been decided yet. I'd put + to the end of the previous line and then align mGraphs, but up to you ::: dom/media/DOMMediaStream.cpp:639 (Diff revision 1) > + return NS_OK; > + })); > + } > + > + protected: > + MediaStreamGraphImpl* mGraph; Is it guaranteed that mGraph stays alive long enough. I assume yes, but this needs a comment. Every raw pointer as a member variable basically needs explanation why it is safe to use. We've had tons of security critical issues because of use of raw pointers.

Comment on attachment 8917284 [details] Bug 1407542 - Remove back reference to consumer in MediaStreamTrack. https://reviewboard.mozilla.org/r/188300/#review197670 Added smaug to look at CC macros. ::: dom/media/DOMMediaStream.cpp:393 (Diff revision 2) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(DOMMediaStream, > DOMEventTargetHelper) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mWindow) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mOwnedTracks) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mTracks) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mConsumersToKeepAlive) What's this, and is there any relation? ::: dom/media/MediaStreamTrack.h:453 (Diff revision 2) > virtual already_AddRefed<MediaStreamTrack> CloneInternal(DOMMediaStream* aOwningStream, > TrackID aTrackID) = 0; > > nsTArray<PrincipalChangeObserver<MediaStreamTrack>*> mPrincipalChangeObservers; > > - nsTArray<RefPtr<MediaStreamTrackConsumer>> mConsumers; > + nsTArray<MediaStreamTrackConsumer*> mConsumers; This seems like a change of contract with callers of AddConsumer. Before, we held an owning reference to every element in the list, so if any caller forgot to call RemoveConsumer, no biggie, we kept the consumer alive until mConsumers is deleted. But now, if any caller forgets to call RemoveConsumer we'll have a UAF. I think we need some more assurances here that this is safe. A comment at least, and maybe some asserts. For instance, I don't see any MOZ_ASSERTS that mConsumers.Length() == 0 on destruct here. Is there an overall lifetime invariant here that makes this safe? If so, please add comment.

Comment on attachment 8921438 [details] Bug 1407542 - Implement static chrome-only MediaStream method to get number of MSG-MediaStreams. https://reviewboard.mozilla.org/r/192468/#review197676 ::: dom/media/DOMMediaStream.cpp:595 (Diff revision 1) > + RefPtr<Promise> p = Promise::Create(go, aRv); > + if (aRv.Failed()) { > + return nullptr; > + } > + > + nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(aGlobal.GetAsSupports()); > + if (!window) { > + aRv.Throw(NS_ERROR_UNEXPECTED); > + return nullptr; > + } Maybe check window before creating the promise (otherwise shouldn't we reject the promise when bailing)? ::: dom/media/DOMMediaStream.cpp:638 (Diff revision 1) > + promise->MaybeResolve(streams); > + return NS_OK; > + })); > + } > + > + protected: private?

Comment on attachment 8917284 [details] Bug 1407542 - Remove back reference to consumer in MediaStreamTrack. https://reviewboard.mozilla.org/r/188300/#review197672 but r+ for the CC stuff ::: commit-message-77a4c:4 (Diff revision 2) > +Bug 1407542 - Remove back reference to consumer in MediaStreamTrack. r?jib > + > +It doesn't matter that this is traversed by the cycle collector when the track is live and playing. > +It prevents garbage collection of any number of MediaStreams that contain (thus consume) this track. cycle collection? ::: dom/media/MediaStreamTrack.h:453 (Diff revision 2) > virtual already_AddRefed<MediaStreamTrack> CloneInternal(DOMMediaStream* aOwningStream, > TrackID aTrackID) = 0; > > nsTArray<PrincipalChangeObserver<MediaStreamTrack>*> mPrincipalChangeObservers; > > - nsTArray<RefPtr<MediaStreamTrackConsumer>> mConsumers; > + nsTArray<MediaStreamTrackConsumer*> mConsumers; Does something guarantee the array won't contain pointers to deleted objects? Please explain why this is safe. And note, MOZ_ASSERTs just assert, they don't guarantee non-bad behavior.

Comment on attachment 8921439 [details] Bug 1407542 - Add mochitest checking that MediaStreams can be GCed. https://reviewboard.mozilla.org/r/192470/#review197692 ::: dom/media/tests/mochitest/test_getUserMedia_GC_MediaStream.html:21 (Diff revision 1) > + let copies = []; > + > + let numCopies = 10; > + while (--numCopies >= 0) { > + copies.push(copy(stream)); > + } couldn't resist (numCopies could be a testGC default arg): let copies = new Array(numCopies).fill(0).map(() => copy(stream)); FFTI ::: dom/media/tests/mochitest/test_getUserMedia_GC_MediaStream.html:27 (Diff revision 1) > + > + let numCopies = 10; > + while (--numCopies >= 0) { > + copies.push(copy(stream)); > + } > + ok((await SpecialStream.countUnderlyingStreams()) > startStreams, Nit: redundant () around await

Comment on attachment 8917284 [details] Bug 1407542 - Remove back reference to consumer in MediaStreamTrack. https://reviewboard.mozilla.org/r/188300/#review197670 > What's this, and is there any relation? No relation, this was put in place long ago to keep MediaStreamAudioSourceNodes alive. > This seems like a change of contract with callers of AddConsumer. > > Before, we held an owning reference to every element in the list, so if any caller forgot to call RemoveConsumer, no biggie, we kept the consumer alive until mConsumers is deleted. > > But now, if any caller forgets to call RemoveConsumer we'll have a UAF. > > I think we need some more assurances here that this is safe. A comment at least, and maybe some asserts. > > For instance, I don't see any MOZ_ASSERTS that mConsumers.Length() == 0 on destruct here. > > Is there an overall lifetime invariant here that makes this safe? If so, please add comment. Agreed, this is fragile. I'll try to make MediaStreamTrackConsumer support WeakPtr.

Comment on attachment 8921438 [details] Bug 1407542 - Implement static chrome-only MediaStream method to get number of MSG-MediaStreams. https://reviewboard.mozilla.org/r/192468/#review197664 > I guess this is a coding style which hasn't been decided yet. > I'd put + to the end of the previous line and then align mGraphs, but up to you Yeah, I conflated this with the js style. > Is it guaranteed that mGraph stays alive long enough. I assume yes, but this needs a comment. Every raw pointer as a member variable basically needs explanation why it is safe to use. We've had tons of security critical issues because of use of raw pointers. Yes, mGraph is owning the `UniquePtr<Counter>`. Comment added.

(In reply to Andreas Pehrson [:pehrsons] from comment #16) > > Is it guaranteed that mGraph stays alive long enough. I assume yes, but this needs a comment. Every raw pointer as a member variable basically needs explanation why it is safe to use. We've had tons of security critical issues because of use of raw pointers. > > Yes, mGraph is owning the `UniquePtr<Counter>`. Comment added. How does that guarantee mGraph stays alive? If the graph goes away, the UniquePtr<Counter> will go away too.

(In reply to Olli Pettay [:smaug] from comment #21) > (In reply to Andreas Pehrson [:pehrsons] from comment #16) > > > Is it guaranteed that mGraph stays alive long enough. I assume yes, but this needs a comment. Every raw pointer as a member variable basically needs explanation why it is safe to use. We've had tons of security critical issues because of use of raw pointers. > > > > Yes, mGraph is owning the `UniquePtr<Counter>`. Comment added. > How does that guarantee mGraph stays alive? If the graph goes away, the > UniquePtr<Counter> will go away too. Indeed, Counter is a ControlMessage that controls the graph, not a runnable. The graph has full ownership and is what will call Run(), so if it goes away, there's nothing left to access neither the Counter instance, or mGraph. Should one want to do something in Counter before the graph is destroyed, one can implement RunDuringShutdown, [1]. However, we don't have to. The nsMainThreadPtrHandle takes care of cleanup here. [1] http://searchfox.org/mozilla-central/rev/1285ae3e810e2dbf2652c49ee539e3199fcfe820/dom/media/MediaStreamGraphImpl.h#78

Comment on attachment 8921437 [details] Bug 1407542 - Implement MediaStreamGraph::GetInstanceIfExists. https://reviewboard.mozilla.org/r/192466/#review198990 ::: dom/media/MediaStreamGraph.cpp:3507 (Diff revision 1) > > - MediaStreamGraphImpl* graph = nullptr; > - > // We hash the nsPIDOMWindowInner to form a key to the gloabl > // MediaStreamGraph hashtable. Effectively, this means there is a graph per > // document. Maybe it's best to move this comment somewhere better than the callsite, now that we have more than one caller.

Comment on attachment 8917284 [details] Bug 1407542 - Remove back reference to consumer in MediaStreamTrack. https://reviewboard.mozilla.org/r/188300/#review198330 Looks great! Except I think we should assert rather than clean up after bad code. ::: dom/media/MediaStreamTrack.cpp:377 (Diff revisions 2 - 4) > - for (int32_t i = mConsumers.Length() - 1; i >= 0; --i) { > - // Loop backwards by index in case the consumer removes itself in the > - // callback. > - mConsumers[i]->NotifyEnded(this); > + auto consumers(mConsumers); > + for (const auto& consumer : consumers) { > + if (consumer) { > + consumer->NotifyEnded(this); > + } else { > + mConsumers.RemoveElement(consumer); I think we should MOZ_ASSERT here, since not calling RemoveConsumer() is still technically a programming error. ::: dom/media/MediaStreamTrack.cpp:402 (Diff revisions 2 - 5) > + // Remove destroyed consumers for cleanliness > + while (mConsumers.RemoveElement(nullptr)) {} I had to check that this actually does something, and it does! Nice. However, I wonder if we should MOZ_ASSERT again instead, since if this ever removes an element it means someone didn't call RemoveConsumer() when they should have. ::: dom/media/MediaStreamTrack.cpp:411 (Diff revisions 2 - 5) > + // Remove destroyed consumers for cleanliness > + while (mConsumers.RemoveElement(nullptr)) {} same here

Comment on attachment 8917284 [details] Bug 1407542 - Remove back reference to consumer in MediaStreamTrack. https://reviewboard.mozilla.org/r/188300/#review198330 > I had to check that this actually does something, and it does! Nice. > > However, I wonder if we should MOZ_ASSERT again instead, since if this ever removes an element it means someone didn't call RemoveConsumer() when they should have. To clarify, I mean use WeakPtrs *and* MOZ_ASSERT, to throw in debug build and avoid UAF in opt build.

Pushed by pehrsons@gmail.com: https://hg.mozilla.org/integration/autoland/rev/fc681729ccc8 Implement MediaStreamGraph::GetInstanceIfExists. r=padenot https://hg.mozilla.org/integration/autoland/rev/858589325edc Implement static chrome-only MediaStream method to get number of MSG-MediaStreams. r=jib,smaug https://hg.mozilla.org/integration/autoland/rev/21762797d5ec Add mochitest checking that MediaStreams can be GCed. r=jib https://hg.mozilla.org/integration/autoland/rev/5cb403db2f66 Remove back reference to consumer in MediaStreamTrack. r=jib,smaug

https://hg.mozilla.org/mozilla-central/rev/fc681729ccc8 https://hg.mozilla.org/mozilla-central/rev/858589325edc https://hg.mozilla.org/mozilla-central/rev/21762797d5ec https://hg.mozilla.org/mozilla-central/rev/5cb403db2f66