975858 - MediaResource still isn't safe to release on non-main threads

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

In bug 929029 we tried to make MediaResource safe to release on non-main threads. We missed a case; ChannelMediaResource is composed of a MediaCacheStream, and that asserts that it should be released on the main thread and interacts with the global mediacache instance assuming it's running on the main thread. So it's not threadsafe.

I could reproduce a crash in a Windows Opt build when looping over lots of MP4 test cases in test_seek with PARALLEL_TESTS=20, which I tracked down to this.

I propose we fix this once and for all by making MediaResource::Release() detect when the refcnt drops to 0 and dispatching an event to the main thread to delete the MediaResource. So we ensure the dtor always runs on the main thread.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch: Always delete MediaResource on the main thread

When the refcount of a MediaResource drops to 0, dispatch an event to the main thread to delete the resource.

Comment 2

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

Comment on attachment 8380380 [details] [diff] [review]
Patch: Always delete MediaResource on the main thread

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

::: content/media/MediaResource.cpp
@@ +75,5 @@
> +    // set to 1 by refcount stabilization by Release() calling us, and the
> +    // refcount should be incremented by ReleaseOnMainThread's ref ptr.
> +    MOZ_ASSERT(aResource->mRefCnt == 2);
> +    NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);
> +    event = nullptr;

I think you can just use NS_ProxyRelease.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2: Using proxy release

Using NS_ProxyRelease. I had to also make MediaResource inherit from nsISupports, but it was already refcounted, so no big deal.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://tbpl.mozilla.org/?tree=Try&rev=234525f6db22

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Err, I meant: https://tbpl.mozilla.org/?tree=Try&rev=30bd8bcbd8ac

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1fffb30a79f2

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1fffb30a79f2