812641 - Merely creating a PeerConnection leaks a Mutex

Status: VERIFIED FIXED

(Core :: WebRTC, defect)

Description

[Jesse Ruderman]

Attachment: 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] (needinfo me)]

Attachment: 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]]

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 3

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3b972e271a7

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b3b972e271a7

Comment 5

[Jason Smith [:jsmith]]

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]

(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][⌚️UTC+1]]

(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)]

(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)]

Reopening just to make sure this gets attention. Sorry if I'm just bumbling over some subtleties somewhere.

Comment 10

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

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]]

https://hg.mozilla.org/mozilla-central/rev/49edf1af4140

Comment 12

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

Leak is gone with:

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