1351053 - Intermittent Main app process exited normally | application crashed [@ mozilla::MediaFormatReader::DemuxerProxy::Wrapper::~Wrapper()]

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P1)

Description

[Treeherder Bug Filer]

Filed by: wkocher [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=86693803&repo=autoland

https://archive.mozilla.org/pub/firefox/tinderbox-builds/autoland-macosx64-debug/autoland_yosemite_r7-debug_test-mochitest-e10s-browser-chrome-4-bm132-tests1-macosx-build199.txt.gz

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

https://treeherder.mozilla.org/logviewer.html#?job_id=86693803&repo=autoland&lineNumber=19318

Assertion failure: !mOwningThread (destroying a still-owned lock!), at /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Mutex.h:57

It looks like a memory corruption to me because mOwningThread is initialized to nullptr and never modified again.

http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/xpcom/threads/Mutex.h#49

Hi Nathan,
Do you have any idea about what this assertion is used for?

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #1)
> It looks like a memory corruption to me because mOwningThread is initialized
> to nullptr and never modified again.
> 
> http://searchfox.org/mozilla-central/rev/
> 7419b368156a6efa24777b21b0e5706be89a9c2f/xpcom/threads/Mutex.h#49

Sure it's modified:

http://searchfox.org/mozilla-central/source/xpcom/threads/BlockingResourceBase.cpp#383

and several other places in that file.

> Do you have any idea about what this assertion is used for?

Just what it sounds like, ensuring that you don't destroy the mutex while somebody owns the lock.

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

This still looks like a memory corruption to me:
1. mMutex is always used with MutexAutoLock in the member functions of MediaFormatReader::DemuxerProxy::Wrapper. There is no way for the lock is still held while ~Wrapper() is happening.
2. From the stack trace [2], I can't find any thread that is calling member functions of Wrapper expect Thread 31 which is in the destructor. So there is no way for the lock to be held by any thread.

[1] http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/dom/media/MediaFormatReader.cpp#954
[2] https://archive.mozilla.org/pub/firefox/tinderbox-builds/autoland-macosx64-debug/autoland_yosemite_r7-debug_test-mochitest-e10s-browser-chrome-4-bm132-tests1-macosx-build199.txt.gz

Comment 4

[Intermittent Failures Robot]

6 failures in 845 pushes (0.007 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* mozilla-inbound: 2
* autoland: 2
* oak: 1
* graphics: 1

Platform breakdown:
* osx-10-10: 6

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1351053&startday=2017-03-27&endday=2017-04-02&tree=all

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1351053. P1 - add some assertions to debug who is holding the lock.

Review commit: https://reviewboard.mozilla.org/r/126728/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126728/

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8854753 [details]
Bug 1351053. P1 - add some assertions to debug who is holding the lock.

https://reviewboard.mozilla.org/r/126728/#review129404

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Thanks!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/da84999fd0b2
P1 - add some assertions to debug who is holding the lock. r=jya

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/da84999fd0b2

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

This bug is regressed by bug 1319992.

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1351053. P1 - Backed out changeset da84999fd0b2.

Review commit: https://reviewboard.mozilla.org/r/130156/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130156/

Comment 12

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1351053. P2 - fix data race in accessing mAudioDemuxer/mVideoDemuxer.

1. http://searchfox.org/mozilla-central/rev/944f87c575e8a0bcefc1ed8efff10b34cf7a5169/dom/media/MediaFormatReader.cpp#755
   mAudioDemuxer is modified on the task queue thread of MFR in Shutdown() called by MFR::Shutdown().
2. http://searchfox.org/mozilla-central/rev/944f87c575e8a0bcefc1ed8efff10b34cf7a5169/dom/media/MediaFormatReader.cpp#1082
   mAudioDemuxer is read on the task queue thread of DemuxerProxy in DemuxerProxy::NotifyDataArrived().

Review commit: https://reviewboard.mozilla.org/r/130158/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130158/

Comment 13

[Jean-Yves Avenard [:jya]]

Comment on attachment 8858206 [details]
Bug 1351053. P1 - Backed out changeset da84999fd0b2.

https://reviewboard.mozilla.org/r/130156/#review132826

Comment 14

[Jean-Yves Avenard [:jya]]

Comment on attachment 8858207 [details]
Bug 1351053. P2 - fix data race in accessing mAudioDemuxer/mVideoDemuxer.

https://reviewboard.mozilla.org/r/130158/#review132828

good catch

Comment 15

[JW Wang [:jwwang] [:jw_wang]]

Thanks for the review!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/712f722c8b3c
P1 - Backed out changeset da84999fd0b2. r=jya
https://hg.mozilla.org/integration/autoland/rev/c557507c83a1
P2 - fix data race in accessing mAudioDemuxer/mVideoDemuxer. r=jya

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/712f722c8b3c
https://hg.mozilla.org/mozilla-central/rev/c557507c83a1

Comment 18

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8858207 [details]
Bug 1351053. P2 - fix data race in accessing mAudioDemuxer/mVideoDemuxer.

Approval Request Comment
[Feature/Bug causing the regression]:1319992
[User impact if declined]:crash
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]:low
[Why is the change risky/not risky?]:the change is simple.
[String changes made/needed]:none

Comment 19

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8858207 [details]
Bug 1351053. P2 - fix data race in accessing mAudioDemuxer/mVideoDemuxer.

fix a race, aurora54+

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3c96fce8a837