Closed Bug 1434600 Opened 6 years ago Closed 6 years ago

MediaManager and MediaEngine*Source are being kept alive until the process exits

Categories

(Core :: WebRTC: Audio/Video, enhancement, P2)

Product:
Core
Component:
WebRTC: Audio/Video
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: padenot, Assigned: padenot)

References

Details

Attachments

(3 files)

Bug 1434600 - Plumb the window ID down to Enumerate{Audio,Video}Devices.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1434600 - Add a method to clean up by window ID, on a MediaEngine.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1434600 - Clean up the MediaEngineWebRTC*Source when navigating away from a document.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details 
This is rather significant: at least a thread and a bunch of big buffers and handles in the *Sources.
The sources should be cleared. They are managed by the SourceListener class to which there are three references (modulo a couple of short-lived runnables):

1 MediaManager through the active window table
2 MediaStreamGraph since SourceListener is a MediaStreamListener attached to a SourceMediaStream
3 MediaStreamTrack through MediaStreamTrackSource

1 is cleared on navigation through [1]->[2]->[3]
2 is cleared after all MediaStreamTracks have stopped through [4]->[5]->[6]->[7]. This also clears the window listener's reference to the SourceListener (from 1).
3 is cleared when the MediaStreamTrack is destroyed, [8]

I take it one of these is still alive because of an obscure ref-cycle or something.

Also note that with bug 1299515 reference nr 3 becomes a WeakPtr, so that reduces it to two references.


[1] https://searchfox.org/mozilla-central/rev/c56f656febb1aa58c72b218699e24a76425bd284/dom/media/MediaManager.cpp#2970
[2] https://searchfox.org/mozilla-central/rev/c56f656febb1aa58c72b218699e24a76425bd284/dom/media/MediaManager.cpp#2944
[3] https://searchfox.org/mozilla-central/rev/c56f656febb1aa58c72b218699e24a76425bd284/dom/media/MediaManager.cpp#424

[4] https://searchfox.org/mozilla-central/rev/c56f656febb1aa58c72b218699e24a76425bd284/dom/media/MediaManager.cpp#3799
[5] https://searchfox.org/mozilla-central/rev/c56f656febb1aa58c72b218699e24a76425bd284/dom/media/MediaManager.cpp#3721
[6] https://searchfox.org/mozilla-central/rev/c56f656febb1aa58c72b218699e24a76425bd284/dom/media/MediaManager.cpp#3905-3908
[7] https://searchfox.org/mozilla-central/rev/c56f656febb1aa58c72b218699e24a76425bd284/dom/media/MediaManager.cpp#3942

[8] https://searchfox.org/mozilla-central/rev/97cb0aa64ae51adcabff76fb3b5eb18368f5f8ab/dom/media/MediaStreamTrack.h#557
Taking for investigation.
Assignee: nobody → padenot
I had missed that MediaManager has an mBackend member (the MediaEngineWebRTC class) that just keeps all MediaEngineSources it has created around until shutdown.

Eeeh, it seems to have been like this forever? Is there any real reason for enumerateDevices to do this, essentially caching, jib?

https://searchfox.org/mozilla-central/rev/c56f656febb1aa58c72b218699e24a76425bd284/dom/media/webrtc/MediaEngineWebRTC.cpp#326
Flags: needinfo?(jib)
Rank: 15
Priority: -- → P2
This does not include code to destroy MediaManager early, I'll do that later.
Comment on attachment 8947866 [details]
Bug 1434600 - Clean up the MediaEngineWebRTC*Source when navigating away from a document.

https://reviewboard.mozilla.org/r/217540/#review223318


