Closed Bug 802538 Opened 9 years ago Closed 9 years ago
Get User Media streams don't get destroyed
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.
Isn't this bug 798323?
It's a specific instance of such a leak
Makes sense. Lets add it as dependency.
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+]
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.
This fixes a bunch of other leaks too: bug 798653, and likely bug 813339, bug 814721, and bug 824362.
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)
With the mochitests limited to the two gum tests, it completes with no leaks!
Attachment #696202 - Flags: review?(hskupin)
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+
Target Milestone: --- → mozilla20
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
(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.
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.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [getUserMedia][blocking-gum+][automation-blocked] → [getUserMedia][blocking-gum+][automation-blocked][qa-]
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.