Merely creating a PeerConnection leaks a Mutex

VERIFIED FIXED in mozilla19

Status

()

Core
WebRTC
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: jesup)

Tracking

(Blocks: 1 bug, {mlk, testcase})

Trunk
mozilla19
x86_64
Mac OS X
mlk, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink][qa-])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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 ;)
Blocks: 812648
Whiteboard: [MemShrink]
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #682836 - Flags: review?(ethanhugg)

Comment 2

5 years ago
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
Attachment #682836 - Flags: review?(ethanhugg) → review+
(Assignee)

Comment 3

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3b972e271a7
Target Milestone: --- → mozilla19
https://hg.mozilla.org/mozilla-central/rev/b3b972e271a7
Assignee: nobody → rjesup
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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.
Whiteboard: [MemShrink] → [MemShrink][qa-]
(Reporter)

Comment 6

5 years ago
(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).
(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!
(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?
Reopening just to make sure this gets attention. Sorry if I'm just bumbling over some subtleties somewhere.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 10

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/49edf1af4140
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Leak is gone with:

BuildID=20121123161546
SourceRepository=https://hg.mozilla.org/mozilla-central
SourceStamp=d8e4f06198dc
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.