Closed
Bug 848946
Opened 11 years ago
Closed 11 years ago
nsIDOMDataChannel is leaking on shutdown
Categories
(Core :: WebRTC: Networking, defect)
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)
5.52 KB,
text/plain
|
Details | |
45.46 KB,
text/plain
|
Details | |
3.64 KB,
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
5.64 KB,
patch
|
Details | Diff | Splinter Review |
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.
Updated•11 years ago
|
Whiteboard: [WebRTC][blocking-webrtc?][automation-blocked] → [WebRTC][blocking-webrtc+][automation-blocked]
Comment 1•11 years ago
|
||
Ethan - Any ideas why we are leaking?
Reporter | ||
Comment 2•11 years ago
|
||
It might be that this leak is caused by bug 837035 because I haven't seen it before. I can check that locally.
Reporter | ||
Comment 3•11 years ago
|
||
I backed out the patch from bug 837035 locally and it doesn't seem to be the reason. The leak is still there.
Reporter | ||
Comment 4•11 years ago
|
||
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)
Reporter | ||
Comment 5•11 years ago
|
||
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]
Assignee | ||
Comment 6•11 years ago
|
||
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)
Reporter | ||
Comment 7•11 years ago
|
||
Log via NSPR_MODULES as given by Randell in the last comment. I hope it's helpful for you.
Reporter | ||
Comment 8•11 years ago
|
||
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)
Reporter | ||
Comment 9•11 years ago
|
||
Same happens with a x86-64 tinderbox debug build on Ubuntu 12.10 64bit.
OS: Mac OS X → All
Hardware: x86 → All
Comment 10•11 years ago
|
||
Is FF21 affected?
Updated•11 years ago
|
Whiteboard: [MemShrink][WebRTC][blocking-webrtc+][automation-blocked] → [MemShrink:P3][WebRTC][blocking-webrtc+][automation-blocked]
Updated•11 years ago
|
Flags: needinfo?(rjesup)
Assignee | ||
Comment 11•11 years ago
|
||
Flags: needinfo?(rjesup)
Assignee | ||
Comment 12•11 years ago
|
||
Assignee | ||
Comment 13•11 years ago
|
||
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)
Assignee | ||
Comment 14•11 years ago
|
||
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.
Updated•11 years ago
|
Attachment #728940 -
Flags: review?(mcmanus) → review+
Assignee | ||
Comment 15•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7a38caee2b5
Target Milestone: --- → mozilla22
Updated•11 years ago
|
status-firefox21:
--- → affected
Reporter | ||
Comment 16•11 years ago
|
||
(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
Comment 17•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b7a38caee2b5
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Whiteboard: [MemShrink:P3][WebRTC][blocking-webrtc+][automation-blocked] → [MemShrink:P3][WebRTC][blocking-webrtc+][automation-blocked][qa-]
Reporter | ||
Comment 18•11 years ago
|
||
I can verify that the datachannel mochitest does no longer causes leaks in a recent debug build. Thanks Randell.
Status: RESOLVED → VERIFIED
Updated•11 years ago
|
Flags: in-testsuite-
Comment 19•11 years ago
|
||
(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?
Updated•11 years ago
|
Flags: needinfo?(rjesup)
Assignee | ||
Comment 20•11 years ago
|
||
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)
Reporter | ||
Updated•11 years ago
|
Whiteboard: [MemShrink:P3][WebRTC][blocking-webrtc+][automation-blocked][qa-] → [MemShrink:P3][WebRTC][blocking-webrtc+][qa-]
Updated•11 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•