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

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Matthew Gregan [:kinetik]]

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

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: bug1104357_v0.patch

Possible fix.

Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=94c74b10a3dc

Comment 2

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

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.

Comment 3

[Matthew Gregan [:kinetik]]

Attachment: bug1104357_v1.patch

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

Comment 4

[Matthew Gregan [:kinetik]]

Attachment: bug1104357_v2.patch

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.

Comment 5

[Matthew Gregan [:kinetik]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=821253707d92

Comment 6

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0cea8df69681

Bug 1104421 covers asserting MDR::Shutdown().

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0cea8df69681