Closed Bug 975858 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Audio/Video, defect)

29 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: cpearce, Assigned: cpearce)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
When the refcount of a MediaResource drops to 0, dispatch an event to the main thread to delete the resource.
Attachment #8380380 - Flags: review?(roc)
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.
Attachment #8380380 - Flags: review?(roc) → review-
Using NS_ProxyRelease. I had to also make MediaResource inherit from nsISupports, but it was already refcounted, so no big deal.
Attachment #8380380 - Attachment is obsolete: true
Attachment #8380950 - Flags: review?(roc)
https://hg.mozilla.org/mozilla-central/rev/1fffb30a79f2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.