Closed Bug 1239983 Opened 4 years ago Closed 4 years ago

Crash in mp4_demuxer::MP4Metadata::GetNumberTracks - Add diagnostic assertions

Categories

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

42 Branch
x86
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox42 --- wontfix
firefox43 --- wontfix
firefox44 --- affected
firefox45 + fixed
firefox46 --- fixed
firefox47 --- fixed

People

(Reporter: philipp, Assigned: gerald)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-cf810184-5435-4c8c-ad22-f171b2160114.
=============================================================
0 	xul.dll 	mp4_demuxer::MP4Metadata::GetNumberTracks(mozilla::TrackInfo::TrackType) 	media/libstagefright/binding/MP4Metadata.cpp
1 	xul.dll 	mozilla::TrackBuffersManager::OnDemuxerInitDone(nsresult) 	dom/media/mediasource/TrackBuffersManager.cpp
2 	xul.dll 	mozilla::MozPromise<nsresult, mozilla::DemuxerFailureReason, 1>::MethodThenValue<mozilla::TrackBuffersManager, void ( mozilla::TrackBuffersManager::*)(nsresult), void ( mozilla::TrackBuffersManager::*)(mozilla::DemuxerFailureReason)>::DoResolveOrRejectInternal(mozilla::MozPromise<nsresult, mozilla::DemuxerFailureReason, 1>::ResolveOrRejectValue const&) 	xpcom/threads/MozPromise.h
3 	xul.dll 	mozilla::MozPromise<nsresult, mozilla::DemuxerFailureReason, 1>::ThenValueBase::DoResolveOrReject(mozilla::MozPromise<nsresult, mozilla::DemuxerFailureReason, 1>::ResolveOrRejectValue const&) 	xpcom/threads/MozPromise.h
4 	xul.dll 	mozilla::MozPromise<nsresult, mozilla::DemuxerFailureReason, 1>::ThenValueBase::ResolveOrRejectRunnable::Run() 	xpcom/threads/MozPromise.h
5 	xul.dll 	mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run() 	xpcom/threads/TaskDispatcher.h
6 	xul.dll 	mozilla::TaskQueue::Runner::Run() 	xpcom/threads/TaskQueue.cpp
7 	xul.dll 	nsThreadPool::Run() 	xpcom/threads/nsThreadPool.cpp
8 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp
9 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/glue/nsThreadUtils.cpp
10 	xul.dll 	mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
11 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc
12 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc
13 	xul.dll 	nsThread::ThreadFunc(void*) 	xpcom/threads/nsThread.cpp
14 	nss3.dll 	_PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c
15 	nss3.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c
16 	msvcr120.dll 	_callthreadstartex 	f:\dd\vctools\crt\crtw32\startup\threadex.c:376
17 	msvcr120.dll 	msvcr120.dll@0x2c000 	
18 	kernel32.dll 	BaseThreadInitThunk 	
19 	ntdll.dll 	__RtlUserThreadStart 	
20 	ntdll.dll 	_RtlUserThreadStart

this media crash has started occurring in larger volume since firefox 42.0 and accross all windows versions. currently on 44.0b8 it causes ~0.5% of all crashes.
Component: Audio/Video → Audio/Video: Playback
Can you look into this one?
Flags: needinfo?(gsquelart)
Priority: -- → P1
Slightly different signature on Mac.

My guess at this time is that something happens to TracksBufferManager::mInputDemuxer between the time the init/reset task is started and when the promise is resolved.
I have not been able to reproduce it yet.
Assignee: nobody → gsquelart
Crash Signature: [@ mp4_demuxer::MP4Metadata::GetNumberTracks] → [@ mp4_demuxer::MP4Metadata::GetNumberTracks] [@ mp4_demuxer::MP4Metadata::GetNumberTracks const ]
Flags: needinfo?(gsquelart)
(In reply to philipp from comment #0)
> 0 	xul.dll mp4_demuxer::MP4Metadata::GetNumberTracks(mozilla::TrackInfo::TrackType) media/libstagefright/binding/MP4Metadata.cpp
> 1 	xul.dll 	mozilla::TrackBuffersManager::OnDemuxerInitDone(nsresult) dom/media/mediasource/TrackBuffersManager.cpp
> ...

Also in some reports, GetNumberTracks is actually called from TrackBuffersManager::OnDemuxerResetDone.
[Tracking Requested - why for this release]: This seems like a common enough crash. We are investigating.
Added diagnostics around demuxer init/reset promises, to catch early cases
where TrackBuffersManager::mInputDemuxer is destroyed while an init/reset
promise is in flight, which would cause later issues when the 'Then' clause
assumes that mInputDemuxer still exists.

Note: This is *not* an actual fix for crashes linked to this bug, but it should
help identify unexpected situations, or instead eliminate these patched
locations as sources of crashes.
A new bug will be needed to examine the fallout from this patch and produce a
real fix, or continue investigating.

Review commit: https://reviewboard.mozilla.org/r/33217/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33217/
Attachment #8714667 - Flags: review?(jyavenard)
Attachment #8714667 - Flags: review?(jyavenard)
Comment on attachment 8714667 [details]
MozReview Request: Bug 1239983 - Diags around TrackBuffersMgr promises - r?jya

https://reviewboard.mozilla.org/r/33217/#review29939

::: dom/media/mediasource/TrackBuffersManager.cpp:851
(Diff revision 1)
> +  MOZ_DIAGNOSTIC_ASSERT(mInputDemuxer.get());

You shouldn't need get() here.
Comment on attachment 8714667 [details]
MozReview Request: Bug 1239983 - Diags around TrackBuffersMgr promises - r?jya

https://reviewboard.mozilla.org/r/33217/#review29941
Attachment #8714667 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/c481014f50c2
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Gerald, do you want to uplift this to aurora/beta? Thanks

Tracking as it is a crash.
(In reply to Sylvestre Ledru [:sylvestre] from comment #11)
> Gerald, do you want to uplift this to aurora/beta? Thanks

As per comment 5, this patch is *not* a fix, but will hopefully help with solving the root cause.

However, after 6 days no corresponding crash has shown up in Socorro yet, so I think either the issue has been magically solved somewhere else, or there is not enough coverage with just Nightly to get this kind of crash. In either case, uplifting should help (either nothing bad will happen, or we will finally hit something interesting).

So I will request uplift, but I just wanted to be clear about what this patch is.
Flags: needinfo?(gsquelart)
OK, So, the bug was incorrectly fixed then, right?
Comment on attachment 8714667 [details]
MozReview Request: Bug 1239983 - Diags around TrackBuffersMgr promises - r?jya

Approval Request Comment
[Feature/regressing bug #]: MP4 playback.
[User impact if declined]: A source of crashes is less likely to get fixed in the short term.
[Describe test coverage new/current, TreeHerder]: Lots of media tests, https://treeherder.mozilla.org/#/jobs?repo=try&revision=167c01f5fdd1 , 6 days on Nightly.
[Risks and why]: This patch intends to cause diagnostic crashes, but if they happen, it means we have reached a point where a crash would happen soon after anyway, so it shouldn't cause any more actual harm than normal. Bug 1245463 is already setup to regularly monitor crashes* and take further actions as soon as more data is available.
[String/UUID change made/needed]: None.

* Looking at crashes involving GetNumberTracks, I can see a few reports in Beta, but none in Aurora. So even though it might be safer to only uplift to Aurora, I think we should indeed uplift to Beta as well so that there are more chances to highlight this issue sooner.
Attachment #8714667 - Flags: approval-mozilla-beta?
Attachment #8714667 - Flags: approval-mozilla-aurora?
(In reply to Sylvestre Ledru [:sylvestre] from comment #13)
> OK, So, the bug was incorrectly fixed then, right?

The actual issue (Crashes in GetNumberTracks) is not actually fixed yet.
But this bug is "fixed", as in: The patch has landed and will hopefully help catch the real culprit in follow-up bug 1243463.
Sorry, that should be follow-up bug 1245463.
Summary: crash in mp4_demuxer::MP4Metadata::GetNumberTracks → Crash in mp4_demuxer::MP4Metadata::GetNumberTracks - Add diagnostic assertions
Aurora try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0544f33d1e7b
Beta try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6788fd0e748b
(None of the failures can be due to this patch, it would appear as a recognizable crash)
Comment on attachment 8714667 [details]
MozReview Request: Bug 1239983 - Diags around TrackBuffersMgr promises - r?jya

Hopefully, will help debugging, taking it.
Should be in 45 beta 5.
Attachment #8714667 - Flags: approval-mozilla-beta?
Attachment #8714667 - Flags: approval-mozilla-beta+
Attachment #8714667 - Flags: approval-mozilla-aurora?
Attachment #8714667 - Flags: approval-mozilla-aurora+
I'm highly confident bug 1245463 will fix it.
You need to log in before you can comment on or make changes to this bug.