973522 - MediaRecorder causes large leak

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

This testcase causes Firefox to leak the world.

Might be related to bug 970236. Hard to tell because I don't see a testcase there.

Note: sometimes this will crash [@ mozilla::DeadlockDetector<mozilla::BlockingResourceBase::DeadlockDetectorEntry>::CheckAcquisition], but I think that's an unrelated bug that is triggered by the leak.  (See intermittent-failure bug 966972 and previously-fixed bug 919176.)

Comment 1

[Jason Smith [:jsmith]]

Very likely a regression, as fuzzers haven't caught this previously.

Comment 2

[Jesse Ruderman]

Attachment: crash stack (NOT THIS BUG)

Comment 3

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Hi Jesse, 
Could you tell me how do you detect the memory leak in detail?
by using about::memory?

Comment 4

[Jesse Ruderman]

By running a debug build with XPCOM_MEM_LEAK_LOG=2, loading the testcase, and quitting.

Comment 5

[Jason Smith [:jsmith]]

When you say leak the world here, how much memory are we leaking in terms of KB?

Comment 6

[Jesse Ruderman]

It leaks 2 nsGlobalWindows and everything they entrain, so at least 500KB for a blank page in a debug build.

Comment 7

[Jesse Ruderman]

Attachment: leak info

Comment 8

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

The audiocontext keeps passing null chunk for opus encoder and let encoder waiting valid chunk...
Should check the release step for this situation.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This is almost certainly a regression from bug 879669, where the reference from the MediaRecorder to the MediaRecorder::Session was changed from a weak reference to a strong reference.

After bug 879669, the MediaRecorder and the MediaRecorder::Session form a cycle.  This cycle must be broken manually, because the Session is threadsafe and hence cannot be cycle collected.  (NB: This means that traversing and unlinking MediaRecorder::mSession is 1) useless and 2) misleading as to what is really going on here).  This cycle must be broken manually, but it is only ever broken if MediaRecorder::Stop is called.  What guarantees that that happens?

The reference counting involved here is also quite difficult to follow.  e.g. at http://mxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp?force=1#118 we construct an already_AddRefed<Session>.  Where did that pointer get AddRefed?

Comment 10

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Found encoder object won't release on this case, keep find a way to fix this null audio chunk issue.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

roc, can you look at comment 9?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The MediaRecoder::Session should not hold a strong reference to the MediaRecorder. Then the MediaRecorder can be cycle-collected. The reference from the MediaRecorder::Session to the MediaRecorder should be weak, and the MediaRecorder's destructor can tell all its Sessions to drop their reference.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

