1016265 - Refine the code for MediaRecorder::Session life-cycle.

Status: RESOLVED FIXED

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

Description

[Benjamin Chen [:bechen]]

http://dxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp?from=mediarecorder.cpp#134
http://dxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp?from=mediarecorder.cpp#140

The comment says that we want to destroy the session object on main thread. But the logic here is not correct if main thread execute the runnable quickly faster than line140.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

http://dxr.mozilla.org/mozilla-central/source/content/media/MediaDecoderStateMachine.cpp#2127

Here is an example for your reference. MediaDecoderStateMachine used chained events to destroy the decoder.

Comment 2

[Benjamin Chen [:bechen]]

Attachment: bug-1016265.patch

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8442739 [details] [diff] [review]
bug-1016265.patch

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

http://hg.mozilla.org/mozilla-central/file/f78e532e8a10/content/media/MediaRecorder.cpp#l394
A raw pointer is converted to an already_AddRefed<> where ref count is not increased at all.

I know this is done on purpose so that DestroyRunnable will decrease the ref count which is addref at http://hg.mozilla.org/mozilla-central/file/f78e532e8a10/content/media/MediaRecorder.cpp#l230.

The code is obscure and hard to realize. What if ExtractRunnable is destroyed before ExtractRunnable::Run is executed? The ref count will not be balanced anymore.

Also, it is a bad idiom to say |NS_DispatchToCurrentThread(new nsRunnable(xxx))| for there will be leaks once dispatch fails.

Comment 4

[Benjamin Chen [:bechen]]

(In reply to JW Wang [:jwwang] from comment #3)
> Comment on attachment 8442739 [details] [diff] [review]
> bug-1016265.patch
> 
> Review of attachment 8442739 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> http://hg.mozilla.org/mozilla-central/file/f78e532e8a10/content/media/
> MediaRecorder.cpp#l394
> A raw pointer is converted to an already_AddRefed<> where ref count is not
> increased at all.
> 
> I know this is done on purpose so that DestroyRunnable will decrease the ref
> count which is addref at
> http://hg.mozilla.org/mozilla-central/file/f78e532e8a10/content/media/
> MediaRecorder.cpp#l230.
> 
> The code is obscure and hard to realize. What if ExtractRunnable is
> destroyed before ExtractRunnable::Run is executed? The ref count will not be
> balanced anymore.
Once the matter happened, in the original code the Session object will leak because it never has a change to do DestroyRunnable. By applied my patch, the Session would be destroyed but not in the main thread.

I will consider how to remove the AddRef in Session's constructor...

Comment 5

[u459114]

Comment on attachment 8442739 [details] [diff] [review]
bug-1016265.patch

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

Better than before

::: content/media/MediaRecorder.cpp
@@ +126,2 @@
>        : mSession(aSession) {}
>  

~ExtractRunnable()
{
  if (mSession)
  {
    NS_DispatchToMainThread(new DestroyRunnable(mSession.forget()));
  }
}

Comment 6

[Benjamin Chen [:bechen]]

Attachment: bug-1016265.patch

Use NS_ProxyRelease at ExtractRunnable and DestroyRunnable destructor.
Also refine the |NS_DispatchToCurrentThread(new nsRunnable(xxx))| pattern.

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8443393 [details] [diff] [review]
bug-1016265.patch

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

::: content/media/MediaRecorder.cpp
@@ +132,5 @@
> +        nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
> +        NS_WARN_IF_FALSE(mainThread, "Couldn't get the main thread!");
> +        Session* rawPtr = nullptr;
> +        mSession.swap(rawPtr);
> +        NS_ProxyRelease(mainThread, rawPtr);

You can just say NS_ProxyRelease(mainThread, mSession). NS_ProxyRelease will do the swap for you. [1]

[1] http://hg.mozilla.org/mozilla-central/file/f78e532e8a10/xpcom/glue/nsProxyRelease.h#l43

@@ +152,5 @@
>          // Flush out remaining encoded data.
>          mSession->Extract(true);
>          // Destroy this Session object in main thread.
> +        nsRefPtr<nsIRunnable> event = new DestroyRunnable(mSession.forget());
> +        if (NS_FAILED(NS_DispatchToMainThread(event))) {

There is not much we can do if we fail to dispatch the task to the main thread. Since if we fail here, we should probably also fail in ~DestroyRunnable() which also try to dispatch a task to the main thread. I would prefer to assert the failure to keep the code simple and remove ~DestroyRunnable().

Comment 8

[John Lin [:jhlin][:jolin]]

Comment on attachment 8443393 [details] [diff] [review]
bug-1016265.patch

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

::: content/media/MediaRecorder.cpp
@@ +122,5 @@
>    // Fetch encoded Audio/Video data from MediaEncoder.
>    class ExtractRunnable : public nsRunnable
>    {
>    public:
> +    ExtractRunnable(already_AddRefed<Session> aSession)

It seems DestroyRunnable() uses |already_AddRefed<Session>&&|[1]. Want to keep it consistent? :D

[1] http://dxr.mozilla.org/mozilla-central/source/content/media/MediaRecorder.cpp?from=MediaRecorder.cpp&case=true#177

@@ +206,5 @@
> +        nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
> +        NS_WARN_IF_FALSE(mainThread, "Couldn't get the main thread!");
> +        Session* rawPtr = nullptr;
> +        mSession.swap(rawPtr);
> +        NS_ProxyRelease(mainThread, rawPtr);

As JW said, you can just |NS_ProxyRelease(mainThread, mSession)|.
Even better, this impl looks identical to ~ExtractRunnable(). How about creating a method, say |ReleaseSessionInMainThread()|, and call it from both destructor?

Comment 9

[Benjamin Chen [:bechen]]

Testcase test_mediarecorder_creation_fail.html failed because I increase the Session's refcnt too late for DoSessionEndTask that will reference the session.

Comment 10

[Benjamin Chen [:bechen]]

Attachment: bug-1016265.patch

Ensure the Session is destroyed on main thread. Add assertion and warning if dispatch runnable failed.

try server:
https://tbpl.mozilla.org/?tree=Try&rev=4a446b78891d

Comment 11

[Benjamin Chen [:bechen]]

Attachment: bug-1016265.patch

r=roc

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Retriggers of B2G M3 are showing a lot of occurrences of bug 991776. Seems unlikely to be a coincidence?

Comment 13

[Benjamin Chen [:bechen]]

Since Bug 99177 had been resolved, I'm trying to push this patch again.
https://tbpl.mozilla.org/?tree=Try&rev=f5ec95c5088f

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bfd5fb37afb5

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bfd5fb37afb5