Closed Bug 1407542 Opened 7 years ago Closed 7 years ago

Garbage collection of MediaStreams doesn't always work

Categories

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

Product:
Core
Component:
Audio/Video: MediaStreamGraph
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: pehrsons, Assigned: pehrsons)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

Bug 1407542 - Remove back reference to consumer in MediaStreamTrack.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1407542 - Implement MediaStreamGraph::GetInstanceIfExists.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1407542 - Implement static chrome-only MediaStream method to get number of MSG-MediaStreams.
59 bytes, text/x-review-board-request
smaug
: review+
jib
: review+
Details
Bug 1407542 - Add mochitest checking that MediaStreams can be GCed.
59 bytes, text/x-review-board-request
jib
: review+
Details 
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
Blocks: 1405893
No longer blocks: 1405983
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.
Flags: needinfo?(jib)
Status: NEW → ASSIGNED
Rank: 17
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
Flags: needinfo?(jib)
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.
Attachment #8921438 - Flags: review?(bugs) → review+
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.
Attachment #8917284 - Flags: review?(jib) → review-
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?
Attachment #8921438 - Flags: review?(jib) → review+
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.
Attachment #8917284 - Flags: review?(bugs) → review+
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
Attachment #8921439 - Flags: review?(jib) → review+
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
No longer blocks: 1410829
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.
Attachment #8921437 - Flags: review?(padenot) → review+
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
Attachment #8917284 - Flags: review?(jib) → review+
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
Depends on: 1513638
Blocks: 1593572
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: