Last Comment Bug 812641 - Merely creating a PeerConnection leaks a Mutex
: Merely creating a PeerConnection leaks a Mutex
Status: VERIFIED FIXED
[MemShrink][qa-]
: mlk, testcase
Product: Core
Classification: Components
Component: WebRTC (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- normal (vote)
: mozilla19
Assigned To: Randell Jesup [:jesup]
: Jason Smith [:jsmith]
:
Mentors:
Depends on:
Blocks: 326633 812648
  Show dependency treegraph
 
Reported: 2012-11-16 14:09 PST by Jesse Ruderman
Modified: 2012-11-23 08:14 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (82 bytes, text/html)
2012-11-16 14:09 PST, Jesse Ruderman
no flags Details
Shut down SipCC instance when number of PeerConnections == 0 (1.36 KB, patch)
2012-11-17 20:06 PST, Randell Jesup [:jesup]
ethanhugg: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-11-16 14:09:50 PST
Created attachment 682627 [details]
testcase

With:
  user_pref("media.peerconnection.enabled", true);

The testcase makes Firefox leak a Mutex (as shown by XPCOM_MEM_LEAK_LOG=2).

This is probably less serious than bug 798323, but my fuzzer is actually able to hit this one ;)
Comment 1 Randell Jesup [:jesup] 2012-11-17 20:06:40 PST
Created attachment 682836 [details] [diff] [review]
Shut down SipCC instance when number of PeerConnections == 0

The 4 mutexes leaked by the testcase are the 4 used by different parts of media/webrtc/signaling (SipCC).  This shuts down the SipCC Instance when the number of PeerConnections is 0, but a better solution is likely (such as having the Instance use refcounting, and/or use a delayed release to avoid churn of a starting/stopping a large module.
Comment 2 Ethan Hugg [:ehugg] 2012-11-17 20:21:13 PST
Comment on attachment 682836 [details] [diff] [review]
Shut down SipCC instance when number of PeerConnections == 0

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +280,5 @@
>  }
>  
>  PeerConnectionImpl::~PeerConnectionImpl()
>  {
> +  Close();

Let's put ASSERTS here and in creation for main thread in case the threading gets changed later.

@@ +289,4 @@
>    peerconnections.erase(mHandle);
> +  if (peerconnections.empty())
> +    Shutdown();
> + 

Trailing whitespace here
Comment 4 Ryan VanderMeulen [:RyanVM] 2012-11-18 05:39:35 PST
https://hg.mozilla.org/mozilla-central/rev/b3b972e271a7
Comment 5 Jason Smith [:jsmith] 2012-11-18 07:57:06 PST
For future reference - how would I detect that an automated test I wrote ends up having a memory leak?

I'd imagine with this leak example that if we had a basic peer connection test in mochitest, we would hit this memory leak.
Comment 6 Jesse Ruderman 2012-11-18 11:41:43 PST
(In reply to Jason Smith [:jsmith] from comment #5)
> For future reference - how would I detect that an automated test I wrote
> ends up having a memory leak?

At the end of a mochitest run, you should see something like:

"BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, default process"

If there are any leaks, they will be listed in the table below this line.

This is the same leak detection infrastructure I used to find this bug (by setting XPCOM_MEM_LEAK_LOG=2).
Comment 7 Henrik Skupin (:whimboo) 2012-11-18 12:40:28 PST
(In reply to Jason Smith [:jsmith] from comment #5)
> I'd imagine with this leak example that if we had a basic peer connection
> test in mochitest, we would hit this memory leak.

We have a couple of memory leaks in our existent crashtests which I'm going to investigate the next couple of days. So it's not only dependent on Mochitests!
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2012-11-18 18:28:18 PST
(In reply to Randell Jesup [:jesup] from comment #3)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b3b972e271a7

The patch pushed here doesn't seem to match the one in the bug at all. Moreover, it's only assertions, so it seems very unlikely that this bug is fixed.

What's the deal?
Comment 9 Bobby Holley (:bholley) (busy with Stylo) 2012-11-18 18:29:32 PST
Reopening just to make sure this gets attention. Sorry if I'm just bumbling over some subtleties somewhere.
Comment 10 Randell Jesup [:jesup] 2012-11-18 19:00:36 PST
Thanks greatly for catching that.  Some foobar with queues while addressing the nits caused me to commit just the requested added assert and not the actual patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/49edf1af4140
Comment 11 Ed Morley [:emorley] 2012-11-19 07:38:52 PST
https://hg.mozilla.org/mozilla-central/rev/49edf1af4140
Comment 12 Henrik Skupin (:whimboo) 2012-11-23 08:14:36 PST
Leak is gone with:

BuildID=20121123161546
SourceRepository=https://hg.mozilla.org/mozilla-central
SourceStamp=d8e4f06198dc

Note You need to log in before you can comment on or make changes to this bug.