Closed Bug 802538 Opened 9 years ago Closed 9 years ago

mozGetUserMedia streams don't get destroyed

Categories

(Core :: WebRTC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: jesup, Assigned: jesup)

References

Details

(Whiteboard: [getUserMedia][blocking-gum+][qa-])

Attachments

(1 file, 1 obsolete file)

Likely lack of proper cycle collection marking....

Go into the gum Video test, stop, leave the page (etc).  Note that if MediaStreamGraph:5 logging is on, you see 

620635904[7f192f1c6be0]: MediaStream 7f194e3403e0 is finished, but not blocked yet (end at 8796093022208.000000, with blocking at 8796093022208.000000)
620635904[7f192f1c6be0]: Media graph 7f192c2f8580 computed blocking for interval 2.842187 to 2.852311
620635904[7f192f1c6be0]: Waiting for next iteration; at 2.822486, timeout=0.010000
620635904[7f192f1c6be0]: Resuming after timeout; at 2.832575, elapsed=0.010089
620635904[7f192f1c6be0]: Updating current time to 2.832469 (real 2.832597, mStateComputedTime 2.852311)
620635904[7f192f1c6be0]: MediaStream 7f194e3403e0 bufferStartTime=0.181739 blockedTime=0.000000
620635904[7f192f1c6be0]: Media graph 7f192c2f8580 computing blocking for time 2.852311

repeating ad infinitum.  And of course they accumulate.
It's a specific instance of such a leak
Makes sense. Lets add it as dependency.
Blocks: 798323
A random question off of this bug - how exactly do I get logging on? A debug build?

I would gladly keep a watch out for these leaks while testing if I know exactly what I'm watching out for.
I can be setup similar to HTTP logging as described here:
https://developer.mozilla.org/en/HTTP_Logging
(In reply to Henrik Skupin (:whimboo) from comment #5)
> I can be setup similar to HTTP logging as described here:
> https://developer.mozilla.org/en/HTTP_Logging

Hmm. I tried reproducing this bug with HTTP logging on, but I'm seeing anything in regards to MediaStream showing up in the log.
(In reply to Jason Smith [:jsmith] from comment #6)
> (In reply to Henrik Skupin (:whimboo) from comment #5)
> > I can be setup similar to HTTP logging as described here:
> > https://developer.mozilla.org/en/HTTP_Logging
> 
> Hmm. I tried reproducing this bug with HTTP logging on, but I'm seeing
> anything in regards to MediaStream showing up in the log.

"Not seeing anything" is what I meant to say
As noted in comment 0 you have to use 'MediaStreamGraph:5'
Priority: -- → P2
Whiteboard: [getUserMedia], [blocking-gum+]
Priority: P2 → P1
Need to verify the LocalMediaStreams get destroyed.  If so, then the issue will be why aren't the underlying streams Finished/removed.  If not, it's a refcount issue elsewhere
Assignee: nobody → rjesup
Don't think there's an explicit test needed here, although other automation for gum might end up covering this with the leak checker that happens in CI.
Flags: in-testsuite-
Blocks: 799417
Comment on attachment 695910 [details] [diff] [review]
Make sure gUM MediaStreams are destroyed (and fix other leaks)

Mochitest results (which includes peerconnection tests):
http://pastebin.mozilla.org/2023207

I see no leaks currently in basic gum hand-testing
Attachment #695910 - Flags: review?(tterribe)
Duplicate of this bug: 798653
With the mochitests limited to the two gum tests, it completes with no leaks!
Attachment #696202 - Flags: review?(hskupin)
Blocks: 814807
Comment on attachment 696202 [details] [diff] [review]
Enable getUserMedia() mochitests

Looks fine. If you could rearrange the tests so they are in an alphabetical order it would be appreciated. Otherwise good to see the progress and a green testrun with the leak-fix patch! Thanks!
Attachment #696202 - Flags: review?(hskupin) → review+
Comment on attachment 695910 [details] [diff] [review]
Make sure gUM MediaStreams are destroyed (and fix other leaks)

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

r=me, but please address the follow-up.

::: dom/media/MediaManager.h
@@ +77,2 @@
>  // Generic class for running long media operations off the main thread, and
>  // then (because nsDOMMediaStreams aren't threadsafe), re-sends itseld to

The comment here appears to now be completely incorrect (and also typo'd).

@@ +226,5 @@
>  
>      // We can't take a chance on blocking here, so proxy this to another
>      // thread.
>      // XXX FIX! I'm cheating and passing a raw pointer to the sourcestream
>      // which is valid as long as the mStream pointer here is.  Need a better solution.

I think you should fix this. A couple possible, though ugly solutions:

A) If we're on the main thread, pass mStream instead of mStream->GetStream()->AsSourceStream(). This increments the ref count properly and keeps mStream alive until the runnable executes (and, given your changes above, properly cleans up after itself).
B) Otherwise, we're being called from NotifyFinished() on the MediaStreamGraph thread. If Remove() has been called from the main thread, but RemoveListener() has not yet executed on the MediaStreamGraph thread, then there's a chance that RemoveListener will destroy this listener and release the last reference to the nsDOMMediaStream on the main thread before the runnable executes. We can solve this by
  1) Dispatching with NS_DISPATCH_SYNC (which blocks the MediaStreamGraph thread, sadly), or
  2) Dispatching a runnable to the main thread to re-call Invalidate() (which would need to hold an nsRefPtr to this listener to keep it from being destroyed until that runnable executes).

Option 2 is more work, but a better solution. The best way to do it would be to assert we're on the main thread in this function, and dispatch the runnable in NotifyFinished().

It's okay if you do this in a follow-up patch (which documents the scenarios I laid out above in a comment).
Attachment #695910 - Flags: review?(tterribe) → review+
Blocks: 825235
(In reply to Josh Matthews [:jdm] from comment #21)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b4a17ae434e8 for
> subsequent constant test_getUserMedia_basicAudio.html failures:
> https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=a47d98073d4e

Looks like the canplaythrough event bug happens with basicAudio as well. Something tells me then all three tests shouldn't be prefed on yet until we fix the canplaythrough event bug.
Duplicate of this bug: 813339
Duplicate of this bug: 814721
Duplicate of this bug: 823520
Duplicate of this bug: 824362
Blocks: 812648
Whiteboard: [getUserMedia], [blocking-gum+] → [getUserMedia][blocking-gum+][automation-blocked]
Comment on attachment 696202 [details] [diff] [review]
Enable getUserMedia() mochitests

Moved mochitest patch to bug 814807
Attachment #696202 - Attachment is obsolete: true
Note that the first patch here is still in inbound and should merge to m-c. The second patch (backed out from inbound) is now in a different bug, so when the first patch uplifts, this bug can be closed.
https://hg.mozilla.org/mozilla-central/rev/0d78b9661807
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
No longer blocks: 813339
Duplicate of this bug: 813339
Whiteboard: [getUserMedia][blocking-gum+][automation-blocked] → [getUserMedia][blocking-gum+][automation-blocked][qa-]
Duplicate of this bug: 824359
Whiteboard: [getUserMedia][blocking-gum+][automation-blocked][qa-] → [getUserMedia][blocking-gum+][qa-]
You need to log in before you can comment on or make changes to this bug.