Closed Bug 1408523 Opened 2 years ago Closed 2 years ago

permaleak of 288 bytes of thread stuff in dom/media/tests/mochitest/test_peerConnection_basicH264Video.html

Categories

(Core :: WebRTC: Signaling, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: mccr8, Assigned: jesup)

References

(Blocks 1 open bug)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 file)

This seems to affect Linux32, but not Linux64 or Windows, in mda3.

TEST-INFO | leakcheck | tab process: leaked 1 CondVar
TEST-INFO | leakcheck | tab process: leaked 1 Mutex
TEST-INFO | leakcheck | tab process: leaked 1 Runnable
TEST-INFO | leakcheck | tab process: leaked 1 ThreadEventTarget
TEST-INFO | leakcheck | tab process: leaked 1 ThreadTargetSink
TEST-INFO | leakcheck | tab process: leaked 4 nsTArray_base
TEST-INFO | leakcheck | tab process: leaked 1 nsThread

TEST-UNEXPECTED-FAIL | leakcheck | tab process: 288 bytes leaked (CondVar, Mutex, Runnable, ThreadEventTarget, ThreadTargetSink, ...)
This also affects OS X mda, at a size of 896 bytes.
Whiteboard: [MemShrink]
Whiteboard: [MemShrink] → [MemShrink:P2]
Summary: permaleak of 288 bytes of thread stuff in in dom/media mochitests → permaleak of 288 bytes of thread stuff in dom/media/tests/mochitest/
I narrowed this down to test_peerConnection_basicH264Video.html.
Summary: permaleak of 288 bytes of thread stuff in dom/media/tests/mochitest/ → permaleak of 288 bytes of thread stuff in dom/media/tests/mochitest/test_peerConnection_basicH264Video.html
More tests may leak, but this seems to be responsible for at least some of the leaks.
Component: XPCOM → WebRTC
Rank: 15
Component: WebRTC → WebRTC: Signaling
Priority: -- → P2
It would be nice if this could get fixed. It is the only remaining leak on non-Windows platforms, I believe.

In case it helps, the allocation stack for the nsThread is:

#00: nsThread::AddRef() (nsThread.cpp:181, in XUL)
#01: nsThreadManager::GetCurrentThread() (RefPtr.h:78, in XUL)
#02: nsThreadManager::GetCurrentThread(nsIThread**) (nsThreadManager.cpp:417, in XUL)
#03: mozilla::GetCurrentThreadEventTarget() (nsThreadUtils.cpp:176, in XUL)
#04: mozilla::ThreadEventTarget::Dispatch(already_AddRefed<nsIRunnable>, unsigned int) (ThreadEventTarget.cpp:142, in XUL)
#05: nsThread::Dispatch(already_AddRefed<nsIRunnable>, unsigned int) (AlreadyAddRefed.h:106, in XUL)
#06: nsIEventTarget::Dispatch(nsIRunnable*, unsigned int) (nsIEventTarget.h:37, in XUL)
#07: mozilla::WebrtcGmpVideoEncoder::Encode(webrtc::VideoFrame const&, webrtc::CodecSpecificInfo const*, std::__1::vector<webrtc::FrameType, std::__1::allocator<webrtc::FrameType> > const*) (WebrtcGmpVideoCodec.cpp:328, in XUL)
#08: webrtc::VCMGenericEncoder::Encode(webrtc::VideoFrame const&, webrtc::CodecSpecificInfo const*, std::__1::vector<webrtc::FrameType, std::__1::allocator<webrtc::FrameType> > const&) (generic_encoder.cc:70, in XUL)
#09: webrtc::vcm::VideoSender::AddVideoFrame(webrtc::VideoFrame const&, webrtc::CodecSpecificInfo const*) (video_sender.cc:350, in XUL)
#10: webrtc::ViEEncoder::EncodeVideoFrame(webrtc::VideoFrame const&, long long) (vie_encoder.cc:0, in XUL)
#11: webrtc::ViEEncoder::EncodeTask::Run() (vie_encoder.cc:115, in XUL)
#12: rtc::TaskQueue::TaskContext::RunTask(void*) (task_queue_gcd.cc:53, in XUL)
#13: _dispatch_client_callout (in libdispatch.dylib) + 8
...
This is caused by using Dispatch() with NS_DISPATCH_SYNC in WebrtcGmpVideoEncoder::Encode(); that requires an nsThread, and this thread is a pthread created by webrtc.org

Switching it to match what's used for Decode() -- SyncRunnable, built explicitly for avoiding turning random threads into nsThreads - should fix it.
Assignee: nobody → rjesup
Status: NEW → ASSIGNED
Thanks, Randell! That stack did look weird to me, but I wasn't sure if it was wrong or not.
Jan-Ivar, can you get to this review soon? I'd like to be able to reduce the leak threshold so further regressions like bug 1412408 don't land. Thanks.
Flags: needinfo?(jib)
Attachment #8921326 - Flags: review?(jib) → review+
Flags: needinfo?(jib)
Randell, is this ready to land?
Flags: needinfo?(rjesup)
yes; doing a check-build before landing
Flags: needinfo?(rjesup)
I've done a bunch of try runs with this patch, so it should be okay to land.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5b4f7ad5fc4
Use SyncRunnable instead of Dispatch(...NS_DISPATCH_SYNC) when running on non-nsThreads. r=jib
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e5b4f7ad5fc4
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.