Closed Bug 848946 Opened 11 years ago Closed 11 years ago

nsIDOMDataChannel is leaking on shutdown

Categories

(Core :: WebRTC: Networking, defect)

22 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla22
Tracking Status
firefox20 --- unaffected
firefox21 + disabled
firefox22 + fixed

People

(Reporter: whimboo, Assigned: jesup)

References

()

Details

(Keywords: memory-leak, reproducible, Whiteboard: [MemShrink:P3][WebRTC][blocking-webrtc+][qa-])

Attachments

(4 files)

Seen with the attached not-done-yet datachannel mochitest. Even we don't clean-up everything correctly we should not leak.

Once applied run the test via:

./mach mochitest-plain dom/media/tests/mochitest/test_peerConnection_basicDataChannel.html

Then quit the browser with Cmd+Q.

Leaks as seen:

7:12.26 nsTraceRefcntImpl::DumpStatistics: 967 entries
 7:12.26 TEST-UNEXPECTED-FAIL| automationutils.processLeakLog() | leaked 5040 bytes during test execution
 7:12.26 TEST-UNEXPECTED-FAIL| automationutils.processLeakLog() | leaked 3 instances of DataChannel with size 128 bytes each (384 bytes total)
 7:12.26 TEST-UNEXPECTED-FAIL| automationutils.processLeakLog() | leaked 1 instance of DataChannelConnection with size 592 bytes
 7:12.26 TEST-UNEXPECTED-FAIL| automationutils.processLeakLog() | leaked 1 instance of DtlsIdentity with size 32 bytes
 7:12.26 TEST-UNEXPECTED-FAIL| automationutils.processLeakLog() | leaked 5 instances of Mutex with size 24 bytes each (120 bytes total)
 7:12.26 TEST-UNEXPECTED-FAIL| automationutils.processLeakLog() | leaked 1 instance of NrIceCtx with size 160 bytes
 7:12.26 TEST-INFO | automationutils.processLeakLog() | leaked 3 instances of NrIceMediaStream with size 168 bytes each (504 bytes total)
 7:12.26 TEST-INFO | automationutils.processLeakLog() | leaked 12 instances of NrSocket with size 200 bytes each (2400 bytes total)
 7:12.26 TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of ReentrantMonitor with size 32 bytes
 7:12.26 TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of TransportFlow with size 224 bytes
 7:12.26 TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of VerificationDigest with size 88 bytes
 7:12.27 TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsDeque with size 96 bytes
 7:12.27 TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsSocketTransportService with size 216 bytes
 7:12.27 TEST-INFO | automationutils.processLeakLog() | leaked 3 instances of nsStringBuffer with size 8 bytes each (24 bytes total)
 7:12.27 TEST-INFO | automationutils.processLeakLog() | leaked 9 instances of nsTArray_base with size 8 bytes each (72 bytes total)
 7:12.27 TEST-INFO | automationutils.processLeakLog() | leaked 1 instance of nsTimerImpl with size 96 bytes

I haven't tested on 21.0a2 so not sure if that's also happening there.
Whiteboard: [WebRTC][blocking-webrtc?][automation-blocked] → [WebRTC][blocking-webrtc+][automation-blocked]
Ethan - Any ideas why we are leaking?
It might be that this leak is caused by bug 837035 because I haven't seen it before. I can check that locally.
I backed out the patch from bug 837035 locally and it doesn't seem to be the reason. The leak is still there.
Randell, sadly I missed the line on IRC where you mentioned which NSPR logs you want to have. Would you mind to tell me that again? Was it datachannel:5 only?
Flags: needinfo?(rjesup)
So this leak is caused by the following line which creates a datachannel. There is no need to listen for events. Once the code has been run close the browser.

var pc1 = mozRTCPeerConnection();
[..] (setup peer connection and data channel)
pc1.createDataChannel("This is channel request from pcLocal",{})
Summary: Datachannel is leaking → nsIDOMDataChannel is leaking on shutdown
Whiteboard: [WebRTC][blocking-webrtc+][automation-blocked] → [MemShrink][WebRTC][blocking-webrtc+][automation-blocked]
If you could do a run with datachannel:5,signaling:5 and add the results it would help a bit, as I'm working from a conference currently.  Thanks
Flags: needinfo?(rjesup)
Attached file NSPR log
Log via NSPR_MODULES as given by Randell in the last comment. I hope it's helpful for you.
It might not only be on shutdown. So it needs further investigation. And as I have seen right now it also leaks the same stuff when running our demo page for data channels.

http://mozilla.github.com/webrtc-landing/data_test.html

Steps:
1. Click on Start
2. Quit the browser (closing the tab might also be the action here)
Same happens with a x86-64 tinderbox debug build on Ubuntu 12.10 64bit.
OS: Mac OS X → All
Hardware: x86 → All
Is FF21 affected?
Assignee: nobody → rjesup
Whiteboard: [MemShrink][WebRTC][blocking-webrtc+][automation-blocked] → [MemShrink:P3][WebRTC][blocking-webrtc+][automation-blocked]
Flags: needinfo?(rjesup)
Comment on attachment 728940 [details] [diff] [review]
In shutdown, don't wait for DataChannel closes to go to peer and back

In shutdown, we can't wait for the other side to close their end of the DataChannel before releasing ours (since we're going to close the socket as soon as we send all our resets.
Attachment #728940 - Flags: review?(mcmanus)
Comment on attachment 728942 [details] [diff] [review]
fixes/updates to test_peerconnection_basicDataChannel.html

Henrik - you should look at some of the fixes here.  There's more to do (in particular factoring out the onmessage/onopen/onclose's) and also this test has no exit condition.
Attachment #728940 - Flags: review?(mcmanus) → review+
(In reply to Randell Jesup [:jesup] from comment #14)
> Henrik - you should look at some of the fixes here.  There's more to do (in
> particular factoring out the onmessage/onopen/onclose's) and also this test
> has no exit condition.

I know. It was a pretty hacky work in progress patch which showed this behavior. However we should never leak. :) Thanks for landing the fix. I will check it tomorrow.
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/b7a38caee2b5
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [MemShrink:P3][WebRTC][blocking-webrtc+][automation-blocked] → [MemShrink:P3][WebRTC][blocking-webrtc+][automation-blocked][qa-]
I can verify that the datachannel mochitest does no longer causes leaks in a recent debug build. Thanks Randell.
Status: RESOLVED → VERIFIED
Flags: in-testsuite-
(In reply to Randell Jesup [:jesup] from comment #15)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b7a38caee2b5

Ideally,we should uplift this to beta asap as Fx21 is affected here and this ix fixed on Fx22.
I am also ok to let the patch ride the trains and mark status-firefox21:disabled since we have webRTC preffed of by default.Jesup,based on the risk of the patch, what do you think would be best here?
Flags: needinfo?(rjesup)
This is a fairly low-risk change - however, webrtc is disabled by default in Fx21, and this merely closes a small leak if it's enabled and used, so I see no advantage in uplifitng to beta
Flags: needinfo?(rjesup)
Whiteboard: [MemShrink:P3][WebRTC][blocking-webrtc+][automation-blocked][qa-] → [MemShrink:P3][WebRTC][blocking-webrtc+][qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: