1115749 - More mp4 demuxer races (MP4Reader::Seek)

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

Sigh.

0   libsystem_kernel.dylib        	0x00007fff87048282 __pthread_kill + 10
1   libsystem_c.dylib             	0x00007fff8fa5fb73 abort + 129
2   libnss3.dylib                 	0x000000010133c7ad PR_Assert + 109
3   libnss3.dylib                 	0x000000010136cc18 PR_AssertCurrentThreadOwnsLock + 104 (ptsynch.c:227)
4   XUL                           	0x00000001015c5549 mozilla::OffTheBooksMutex::AssertCurrentThreadOwns() const + 25
5   XUL                           	0x00000001015c5525 mozilla::Monitor::AssertCurrentThreadOwns() const + 21
6   XUL                           	0x00000001015b3400 mp4_demuxer::MoofParser::Moofs() + 32
7   XUL                           	0x0000000101596b8f mp4_demuxer::SampleIterator::Get() + 95
8   XUL                           	0x0000000101596f91 mp4_demuxer::SampleIterator::Seek(long long) + 65
9   XUL                           	0x000000010159afcd mp4_demuxer::MP4Demuxer::SeekAudio(long long) + 109
10  XUL                           	0x0000000104298ca2 mozilla::MP4Reader::Seek(long long, long long, long long, long long) + 626
11  XUL                           	0x00000001041ab2a0 mozilla::MediaSourceReader::AttemptSeek() + 544
12  XUL                           	0x00000001041ab73c mozilla::MediaSourceReader::Seek(long long, long long, long long, long long) + 492
13  XUL                           	0x00000001040c1caa mozilla::MediaDecoderStateMachine::DecodeSeek() + 1114
14  XUL                           	0x000000010410f17a nsRunnableMethodImpl<void (mozilla::MediaDecoderStateMachine::*)(), void, true>::Run() + 154
15  XUL                           	0x00000001040e7389 mozilla::MediaTaskQueue::Runner::Run() + 633
16  XUL                           	0x0000000101702a27 nsThreadPool::Run() + 967
17  XUL                           	0x0000000101702b1c non-virtual thunk to nsThreadPool::Run() + 28
18  XUL                           	0x00000001016ff488 nsThread::ProcessNextEvent(bool, bool*) + 2088
19  XUL                           	0x0000000101758697 NS_ProcessNextEvent(nsIThread*, bool) + 151
20  XUL                           	0x0000000101d82d91 mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) + 961
21  XUL                           	0x0000000101d1a105 MessageLoop::RunInternal() + 117
22  XUL                           	0x0000000101d1a015 MessageLoop::RunHandler() + 21
23  XUL                           	0x0000000101d19fbd MessageLoop::Run() + 45
24  XUL                           	0x00000001016fd916 nsThread::ThreadFunc(void*) + 358
25  libnss3.dylib                 	0x000000010137203f _pt_root + 463 (ptthread.c:215)
26  libsystem_pthread.dylib       	0x00007fff8a7c82fc _pthread_body + 131
27  libsystem_pthread.dylib       	0x00007fff8a7c8279 _pthread_start + 176
28  libsystem_pthread.dylib       	0x00007fff8a7c64b1 thread_start + 13

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Rename mIndexMonitor to mDemuxerMonitor. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Hold the demuxer monitor in MP4Reader::Seek. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 3 - Hold the demuxer lock while accessing metadata. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 4 - Assert that we hold the monitor at all the MP4Demuxer API entry points. v1

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8541813 [details] [diff] [review]
Part 3 - Hold the demuxer lock while accessing metadata. v1

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

::: dom/media/fmp4/MP4Reader.cpp
@@ +413,5 @@
> +  Microseconds duration;
> +  {
> +    MonitorAutoLock lock(mDemuxerMonitor);
> +    duration = mDemuxer->Duration();
> +  }

you already own the lock.

@@ +421,5 @@
>    }
>  
>    *aInfo = mInfo;
>    *aTags = nullptr;
>  

also need to remove the lock   MonitorAutoLock mon(mIndexMonitor); before Update() line 424

Comment 8

[Bobby Holley (:bholley)]

(In reply to Jean-Yves Avenard [:jya] from comment #7)
> you already own the lock.

No, it goes out of scope above.

> also need to remove the lock   MonitorAutoLock mon(mIndexMonitor); before
> Update() line 424

No, for the same reason.

Comment 9

[Jean-Yves Avenard [:jya]]

Duh... Will crawl back into my hole :)

Comment 10

[Ralph Giles (:rillian)]

Comment on attachment 8541811 [details] [diff] [review]
Part 1 - Rename mIndexMonitor to mDemuxerMonitor. v1

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

Stealing review since k17e is on vacation and this is causing problems with debug builds.

Comment 11

[Ralph Giles (:rillian)]

Comment on attachment 8541814 [details] [diff] [review]
Part 4 - Assert that we hold the monitor at all the MP4Demuxer API entry points. v1

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

r=me, stealing review.

Comment 12

[Ralph Giles (:rillian)]

Comment on attachment 8541813 [details] [diff] [review]
Part 3 - Hold the demuxer lock while accessing metadata. v1

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

Stealing review. r=me

::: dom/media/fmp4/MP4Reader.cpp
@@ +413,5 @@
> +  Microseconds duration;
> +  {
> +    MonitorAutoLock lock(mDemuxerMonitor);
> +    duration = mDemuxer->Duration();
> +  }

No, this is ok. The previous lock goes out of scope at the end of 'if (!mDemuxerInitialized)' at the start of the function.

@@ +421,5 @@
>    }
>  
>    *aInfo = mInfo;
>    *aTags = nullptr;
>  

Likewise.

Comment 13

[Ralph Giles (:rillian)]

Comment on attachment 8541812 [details] [diff] [review]
Part 2 - Hold the demuxer monitor in MP4Reader::Seek. v1

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

Stealing review. r=me with comments addressed.

Could you please put some motivation in the body of the commit message for future reference. Why is it important to take the monitor here but not on other methods?

::: dom/media/fmp4/MP4Reader.h
@@ +101,5 @@
>  
>    // Blocks until the demuxer produces an sample of specified type.
>    // Returns nullptr on error on EOS. Caller must delete sample.
>    mp4_demuxer::MP4Sample* PopSample(mp4_demuxer::TrackType aTrack);
> +  mp4_demuxer::MP4Sample* PopSampleLocked(mp4_demuxer::TrackType aTrack);

It's unfortunate you need two versions here.

Is there no way to prevent deadlock if one calls PopSample() instead of PopSampleLocked()?

Comment 14

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c603370ae39d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5640e21c2544
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ed1950e4c697
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/30c89bc08ce5

Comment 15

[Ralph Giles (:rillian)]

Per IRC discussion, the answer to my deadlock question was 'no'.

Comment 16

[Andrew McCreight [:mccr8]]

Given all the crashes that are showing up from this, it doesn't seem out of the question that something bad could happen with this, so I'm going to mark it as sec-high.  Feel free to adjust as needed.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c603370ae39d
https://hg.mozilla.org/mozilla-central/rev/5640e21c2544
https://hg.mozilla.org/mozilla-central/rev/ed1950e4c697
https://hg.mozilla.org/mozilla-central/rev/30c89bc08ce5

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Was this trunk only?

Comment 19

[Bobby Holley (:bholley)]

(In reply to Al Billings [:abillings] from comment #18)
> Was this trunk only?

Given that we're aggressively uplifting all media patches to 36 for MSE, this also affects aurora.

Speaking of which, Ralph, can you uplift this?

Comment 20

[Al Billings [:abillings - ex-MoCo]]

I was curious as to whether it affected ESR31 or not.

Comment 21

[Ralph Giles (:rillian)]

(In reply to Al Billings [:abillings] from comment #20)
> I was curious as to whether it affected ESR31 or not.

No. This code is pref'd off prior to Firefox 36.

Comment 22

[Bobby Holley (:bholley)]

(In reply to Ralph Giles (:rillian) from comment #21)
> (In reply to Al Billings [:abillings] from comment #20)
> > I was curious as to whether it affected ESR31 or not.
> 
> No. This code is pref'd off prior to Firefox 36.

The NI was from comment 19.

Comment 23

[Ralph Giles (:rillian)]

Comment on attachment 8541811 [details] [diff] [review]
Part 1 - Rename mIndexMonitor to mDemuxerMonitor. v1

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent testing, sites more likely to serve flash video.
[Describe test coverage new/current, TBPL]: Landed on m-c.
[Risks and why]: Low. These changes are MSE-specific.
[String/UUID change made/needed]: None.

This request applies to all patches on this bug.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7318fa1f151c
https://hg.mozilla.org/releases/mozilla-aurora/rev/883b891354ae
https://hg.mozilla.org/releases/mozilla-aurora/rev/c55d5658c652
https://hg.mozilla.org/releases/mozilla-aurora/rev/eb36d731bf47