819867 - localMediaStreams should stop() when there are no active references

Status: RESOLVED INVALID

(Core :: WebRTC: Audio/Video, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

The camera/mic capture should be released as soon as we have no referrers.  There's still an issue with cycles, in which case the camera will remain active until it's cycle-collected, but we still should release the camera as soon as we can and try to arrange the code to avoid cycles if at all possible.

URL.createObjectURL(stream) is a problem.  This creates a reference that needs to live as long as that URL could be assigned to a video/etc element.  I'm assuming for now that this reference is handled as a strong ref and so we don't need to make any exceptions for it here.  (And hopefully it has "use-once" semantics similar to Chrome's so it "gives up" its ref when it's assigned to sink, and is not usable again - otherwise you MUST call stop() manually to release the mic/camera.)

Note that Chrome and Opera only activate the camera when it's assigned to a sink, and deactivate it when the sink dies or is disconnected from them (default use-once semantics for URLs from createObjectURL - not sure what they do/will do for mozSrcObject)

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Randell, can this be classified as a leak?

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

No, since they'll eventually be released when GC/CC happen.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

The sequence here in JS is roughly:
...
   navigator.mozGetUserMedia({video:true},function(stream) {
      video.mozSrcObject = stream;
   }, function(err) {});

...
   video.mozSrcObject = null;

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

So stream should die during the next GC. (Stream is not even a wrappercache object atm).
Or does something in C++ keep it alive?

Or are you saying you want to detect that = null; case and stop asap even before gc/cc?
If Stream was a wrappercache object, you could easily check whether it has js wrapper, but
expando properties would make the wrapper to stay there until next gc/cc.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Not much we can do here... Turns out there's no easy way to know.  Bummer

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Maybe we could start the timer to run GC/CC when video.mozSrcObject is set to null?
See PokeGC

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

How long should the delay be?
In the common case where no other references are present, I would think the delay should be very short (or is a delay even needed?). If there's another JS reference, then hopefully that's someone coding specifically toward Firefox and knows to call stop() once they're done with the stream.

Comment 8

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

(In reply to Randell Jesup [:jesup] from comment #0)
> URL.createObjectURL(stream) is a problem.  This creates a reference that
> needs to live as long as that URL could be assigned to a video/etc element. 
> I'm assuming for now that this reference is handled as a strong ref and so
> we don't need to make any exceptions for it here.  (And hopefully it has
> "use-once" semantics similar to Chrome's so it "gives up" its ref when it's
> assigned to sink, and is not usable again - otherwise you MUST call stop()
> manually to release the mic/camera.)

Really, Chrome's createObjectURL autorevokes by default? My understanding was it did not.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #7)
> In the common case where no other references are present, I would think the
> delay should be very short (or is a delay even needed?). If there's another
> JS reference, then hopefully that's someone coding specifically toward
> Firefox and knows to call stop() once they're done with the stream.

I'm not sure what you mean here.

Basically, relying on GC, a.k.a. "dropping all references" for immediate cleanup of resources is not a good idea and we shouldn't do it.

We could do something else. For example, we could say that if a getUserMedia stream is not connected (directly or indirectly) to a sink such as a PeerConnection or media element, then we stop capturing until it is reconnected.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> (In reply to Randell Jesup [:jesup] from comment #0)
> > URL.createObjectURL(stream) is a problem.  This creates a reference that
> > needs to live as long as that URL could be assigned to a video/etc element. 
> > I'm assuming for now that this reference is handled as a strong ref and so
> > we don't need to make any exceptions for it here.  (And hopefully it has
> > "use-once" semantics similar to Chrome's so it "gives up" its ref when it's
> > assigned to sink, and is not usable again - otherwise you MUST call stop()
> > manually to release the mic/camera.)
> 
> Really, Chrome's createObjectURL autorevokes by default? My understanding
> was it did not.

I think you're right - I don't think it does (though they proposed that IIRC). It does turn off the camera/mic when there are no consumers.  (Though this means the temporary leaks you were concerned about due to createObjectURL exist.)

> 
> (In reply to Jan-Ivar Bruaroey [:jib] from comment #7)
> > In the common case where no other references are present, I would think the
> > delay should be very short (or is a delay even needed?). If there's another
> > JS reference, then hopefully that's someone coding specifically toward
> > Firefox and knows to call stop() once they're done with the stream.
> 
> I'm not sure what you mean here.
> 
> Basically, relying on GC, a.k.a. "dropping all references" for immediate
> cleanup of resources is not a good idea and we shouldn't do it.

Right.  The spec says they should call stop() to turn off the mic/camera and revoke access; it doesn't talk to what happens if you don't.
 
> We could do something else. For example, we could say that if a getUserMedia
> stream is not connected (directly or indirectly) to a sink such as a
> PeerConnection or media element, then we stop capturing until it is
> reconnected.

That's effectively what chrome does.  Problems:
1) Privacy/Security:  An app could "finish" and turn off the camera (and light), but retain the mediastream and renable it (snoop) at any time.  Some cameras don't have lights, others ramp them up/down so you can take snapshots without showing, and mics rarely have lights.  We do have the new chrome indicator of media access that would remain on while the page has access, but it's arguable if that alone is sufficient.

2) I'm concerned that some cameras take seconds to enable, so for uses where something does want to disconnect and reconnect a mediastream to a different element/peerconnection/etc, you could end up with long delays and a bad UX.  The alternative would be contortions to avoid ever having it appear disconnected, or timeouts to allow moves from one element to another not to trigger stopping, which seems kludgy and error-prone to me.

All that said, as I said to Jan-Ivar and smaug, we may have to simply act like chrome.  Right now I'd rather try to enforce/encourage use of stop() if we can, maybe targeted at poking developers when they test code that forgets to do so.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Still relevant?

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I think we're good here. All browsers today open devices on gUM success, not on assignment to sink, so comment 9 is not an issue.

All browsers also require apps to call stop() on all tracks to close devices and have privacy indicators and lights goes out. If stop() isn't called, Chrome and Firefox close devices on GC/CC, whereas Edge and Safari appear not to.

This seems good for privacy. Apps seem to be good at calling stop() and there doesn't appear to be much value in optimizing sloppy apps.