Closed Bug 1104357 Opened 9 years ago Closed 9 years ago

MediaBufferDecoder does not call MediaDecoderReader::Shutdown on some error paths

Categories

(Core :: Web Audio, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: kinetik, Assigned: kinetik)

References

Details

Attachments

(1 file, 2 obsolete files)

MediaBufferDecoder only calls Shutdown() and BreakCycles() after succcessfully decoding the entire media stream.  It should be called in all cases.

This shows up as an assert caused by WebMReader's MediaTaskQueue not being Shutdown() before it is destroyed.  See failing test test_mediaDecoding.html on Windows here: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=09494ecc9bbe
Attached patch bug1104357_v0.patch (obsolete) — Splinter Review
Possible fix.

Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=94c74b10a3dc
Attachment #8528013 - Flags: review?(cpearce)
Comment on attachment 8528013 [details] [diff] [review]
bug1104357_v0.patch

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

I think it's OK to call BreakCycles on the main thread, but MediaDecoderReader::Shutdown should happen on a decode thread, or at least on a thread from a SharedThreadPool. The Readers expect this, and I think it's particularly important in the case where we're using WMF or DirectShow which interacts with MSCOM.
Attachment #8528013 - Flags: review?(cpearce) → review-
Attached patch bug1104357_v1.patch (obsolete) — Splinter Review
Thanks, I had a feeling that was the case but wasn't sure.  Here's a version that calls BreakCycles() from Cleanup() and calls Shutdown() whenever Decode() returns.

Also adds an assertion to the base reader to ensure Shutdown() is called (kind of ugly, but DebugOnly<> doesn't seem like a great option for member variables) and an assertion to verify Shutdown() is called from the appropriate thread.

Let's see if that breaks anything: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=eacb26905650
Attachment #8528013 - Attachment is obsolete: true
Attachment #8528048 - Flags: review?(cpearce)
Attachment #8528048 - Attachment is obsolete: true
Attachment #8528048 - Flags: review?(cpearce)
Okay, time for the simple approach.  Call Shutdown() in the existing error exit paths, move BreakCycles() to Cleanup() to avoid having to call it from multiple places.

Using a scope guard to call Shutdown doesn't work, because the event dispatched by ReportFailureOnMainThread could run before the scope guard's destructor, which results in unsafe access of mDecoderReader.

MOZ_ASSERT(mIsShutdown) had too many causalities on try, so I'll spin off a new bug for that.
Attachment #8528106 - Flags: review?(cpearce)
Attachment #8528106 - Flags: review?(cpearce) → review+
https://hg.mozilla.org/mozilla-central/rev/0cea8df69681
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Target Milestone: mozilla37 → mozilla36
You need to log in before you can comment on or make changes to this bug.