Closed Bug 1016265 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: bechen, Assigned: bechen)

Details

Attachments

(1 file, 3 obsolete files)

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.
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.
Assignee: nobody → bechen
Attached patch bug-1016265.patch (obsolete) — Splinter Review
Attachment #8442739 - Flags: feedback?(rlin)
Attachment #8442739 - Flags: feedback?(jwwang)
Attachment #8442739 - Flags: feedback?(cku)
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.
Attachment #8442739 - Flags: feedback?(jwwang) → feedback-
(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 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()));
  }
}
Attachment #8442739 - Flags: feedback?(cku) → feedback+
Attached patch bug-1016265.patch (obsolete) — Splinter Review
Use NS_ProxyRelease at ExtractRunnable and DestroyRunnable destructor.
Also refine the |NS_DispatchToCurrentThread(new nsRunnable(xxx))| pattern.
Attachment #8442739 - Attachment is obsolete: true
Attachment #8442739 - Flags: feedback?(rlin)
Attachment #8443393 - Flags: feedback?(jwwang)
Attachment #8443393 - Flags: feedback?(jolin)
Attachment #8443393 - Flags: feedback?(cku)
Attachment #8443393 - Flags: feedback?(cku) → feedback+
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().
Attachment #8443393 - Flags: feedback?(jwwang) → feedback+
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?
Attachment #8443393 - Flags: feedback?(jolin) → feedback+
Testcase test_mediarecorder_creation_fail.html failed because I increase the Session's refcnt too late for DoSessionEndTask that will reference the session.
Attached patch bug-1016265.patch (obsolete) — Splinter Review
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
Attachment #8443393 - Attachment is obsolete: true
Attachment #8445774 - Flags: review?(roc)
r=roc
Attachment #8445774 - Attachment is obsolete: true
Attachment #8446417 - Flags: review+
Keywords: checkin-needed
Retriggers of B2G M3 are showing a lot of occurrences of bug 991776. Seems unlikely to be a coincidence?
Keywords: checkin-needed
Since Bug 99177 had been resolved, I'm trying to push this patch again.
https://tbpl.mozilla.org/?tree=Try&rev=f5ec95c5088f
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bfd5fb37afb5
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.