Closed
Bug 802538
Opened 12 years ago
Closed 11 years ago
mozGetUserMedia streams don't get destroyed
Categories
(Core :: WebRTC, defect, P1)
Core
WebRTC
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: jesup, Assigned: jesup)
References
Details
(Whiteboard: [getUserMedia][blocking-gum+][qa-])
Attachments
(1 file, 1 obsolete file)
6.04 KB,
patch
|
derf
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•12 years ago
|
||
Isn't this bug 798323?
Assignee | ||
Comment 2•12 years ago
|
||
It's a specific instance of such a leak
Comment 4•12 years ago
|
||
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.
Comment 5•12 years ago
|
||
I can be setup similar to HTTP logging as described here: https://developer.mozilla.org/en/HTTP_Logging
Comment 6•12 years ago
|
||
(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.
Comment 7•12 years ago
|
||
(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
Updated•12 years ago
|
Priority: -- → P2
Whiteboard: [getUserMedia], [blocking-gum+]
Updated•12 years ago
|
Priority: P2 → P1
Assignee | ||
Comment 9•12 years ago
|
||
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
Comment 10•12 years ago
|
||
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-
Assignee | ||
Comment 11•11 years ago
|
||
This fixes a bunch of other leaks too: bug 798653, and likely bug 813339, bug 814721, and bug 824362.
Assignee | ||
Comment 12•11 years ago
|
||
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)
Assignee | ||
Comment 14•11 years ago
|
||
With the mochitests limited to the two gum tests, it completes with no leaks!
Assignee | ||
Comment 15•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=87d42525941f
Assignee | ||
Comment 16•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #696202 -
Flags: review?(hskupin)
Comment 17•11 years ago
|
||
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 18•11 years ago
|
||
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+
Assignee | ||
Comment 19•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d78b9661807
Target Milestone: --- → mozilla20
Assignee | ||
Comment 20•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/35a7f17ac707
Comment 21•11 years ago
|
||
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
Comment 22•11 years ago
|
||
(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.
Updated•11 years ago
|
Whiteboard: [getUserMedia], [blocking-gum+] → [getUserMedia][blocking-gum+][automation-blocked]
Assignee | ||
Comment 27•11 years ago
|
||
Comment on attachment 696202 [details] [diff] [review] Enable getUserMedia() mochitests Moved mochitest patch to bug 814807
Attachment #696202 -
Attachment is obsolete: true
Assignee | ||
Comment 28•11 years ago
|
||
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.
Comment 29•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0d78b9661807
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Whiteboard: [getUserMedia][blocking-gum+][automation-blocked] → [getUserMedia][blocking-gum+][automation-blocked][qa-]
Updated•11 years ago
|
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.
Description
•