MediaRecorder::Session currently uses mRecorder off the main thread (e.g. at http://mxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp#282) so you will to either move all those uses to the main thread or synchronize somehow.

Comment 14

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v1

Found leak on session object and force stop from recorder during xpcom shutdown event.

Comment 15

[Jason Smith [:jsmith]]

(In reply to Randy Lin [:rlin] from comment #14)
> Created attachment 8385822 [details] [diff] [review]
> patch v1
> 
> Found leak on session object and force stop from recorder during xpcom
> shutdown event.

Note - we need an automated test for this. Either a crashtest or a mochitest would work here.

Comment 16

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: test case v1

Test case for capturing this leak.
Without this fix, the mochitest would report leak after endding of this test case.
 0:30.65 WARNING: YOU ARE LEAKING THE WORLD

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8385822 [details] [diff] [review]
patch v1

Review of attachment 8385822 [details] [diff] [review]:
-----------------------------------------------------------------

All this does is turn a leak through shutdown into a leak until shutdown.  You haven't actually fixed anything.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #13)
> MediaRecorder::Session currently uses mRecorder off the main thread (e.g. at
> http://mxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.
> cpp#282) so you will to either move all those uses to the main thread or
> synchronize somehow.

I believe that's the only use off the main thread. I think we should remove that and just pass the MIME type as a parameter to the PushBlobRunnable, which can set it on the recorder on the main thread.

Comment 19

[Jason Smith [:jsmith]]

Comment on attachment 8385829 [details] [diff] [review]
test case v1

Review of attachment 8385829 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/test/test_mediarecorder_record_audiocontext_mlk.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Test for possible memory leak</title>

Nit - can you use a more descriptive title here?

@@ +3,5 @@
> +<head>
> +  <title>Test for possible memory leak</title>
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
> +  <script type="text/javascript" src="manifest.js"></script>

Nit - this isn't needed, as we're not doing a test here that makes use of this file.

Comment 20

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Hi roc, 
So the design would change to let media recorder hold the weak reference of session and
try to notify session object & stop if media recorder enter release?

Comment 21

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Hi Kyle, 
We use the ReentrantMonitorAutoEnter to wait for valid data input. 
For this case, the audiocontext always sent invalid data to encoder, so the encoder would keep waiting.
If I try to use MediaRecorder's destructor and stop the encoder, but it failed to enter that section. (Remove the XPCOM shutdown event and dump the hang callstack)
Check gdb with callstack：
=====
#1  0x00007f248bfde287 in PR_Wait (mon=0x7f248c2beed0, timeout=4294967295) at /media/ssd/gecko_c2/nsprpub/pr/src/pthreads/ptsynch.c:691
#2  0x00007f248420d806 in mozilla::ReentrantMonitor::Wait (this=0x7f248c264060, interval=4294967295) at /media/ssd/gecko_c2/xpcom/glue/BlockingResourceBase.cpp:309
#3  0x00007f24842b3fa1 in mozilla::ReentrantMonitorAutoEnter::Wait (this=0x7fffa66d3760, interval=4294967295) at ../../dist/include/mozilla/ReentrantMonitor.h:192
#4  0x00007f24842db6e6 in nsEventQueue::GetEvent (this=0x7f248c264060, mayWait=true, result=0x7fffa66d3810) at /media/ssd/gecko_c2/xpcom/threads/nsEventQueue.cpp:61
#5  0x00007f24842e447b in nsThread::nsChainedEventQueue::GetEvent (this=0x7f248c264050, mayWait=true, event=0x7fffa66d3810) at /media/ssd/gecko_c2/xpcom/threads/nsThread.h:97
#6  0x00007f24842de2fe in nsThread::ProcessNextEvent (this=0x7f248c263fe0, mayWait=true, result=0x7fffa66d389f) at /media/ssd/gecko_c2/xpcom/threads/nsThread.cpp:635
#7  0x00007f248421ef40 in NS_ProcessNextEvent (thread=0x7f248c263fe0, mayWait=true) at /media/ssd/gecko_c2/xpcom/glue/nsThreadUtils.cpp:263
#8  0x00007f24842ddd5d in nsThread::Shutdown (this=0x7f2466d2c280) at /media/ssd/gecko_c2/xpcom/threads/nsThread.cpp:487
#9  0x00007f24842df67b in nsThreadManager::Shutdown (this=0x7f248ae34700) at /media/ssd/gecko_c2/xpcom/threads/nsThreadManager.cpp:138
#10 0x00007f2484229944 in mozilla::ShutdownXPCOM (servMgr=0x7f248c27b338) at /media/ssd/gecko_c2/xpcom/build/nsXPComInit.cpp:724
#11 0x00007f2484229627 in NS_ShutdownXPCOM (servMgr=0x7f248c27b338) at /media/ssd/gecko_c2/xpcom/build/nsXPComInit.cpp:645


My question is, Does this kind of monitor would also block the destructor process?
Is it any better implementation for avoiding this problem?
Thanks.

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

nsThreadManager will automatically try to shut down any threads that you did not shut down when the process wants to exit.  Shutting down the thread requires it to be processing events (to get the event from nsThreadManager telling it to shut down), so if your thread is blocked on some other monitor/condvar/whatever it will fail to shut down and we will hang.

To be clear, in comment 17, I'm not saying that the changes are not necessary.  I'm saying that they are not sufficient to address the real problem here.  We may still need everything in that patch, including the changes to Cancel().

Comment 23

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

hmm, change to weak reference would cause some new use after free issue.
I'm debugging this.

Comment 24

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v2

Hi Kyle, 
This patch remove the strong reference of session object in media recorder.
Please help review it if possible.

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8388551 [details] [diff] [review]
patch v2

roc should look at this first.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8388551 [details] [diff] [review]
patch v2

Review of attachment 8388551 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/MediaRecorder.h
@@ +101,5 @@
>    nsRefPtr<DOMMediaStream> mStream;
>    // The current state of the MediaRecorder object.
>    RecordingState mState;
>    // Current recording session.
> +  Session* mSession;

I think this is incorrect. We should strongly own the current session. The session's mRecorder reference is the one that should be changed to not be a strong reference. See comment #12.

Comment 27

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Still debugging for the mochitest use after free problem.
I need some refactor for this reference count change.

Comment 28

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v3

Thanks roc's suggestion and great help, It works and pass local mochitest.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8392773 [details] [diff] [review]
patch v3

Review of attachment 8392773 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/MediaRecorder.cpp
@@ +90,4 @@
>        MOZ_ASSERT(NS_IsMainThread());
> +      if (!mSession) {
> +        return NS_OK;
> +      }

mSession can't be null so let's not do this.

@@ +94,1 @@
>        MediaRecorder *recorder = mSession->mRecorder;

Make this an nsRefPtr so we temporarily keep the recorder alive for the rest of this method.

@@ +99,4 @@
>        if (mSession->IsEncoderError()) {
>          recorder->NotifyError(NS_ERROR_UNEXPECTED);
>        }
> +      if (!mSession || !recorder) {

No need to check mSession or recorder again.

@@ +186,2 @@
>        MOZ_ASSERT(NS_IsMainThread() && mSession.get());
>        MediaRecorder *recorder = mSession->mRecorder;

Make this an nsRefPtr to ensure the recorder stays alive during this function.

@@ +290,5 @@
>        return true;
>      }
>      return false;
>    }
> +  void CleanRecorder() {

We don't need this. Set mRecorder to null in Session::Stop().

@@ +418,5 @@
>    }
>  
>  private:
>    // Hold a reference to MediaRecoder to make sure MediaRecoder be
>    // destroyed after all session object dead.

Fix comment since we're not keeping the MediaRecorder alive here anymore.

Comment 30

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

>We don't need this. Set mRecorder to null in Session::Stop().
When recorder stopped, it should has ondataavablable & onstop event through recorder, 
Set mRecorder to be null on this function and it would break this event sequence..

Comment 31

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v4

Thanks. update new one.
Remove the CleanRecoder function in session. 
Correct the PushBlobRunnable incorrect pointer usage.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Randy Lin [:rlin] from comment #30)
> >We don't need this. Set mRecorder to null in Session::Stop().
> When recorder stopped, it should has ondataavablable & onstop event through
> recorder, 
> Set mRecorder to be null on this function and it would break this event
> sequence..

Right, OK.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8393258 [details] [diff] [review]
patch v4

Review of attachment 8393258 [details] [diff] [review]:
-----------------------------------------------------------------

You're not clearing mRecorder when a Session becomes "not the current session", or when MediaRecorder is destroyed. So you need to bring back CleanRecorder, but call it ForgetRecorder instead.

::: content/media/MediaRecorder.cpp
@@ +410,5 @@
>      return NS_OK;
>    }
>  
>  private:
> +  // Hold weak a reference to MediaRecoder

Add a comment saying this must only be accessed on the main thread.

Comment 34

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v4.1

Set mRecorder to be null when MediaRecorder destroyed.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8393303 [details] [diff] [review]
patch v4.1

Review of attachment 8393303 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/MediaRecorder.cpp
@@ +531,5 @@
>      return;
>    }
>    mState = RecordingState::Inactive;
> +  if (mSession) {
> +    mSession->Stop();

You need to set mSession to null here. You also need to call ForgetMediaRecorder.

Comment 36

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

If I set the mRecorder to be nullptr at this point.
It would cause missing dataavailable & onstop event...

Comment 37

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

BTW, I got a crash on start immediate stop case. On this link: 
https://tbpl.mozilla.org/php/getParsedLog.php?id=36378792&tree=Try
Is it possible to get the null pointer on mSession object on PushBlobRunnable runnable?

Comment 38

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Found unpredicted release session object when call the PushBlobRunnable.run. Can reproduce on win7 build and debugging this issue.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Randy Lin [:rlin] from comment #36)
> If I set the mRecorder to be nullptr at this point.
> It would cause missing dataavailable & onstop event...

Right OK. So then you need to keep an array of all the Sessions for this MediaRecorder and in the MediaRecorder destructor, tell *all* of them to clear their mRecorder references.

Comment 40

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Hi roc, 
Found timing issue related to 985783. 
When encoder thread try to dispatch runnable object to main thread, (the object member already set the reference pointer)
When something delay the run() method,
It can cause "use after free" problem when try to access those reference like media recorder in run() method...

Ex run this first
http://dxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp?from=PushBlobRunnable#78
then enter this method slower than usual.
http://dxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp?from=PushBlobRunnable#82
then crash..
http://dxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp?from=PushBlobRunnable#86

Any Suggestion?

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

mSession should not have been deleted there because mSession is an nsRefPtr so PushBlobRunnable keeps it alive.

mSession->mRecorder should either be null or pointing to a valid not-deleted MediaRecorder. Because if the MediaRecorder is destroyed, then it would have set mRecorder in all its sessions to null, including the mRecorder in PushBlobRunnable's mSession.

Comment 42

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v5

1. Found problem that I should remove the mSession in NS_IMPL_CYCLE_COLLECTION_INHERITED.
Then the mSession can not be the nullptr then I can clean the recorder object in session. 
2. I clean the record object when session can enter stop stage like DestroyRunnable, xpcom shutdown, forgetMediaRecorder.
3. Also need andther bug that is : bug 985878 AtomicRefCounted is not thread-safe.

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8395595 [details] [diff] [review]
patch v5

Review of attachment 8395595 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/MediaRecorder.cpp
@@ +467,5 @@
> +  LOG(PR_LOG_DEBUG, ("~MediaRecorder (%p)", this));
> +  if (mSession) {
> +    mSession->ForgetMediaRecorder();
> +    mSession->Stop();
> +  }

You need to call ForgetMediaRecorder in all other sessions that have this as their mRecorder, but you aren't.

@@ +662,5 @@
>    if (!CheckPrincipal()) {
>      // Media is not same-origin, don't allow the data out.
> +    // Need to take over this blob, otherwise crash..
> +    BlobEventInit init;
> +    init.mData = aBlob;

This fix belongs in a separate patch. As far as I can tell it has nothing to do with this bug.

::: content/media/MediaRecorder.h
@@ +115,1 @@
>  };

Drop this change

Comment 44

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v5.1

Use array to remember sessions's pointer created in media recorder and clean in destructor.

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8395687 [details] [diff] [review]
patch v5.1

Review of attachment 8395687 [details] [diff] [review]:
-----------------------------------------------------------------

This patch has big problems:
-- No Session is ever removed from the array. So if you call start()/stop() on the MediaRecorder over and over again, we'll leak memory.
-- Also, you're bringing back the reference cycle that this bug was about in the first place.

So, we need an array of MediaSession* pointers. Also, when a MediaSession's mRecorder field is set to null, it should remove itself from the array at that point, so we don't have references to dead or dying MediaSessions in the array.

::: content/media/MediaRecorder.cpp
@@ +468,5 @@
> +  for (uint32_t i = 0; i < mSessions.Length(); i ++) {
> +  if (mSessions[i]) {
> +    mSessions[i]->ForgetMediaRecorder();
> +    mSessions[i]->Stop();
> +  }

Indent the body of the for loop properly

::: content/media/MediaRecorder.h
@@ +93,5 @@
>    // Creating a simple event to notify UA simple event.
>    void DispatchSimpleEvent(const nsAString & aStr);
>    // Creating a error event with message.
>    void NotifyError(nsresult aRv);
> +  // Check if the recorder's principalCreateAnd is the subsume of mediaStream

I'm not sure what you meant to say here, but "principalCreateAnd" probably isn't it.

@@ +105,5 @@
>    nsRefPtr<DOMMediaStream> mStream;
>    // The current state of the MediaRecorder object.
>    RecordingState mState;
> +  // Hold the sessions pointer in media recorder and clean in the destructor of recorder.
> +  nsTArray<nsRefPtr<Session> >mSessions;

Space before mSessions

Comment 46

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v6

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8396114 [details] [diff] [review]
patch v6

Review of attachment 8396114 [details] [diff] [review]:
-----------------------------------------------------------------

I think you uploaded the wrong patch

Comment 48

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v6

=.=

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8396124 [details] [diff] [review]
patch v6

Review of attachment 8396124 [details] [diff] [review]:
-----------------------------------------------------------------

Looks pretty good. Just a minor fix left.

::: content/media/MediaRecorder.cpp
@@ +556,5 @@
>      aResult.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>      return;
>    }
>    mState = RecordingState::Inactive;
> +  if (mSessions.Length() > 0 && mSessions.LastElement()) {

No point in null-checking last element since the array should never contain a null entry.

@@ +571,5 @@
>      return;
>    }
>  
> +  MOZ_ASSERT(mSessions.Length() > 0);
> +  if (mSessions.LastElement()) {

No point in null-checking last element since the array should never contain a null entry.

@@ +591,5 @@
>      return;
>    }
>  
> +  MOZ_ASSERT(mSessions.Length() > 0);
> +  if (mSessions.LastElement()) {

No point in null-checking last element since the array should never contain a null entry.

::: content/media/MediaRecorder.h
@@ +106,5 @@
>    nsRefPtr<DOMMediaStream> mStream;
>    // The current state of the MediaRecorder object.
>    RecordingState mState;
> +  // Hold the sessions pointer in media recorder and clean in the destructor of recorder.
> +  nsTArray<Session* > mSessions;

No space before > here

Comment 50

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v6.1

Thanks for reviewing.
Address those nits and have the new one.

Comment 51

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: check-in patch

carry reviewer including test case.
try: https://tbpl.mozilla.org/?tree=Try&rev=d3f1d6123fac

Comment 52

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92d454193e61

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/92d454193e61

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d17672d2b6d0