1153690 - Leak with MediaSource and MediaRecorder

Status: RESOLVED FIXED

(Core :: Audio/Video: Recording, defect)

Description

[Jesse Ruderman]

Attachment: w.html

1. Run a (debug) build with XPCOM_MEM_LEAK_LOG=2
2. Load the testcase
3. Wait at least 500ms
4. Quit Firefox

Leaks nsGlobalWindow & nsDocument.

Comment 1

[Andrew McCreight [:mccr8]]

I'll take a first look at this.

Comment 2

[Andrew McCreight [:mccr8]]

There's a DETH that is holding alive the window.  rc=1, no known reference.  No other information on it.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: refcount log

According to refcount logging the DETH is a DOMMediaStream.  Attached is the log for this object.  I don't know this code very well, but it looks like the AddRef from the MediaRecorder() ctor is unaccounted for.  This looks like it must be the mDOMStream field, but that field is probably reported to the cycle collector.

Oh, looking at the CC logs some more, there is what looks like a MediaRecorder holding onto our DOMMediaStream in the first CC log, which zero known references, but then it isn't in the second log, so that's maybe what is holding things alive.  I'll investigate that.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: refcount log for MediaRecorder

Here's the refcount log for the MediaRecorder.  I'm not sure what is going on yet.

Comment 5

[Andrew McCreight [:mccr8]]

Doing some simple cancellation, it looks like the unaccounted for reference to the MediaRecorder was created in the MediaRecorder::Session ctor:

<DOMEventTargetHelper> 0x11981e880 12 AddRef 2
    #01: mozilla::dom::MediaRecorder::Session::Session(mozilla::dom::MediaRecorder*, int) (MediaRecorder.cpp:119, in XUL)
    #02: mozilla::dom::MediaRecorder::Start(mozilla::dom::Optional<int> const&, mozilla::ErrorResult&) (nsRefPtr.h:28, in XUL)
    #03: mozilla::dom::MediaRecorderBinding::start(JSContext*, JS::Handle<JSObject*>, mozilla::dom::MediaRecorder*, JSJitMethodCallArgs const&) (ErrorResult.h:134, in XUL)

Comment 6

[Andrew McCreight [:mccr8]]

Benjamin, it looks like you looked into the lifetime of these objects fairly recently in bug 1047022.  Is this leak something you have time to investigate?  Thanks.

Comment 7

[Benjamin Chen [:bechen]]

(In reply to Andrew McCreight [:mccr8] from comment #6)
> Benjamin, it looks like you looked into the lifetime of these objects fairly
> recently in bug 1047022.  Is this leak something you have time to
> investigate?  Thanks.

Thanks for your investigation, I'll look it soon. I'll try to reproduce it on my ubuntu desktop build.

And according to the STR in comment 0, the session object should receive the notification from ShutdownObserver then release the MediaRecorder.

Comment 8

[Benjamin Chen [:bechen]]

The MediaRecorder and Session's destroy flow rely on the ExtractRunnable -> DestroyRunnable. At this case, the SetupStreams() function send the TracksAvailableCallback to DOMMediaStream, but the callback function is never called. (https://dxr.mozilla.org/mozilla-central/source/dom/media/DOMMediaStream.cpp#504)
So there is no ExtractRunnable running cause the leak.

Comment 9

[Benjamin Chen [:bechen]]

Attachment: bug-1153690.v01.patch

Comment 10

[Benjamin Chen [:bechen]]

Attachment: bug-1153690.v01.patch

r=roc

tryserver:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=083e224d3457

Comment 11

[Benjamin Chen [:bechen]]

(In reply to Benjamin Chen [:bechen] from comment #10)
> Created attachment 8594578 [details] [diff] [review]
> bug-1153690.v01.patch
> 
> r=roc
> 
> tryserver:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=083e224d3457

Now I'm dixing the build break and other testscase failed like
TEST-UNEXPECTED-FAIL | dom/media/test/test_mediarecorder_record_immediate_stop.html | application terminated with exit code 11

Comment 12

[Benjamin Chen [:bechen]]

There are 2 situations we need to handle in this patch.
1. MediaRecorder.Start then MediaRecorder.Stop immediately (test_mediarecorder_record_immediate_stop.html) , then TracksAvailableCallback comes.

2. MediaRecorder.Start then close tab(comment 0) then no TracksAvailableCallback comes.

My v01 patch breaks the test_mediarecorder_record_immediate_stop.html

Comment 13

[Benjamin Chen [:bechen]]

Attachment: bug-1153690.v02.patch

Fix build break and testcase fail.
Carry r+ from v01 because the only change is to call DoSessionEndTask() instead of BreakCycle() and put some prlog.

try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0352305daca6

Comment 14

[Benjamin Chen [:bechen]]

Attachment: bug-1153690.v02.patch

Fix other MediaRecorder testcase failed.

tryserver:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fab282e6dd3

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4803c84d4976

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4803c84d4976

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Backed out this bug and bug 1140995 for causing bug 1157654.

Comment 18

[Benjamin Chen [:bechen]]

Attachment: bug-1153690.v02.patch

Rebase.
try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=53da71ef6339

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/958cafbaad72

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/958cafbaad72