1182287 - crash in mozilla::MediaSourceDecoder::IsActiveReader(mozilla::MediaDecoderReader*)

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[(Away)]

This bug was filed from the Socorro interface and is 
report bp-0b65753c-db30-4140-a31f-ea6fe2150708.
=============================================================

xul!mozilla::MediaSourceDecoder::IsActiveReader
xul!<lambda_c3c182eb89b8d5d2fe16e26422c4fee7>::operator()
xul!nsRunnableFunction<<lambda_c3c182eb89b8d5d2fe16e26422c4fee7> >::Run
xul!mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run
xul!nsThread::ProcessNextEvent

self->mParentDecoder is null in the lambda launched from RemoveEmptyDecoders (yeah, I know there's a null-check a little earlier :( ). Possibly from bug 1175768?

Comment 1

[(Away)]

[Tracking Requested - why for this release]: This is a top crash on aurora 41.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Test that MediaSourceReader hasn't been shutdown.

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: P2. Check TrackBuffer::Shutdown() wasn't called.

If this lambda was waiting on the monitor, and that monitor was currently held by TrackBuffer::ContinueShutdown() it would be possible to find TrackBuffer::mParentDecoder set to null.

Comment 4

[Ralph Giles (:rillian)]

Comment on attachment 8631850 [details] [diff] [review]
Test that MediaSourceReader hasn't been shutdown.

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

Messy that you can't assert mReader like the other methods.

Comment 5

[Karl Tomlinson (:karlt)]

(In reply to David Major [:dmajor] from comment #0)
> Possibly from bug 1175768?

Yes, this is a regression from moving the mParentDecoder read to an event that
may run concurrently with ContinueShutdown().
http://hg.mozilla.org/releases/mozilla-aurora/diff/f5b56f26141b/dom/media/mediasource/TrackBuffer.cpp#l1.378

Why does this code need to run from a different event?

The commit message says "Use mirroring for buffered ranges" but I don't know
exactly how that affects this code.

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 8631865 [details] [diff] [review]
P2. Check TrackBuffer::Shutdown() wasn't called.

(In reply to Jean-Yves Avenard [:jya] from comment #3)
> If this lambda was waiting on the monitor, and that monitor was currently
> held by TrackBuffer::ContinueShutdown() it would be possible to find
> TrackBuffer::mParentDecoder set to null.

Yes, but this is not fixing the problem that it is not safe to read
mParentDecoder on the main thread outside the monitor after the
MediaSourceDecoder has shut down.

>       if (!self->mParentDecoder) {
>         return;
>       }
>       ReentrantMonitorAutoEnter mon(self->mParentDecoder->GetReentrantMonitor());
> 
>+      if (!self->mParentDecoder) {

The first mParentDecoder read in the first test (and when attempting to find
the monitor) may return anything if mParentDecoder is being modified
concurrently.

Perhaps it is possible to make this work through taking a reference to the
mParentDecoder or the monitor before dispatching the event, but I'd first like
to know why we need the event.

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: Partial revert of bug 1175768, immediately runs TrackBuffer::RemoveEmptyDecoders

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac46c38d047f

Comment 9

[Kate Glazko]

Tracking for 41, 42- see comment 1.

Comment 10

[Guilherme Lima]

This landed yesterday - https://hg.mozilla.org/mozilla-central/rev/ac46c38d047f

Comment 11

[Jean-Yves Avenard [:jya]]

Comment on attachment 8631945 [details] [diff] [review]
Partial revert of bug 1175768, immediately runs TrackBuffer::RemoveEmptyDecoders

Approval Request Comment
[Feature/regressing bug #]:1175768
[User impact if declined]: Crashes
[Describe test coverage new/current, TreeHerder]: In central for a few days, tested locally
[Risks and why]: Low, It's a reversal of some changes going back to the old behaviour.
[String/UUID change made/needed]: None

Comment 12

[Kate Glazko]

Comment on attachment 8631945 [details] [diff] [review]
Partial revert of bug 1175768, immediately runs TrackBuffer::RemoveEmptyDecoders

Low risk patch, on m-c with no known issues.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cf04b592d209