Static analysis found 1 defect in this patch.
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/MediaManager.cpp:2980
(Diff revision 1)
>    MOZ_ASSERT(!GetWindowListener(aWindowID));
>  
>    RemoveMediaDevicesCallback(aWindowID);
> +
> +  MediaManager::PostTask(NewTaskFrom([this, aWindowID]() {
> +    this->GetBackend()->ReleaseResourcesForWindow(aWindowID);

Error: Refcounted variable 'this' of type 'mozilla::mediamanager' cannot be captured by a lambda [clang-tidy: mozilla-refcounted-inside-lambda]
FYI, comment 8
Flags: needinfo?(padenot)
AFAIK it's been like this forever since before we had multiple content processes. MediaManager is lazy-loaded but then basically stays around until shutdown (which with content processes can be more often than before in some cases, but not for tab hoarders). Yes this was for caching. The alternative is to tear down media manager and the media thread after every gUM call.

When you say "thread and a bunch of big buffers" is that the media thread or additional threads? I wasn't aware the source itself kept a big buffer.

Tearing down mBackend sounds like a good first step. I think this would be great to fix provided it doesn't turn into a rabbit hole. Provided this is contained to the media thread, it should be safe too. 

Looking at lifetimes might shed more light on our shutdown situation as well.
Flags: needinfo?(jib)
(In reply to Andreas Pehrson [:pehrsons] from comment #3)
> Eeeh, it seems to have been like this forever? Is there any real reason for
> enumerateDevices to do this, essentially caching, jib?

Oh and yes, enumerateDevices is a good point. It can take quite a while to finish the first time on some platforms apparently.

The spec advises web sites to use enumerateDevices() ahead of getUserMedia() [1][2], so without caching we'd be basically enumerating twice (because getUserMedia internally has to call enumerateDevices to populate the resulting tracks with their deviceIds).

[1] To support cam-only and mic-only users https://stackoverflow.com/a/33770858/918910
[2] To support in-content cam + mic selection https://webrtc.github.io/samples/src/content/devices/input-output/
> to populate the resulting tracks with their deviceIds

As well as to resolve any passed-in deviceId constraints.
> Oh and yes, enumerateDevices is a good point. It can take quite a while to finish the first time on some platforms apparently.

There was a bug on stopping MediaManager staying alive until shutdown, but I can't find it.  We talked about having it shut down after N seconds of no use, to avoid churn in the "normal" sequence.  (Especially as at one time, initial device scan was a few seconds at least on linux as it tries all the different possible video sources (and audio sources, but that's cubeb now)).
(In reply to Randell Jesup [:jesup] from comment #13)
> > Oh and yes, enumerateDevices is a good point. It can take quite a while to finish the first time on some platforms apparently.
> 
> There was a bug on stopping MediaManager staying alive until shutdown, but I
> can't find it.  We talked about having it shut down after N seconds of no
> use, to avoid churn in the "normal" sequence.  (Especially as at one time,
> initial device scan was a few seconds at least on linux as it tries all the
> different possible video sources (and audio sources, but that's cubeb now)).

It should be addrefed by the globals using it, that's it.
Flags: needinfo?(padenot)
(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #10)
> AFAIK it's been like this forever since before we had multiple content
> processes. MediaManager is lazy-loaded but then basically stays around until
> shutdown (which with content processes can be more often than before in some
> cases, but not for tab hoarders). Yes this was for caching. The alternative
> is to tear down media manager and the media thread after every gUM call.
> 
> When you say "thread and a bunch of big buffers" is that the media thread or
> additional threads? I wasn't aware the source itself kept a big buffer.

I was talking about the MediaManager thread, and the fact that MediaEngineWebRTC*Source are enormous objects that we don't want to keep around if not needed.

If the sound has been started, then it has a number of buffers that are being used for the various DSP operations and up/down mixing/conversion, all that kind of things.

> Tearing down mBackend sounds like a good first step. I think this would be
> great to fix provided it doesn't turn into a rabbit hole. Provided this is
> contained to the media thread, it should be safe too. 

No it's finished already, it's the least we can do here.
Comment on attachment 8947864 [details]
Bug 1434600 - Plumb the window ID down to Enumerate{Audio,Video}Devices.

https://reviewboard.mozilla.org/r/217542/#review223552
Attachment #8947864 - Flags: review?(apehrson) → review+
Comment on attachment 8947865 [details]
Bug 1434600 - Add a method to clean up by window ID, on a MediaEngine.

https://reviewboard.mozilla.org/r/217544/#review223554

::: dom/media/webrtc/MediaEngineDefault.h:211
(Diff revision 1)
> +  void ReleaseResourcesForWindow(uint64_t aWindowId)
> +  { }

Why not do it here too? fake:true is exposed to content and can create these devices.

::: dom/media/webrtc/MediaEngineWebRTC.h:601
(Diff revision 1)
>    // Store devices we've already seen in a hashtable for quick return.
> -  // Maps UUID to MediaEngineSource (one set for audio, one for video).
> -  nsRefPtrHashtable<nsStringHashKey, MediaEngineVideoSource> mVideoSources;
> -  nsRefPtrHashtable<nsStringHashKey, MediaEngineAudioSource> mAudioSources;
> +  // Maps WindowID to a map of device uuid to their MediaEngineSource,
> +  // separately for audio and video.
> +  nsClassHashtable<nsUint64HashKey,
> +                    nsRefPtrHashtable<nsStringHashKey,
> +                                      MediaEngineVideoSource>> mVideoSources;
> +  nsClassHashtable<nsUint64HashKey,
> +                    nsRefPtrHashtable<nsStringHashKey,
> +                                      MediaEngineAudioSource>> mAudioSources;

So when we implement MediaManager-per-document we can remove this yes? Since this is basically making MediaEngineSources per document for now.

Do we have a bug number for that work that we can mention here?

::: dom/media/webrtc/MediaEngineWebRTC.cpp:135
(Diff revision 1)
>    camera::GetChildAndCall(
>      &camera::CamerasChild::SetFakeDeviceChangeEvents);
>  }
>  
>  void
> -MediaEngineWebRTC::EnumerateVideoDevices(uint64_t aWindowID,
> +MediaEngineWebRTC::EnumerateVideoDevices(uint64_t aWindowId,

So this should go into the first patch.

::: dom/media/webrtc/MediaEngineWebRTC.cpp:354
(Diff revision 1)
> +  if (audioDevicesForThisWindow) {
> +    for (auto iter = audioDevicesForThisWindow->Iter(); !iter.Done(); iter.Next()) {
> +      iter.UserData()->Shutdown();
> +    }
> +
> +    audioDevicesForThisWindow->Clear();

Seems redundant.

::: dom/media/webrtc/MediaEngineWebRTC.cpp:366
(Diff revision 1)
> +  if (videoDevicesForThisWindow) {
> +    for (auto iter = videoDevicesForThisWindow->Iter(); !iter.Done(); iter.Next()) {
> +      iter.UserData()->Shutdown();
> +    }
> +
> +    videoDevicesForThisWindow->Clear();

Also redundant

::: dom/media/webrtc/MediaEngineWebRTC.cpp:385
(Diff revision 1)
>    for (auto iter = mVideoSources.Iter(); !iter.Done(); iter.Next()) {
> -    MediaEngineVideoSource* source = iter.UserData();
> +    for (auto iterInner = iter.UserData()->Iter(); !iterInner.Done(); iterInner.Next()) {
> +      MediaEngineVideoSource* source = iterInner.UserData();
> -    if (source) {
> +      if (source) {
> -      source->Shutdown();
> +        source->Shutdown();
> -    }
> +      }
> -  }
> +    }
> +  }
>    for (auto iter = mAudioSources.Iter(); !iter.Done(); iter.Next()) {
> -    MediaEngineAudioSource* source = iter.UserData();
> +    for (auto iterInner = iter.UserData()->Iter(); !iterInner.Done(); iterInner.Next()) {
> +      MediaEngineAudioSource* source = iterInner.UserData();
> -    if (source) {
> +      if (source) {
> -      source->Shutdown();
> +        source->Shutdown();
> -    }
> +      }
> -  }
> +    }
> +  }

Could we keep these simpler by calling ReleaseResourcesForWindow() with the key for the outer iterator's key?

One path shutting down a MediaEngineSource vs two.

::: dom/media/webrtc/MediaEngineWebRTC.cpp:388
(Diff revision 1)
>    // Shutdown all the sources, since we may have dangling references to the
>    // sources in nsDOMUserMediaStreams waiting for GC/CC
>    for (auto iter = mVideoSources.Iter(); !iter.Done(); iter.Next()) {
> -    MediaEngineVideoSource* source = iter.UserData();
> +    for (auto iterInner = iter.UserData()->Iter(); !iterInner.Done(); iterInner.Next()) {
> +      MediaEngineVideoSource* source = iterInner.UserData();
> -    if (source) {
> +      if (source) {

When can `source` be null?
Attachment #8947865 - Flags: review?(apehrson) → review+
Comment on attachment 8947866 [details]
Bug 1434600 - Clean up the MediaEngineWebRTC*Source when navigating away from a document.

https://reviewboard.mozilla.org/r/217540/#review223558

::: dom/media/MediaManager.cpp:2979
(Diff revision 1)
>    }
>    MOZ_ASSERT(!GetWindowListener(aWindowID));
>  
>    RemoveMediaDevicesCallback(aWindowID);
> +
> +  MediaManager::PostTask(NewTaskFrom([this, aWindowID]() {

I think you can do `[this, self = RefPtr<MediaManager>(this), aWindowID]` without static analysis complaining.

::: dom/media/MediaManager.cpp:2980
(Diff revision 1)
>    MOZ_ASSERT(!GetWindowListener(aWindowID));
>  
>    RemoveMediaDevicesCallback(aWindowID);
> +
> +  MediaManager::PostTask(NewTaskFrom([this, aWindowID]() {
> +    this->GetBackend()->ReleaseResourcesForWindow(aWindowID);

Or maybe it's complaining that you're using `this`. If you capture `this` you can just call GetBackend() directly as if you were in a method.
Attachment #8947866 - Flags: review?(apehrson) → review+
Comment on attachment 8947866 [details]
Bug 1434600 - Clean up the MediaEngineWebRTC*Source when navigating away from a document.

https://reviewboard.mozilla.org/r/217540/#review223580


Static analysis found 1 defect in this patch.
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/MediaManager.cpp:3148
(Diff revision 2)
>    MOZ_ASSERT(!GetWindowListener(aWindowID));
>  
>    RemoveMediaDevicesCallback(aWindowID);
> +
> +  MediaManager::PostTask(NewTaskFrom([this, aWindowID]() {
> +    this->GetBackend()->ReleaseResourcesForWindow(aWindowID);

Error: Refcounted variable 'this' of type 'mozilla::mediamanager' cannot be captured by a lambda [clang-tidy: mozilla-refcounted-inside-lambda]
Comment on attachment 8947865 [details]
Bug 1434600 - Add a method to clean up by window ID, on a MediaEngine.

https://reviewboard.mozilla.org/r/217544/#review223604

::: dom/media/webrtc/MediaEngineWebRTC.cpp:254
(Diff revision 2)
>          static_cast<MediaEngineRemoteVideoSource*>(vSource.get())->Refresh(i);
>          aSources->AppendElement(vSource.get());
>        } else {
>          vSource = new MediaEngineRemoteVideoSource(i, capEngine, aMediaSource,
>                                                     scaryKind || scarySource);
> -        mVideoSources.Put(uuid, vSource);
> +        devicesForThisWindow->Put(uuid, vSource); // Hashtable takes ownership.

It's a RefPtr so ownership is implicit. IMO you can remove the comment.
Comment on attachment 8947865 [details]
Bug 1434600 - Add a method to clean up by window ID, on a MediaEngine.

https://reviewboard.mozilla.org/r/217544/#review223554

> Why not do it here too? fake:true is exposed to content and can create these devices.

You're right. I've fixed it, using mostly the same approch, using an array rather than keying on the UUID.

> So when we implement MediaManager-per-document we can remove this yes? Since this is basically making MediaEngineSources per document for now.
> 
> Do we have a bug number for that work that we can mention here?

I think so. However, this is good enough for now. Having a MediaManager per Window is a bit more complex, and I need to read some webcam code to be confident enough.

For those reasons, I don't have a bug number.

> So this should go into the first patch.

This was fixed in the version that I pushed a second after you reviewed.

> Seems redundant.

True, fixed.

> Could we keep these simpler by calling ReleaseResourcesForWindow() with the key for the outer iterator's key?
> 
> One path shutting down a MediaEngineSource vs two.

I've used a templated helper function.

> When can `source` be null?

I don't think so, I've removed this.
Comment on attachment 8947864 [details]
Bug 1434600 - Plumb the window ID down to Enumerate{Audio,Video}Devices.

https://reviewboard.mozilla.org/r/217542/#review223862

::: dom/media/webrtc/MediaEngine.h:47
(Diff revision 2)
>  
>    /**
>     * Populate an array of sources of the requested type in the nsTArray.
>     * Also include devices that are currently unavailable.
>     */
> -  virtual void EnumerateDevices(dom::MediaSourceEnum,
> +  virtual void EnumerateDevices(uint64_t aWindowID,

nit: all of them say "Id" except this one.

::: dom/media/webrtc/MediaEngineDefault.cpp:561
(Diff revision 2)
>        // We once had code here to find a VideoSource with the same settings and
>        // re-use that. This is no longer possible since the resolution gets set
>        // in Allocate().
> -

This comment is for a block that used to exist here and not for the code below so I think keeping the empty line is the right thing.
Comment on attachment 8947865 [details]
Bug 1434600 - Add a method to clean up by window ID, on a MediaEngine.

https://reviewboard.mozilla.org/r/217544/#review223864

This went a bit overboard :-)

Consider them suggestions.

::: dom/media/webrtc/MediaEngine.h:51
(Diff revision 3)
>     */
>    virtual void EnumerateDevices(uint64_t aWindowID,
>                                  dom::MediaSourceEnum,
>                                  nsTArray<RefPtr<MediaEngineSource>>*) = 0;
>  
> +  virtual void ReleaseResourcesForWindow(uint64_t aWindowID) = 0;

..and this one also deviates from "Id".

::: dom/media/webrtc/MediaEngineDefault.cpp:557
(Diff revision 3)
>                                       dom::MediaSourceEnum aMediaSource,
>                                       nsTArray<RefPtr<MediaEngineSource>>* aSources)
>  {
>    AssertIsOnOwningThread();
>  
> +

nit: superfluous line

::: dom/media/webrtc/MediaEngineWebRTC.cpp:340
(Diff revision 3)
>  }
>  
>  void
> +MediaEngineWebRTC::ReleaseResourcesForWindow(uint64_t aWindowId)
> +{
> +  nsRefPtrHashtable<nsStringHashKey, MediaEngineSource>*

Let's do something about the scope of this. After `mAudioSource.Remove(aWindowId)` this will be a footgun primed for a UAF.

We could put this decl in the if-condition no? And put a comment at the Remove() maybe.

::: dom/media/webrtc/MediaEngineWebRTC.cpp:348
(Diff revision 3)
> +  if (audioDevicesForThisWindow) {
> +    for (auto iter = audioDevicesForThisWindow->Iter(); !iter.Done(); iter.Next()) {
> +      iter.UserData()->Shutdown();
> +    }
> +
> +    audioDevicesForThisWindow->Clear();

Unnecessary since `audioDevicesForThisWindow` is owned by the hashtable and will be destructed on the next line.

::: dom/media/webrtc/MediaEngineWebRTC.cpp:365
(Diff revision 3)
> +namespace {
> +template<typename T>
> +void ShutdownSources(T& aHashTable)

Why the namespace and not a static function?

::: dom/media/webrtc/MediaEngineWebRTC.cpp:367
(Diff revision 3)
> +  }
> +}
> +
> +namespace {
> +template<typename T>
> +void ShutdownSources(T& aHashTable)

Should we make one for the inner loop too? Say ShutdownSourcesForDevice and ShutdownSourcesForWindow.
(In reply to Andreas Pehrson [:pehrsons] from comment #30)
> ::: dom/media/webrtc/MediaEngineWebRTC.cpp:365
> (Diff revision 3)
> > +namespace {
> > +template<typename T>
> > +void ShutdownSources(T& aHashTable)
> 
> Why the namespace and not a static function?

It used to matter (static functions in C++ were about to be deprecated), but it does not anymore (the committee decided not to deprecate them). Just a matter of style I suppose.

> 
> ::: dom/media/webrtc/MediaEngineWebRTC.cpp:367
> (Diff revision 3)
> > +  }
> > +}
> > +
> > +namespace {
> > +template<typename T>
> > +void ShutdownSources(T& aHashTable)
> 
> Should we make one for the inner loop too? Say ShutdownSourcesForDevice and
> ShutdownSourcesForWindow.

Yeah ok.
(In reply to Paul Adenot (:padenot) from comment #31)
> (In reply to Andreas Pehrson [:pehrsons] from comment #30)
> > ::: dom/media/webrtc/MediaEngineWebRTC.cpp:367
> > (Diff revision 3)
> > > +  }
> > > +}
> > > +
> > > +namespace {
> > > +template<typename T>
> > > +void ShutdownSources(T& aHashTable)
> > 
> > Should we make one for the inner loop too? Say ShutdownSourcesForDevice and
> > ShutdownSourcesForWindow.
> 
> Yeah ok.

In fact, this will not remove any duplication, would it ?
(In reply to Paul Adenot (:padenot) from comment #32)
> (In reply to Paul Adenot (:padenot) from comment #31)
> > (In reply to Andreas Pehrson [:pehrsons] from comment #30)
> > > ::: dom/media/webrtc/MediaEngineWebRTC.cpp:367
> > > (Diff revision 3)
> > > > +  }
> > > > +}
> > > > +
> > > > +namespace {
> > > > +template<typename T>
> > > > +void ShutdownSources(T& aHashTable)
> > > 
> > > Should we make one for the inner loop too? Say ShutdownSourcesForDevice and
> > > ShutdownSourcesForWindow.
> > 
> > Yeah ok.
> 
> In fact, this will not remove any duplication, would it ?

Wouldn't it cover the two cases in ReleaseResourcesForWindow and the one in ShutdownSources?

Oh, I see. Make it s/ShutdownSourcesForDevice/ShutdownAllSources/. Or just ShutdownSources. Or leave it all explicit, it's good for the indirection anyway.
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/autoland/rev/f0219139820a
Plumb the window ID down to Enumerate{Audio,Video}Devices. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/88520bbb4354
Add a method to clean up by window ID, on a MediaEngine. r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/b6828f737104
Clean up the MediaEngineWebRTC*Source when navigating away from a document. r=pehrsons
See Also: → 1734267
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: