Closed Bug 1182287 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Audio/Video, defect, critical)

41 Branch
x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox40 --- unaffected
firefox41 + fixed
firefox42 + fixed

People

(Reporter: dmajor, Assigned: jya)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file, 2 obsolete files)

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?
Flags: needinfo?(bobbyholley)
[Tracking Requested - why for this release]: This is a top crash on aurora 41.
Attachment #8631850 - Flags: review?(giles)
Assignee: nobody → jyavenard
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.
Attachment #8631865 - Flags: review?(karlt)
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.
Attachment #8631850 - Flags: review?(giles) → review+
(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.
Blocks: 1175768
Keywords: regression
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.
Attachment #8631865 - Flags: review?(karlt) → review-
Attachment #8631850 - Attachment is obsolete: true
Attachment #8631865 - Attachment is obsolete: true
Attachment #8631945 - Flags: review?(karlt) → review+
Tracking for 41, 42- see comment 1.
This landed yesterday - https://hg.mozilla.org/mozilla-central/rev/ac46c38d047f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
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
Attachment #8631945 - Flags: approval-mozilla-aurora?
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.
Attachment #8631945 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(bobbyholley)
You need to log in before you can comment on or make changes to this bug.