Closed Bug 1191889 Opened 5 years ago Closed 5 years ago

crash in mozilla::ReentrantMonitorAutoEnter::ReentrantMonitorAutoEnter(mozilla::ReentrantMonitor&)

Categories

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

42 Branch
Unspecified
Windows NT
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla43
Tracking Status
firefox41 + fixed
firefox42 + fixed
firefox43 --- fixed

People

(Reporter: dmajor, Assigned: karlt)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-4e29e573-1031-4a97-bbda-f626f2150805.
=============================================================

This crash is generally pretty low volume, but one Aurora user hit it 83 times, so it bubbled up on the reports.

gMediaCache->GetReentrantMonitor() is null in MediaCacheStream::Close.

It claims to be Win64-only but I don't believe that. I bet 32-bit builds just got another signature due to inlining differences.

0 	xul.dll 	mozilla::ReentrantMonitorAutoEnter::ReentrantMonitorAutoEnter(mozilla::ReentrantMonitor&) 	xpcom/glue/ReentrantMonitor.h
1 	xul.dll 	mozilla::ChannelMediaResource::Close() 	dom/media/MediaResource.cpp
2 	xul.dll 	mozilla::MediaDecoder::Shutdown() 	dom/media/MediaDecoder.cpp
3 	xul.dll 	mozilla::dom::HTMLMediaElement::ShutdownDecoder() 	dom/html/HTMLMediaElement.cpp
4 	xul.dll 	mozilla::dom::HTMLMediaElement::FinishDecoderSetup(mozilla::MediaDecoder*, mozilla::MediaResource*, nsIStreamListener**, mozilla::MediaDecoder*) 	dom/html/HTMLMediaElement.cpp
5 	xul.dll 	mozilla::dom::HTMLMediaElement::InitializeDecoderForChannel(nsIChannel*, nsIStreamListener**) 	dom/html/HTMLMediaElement.cpp
6 	xul.dll 	mozilla::dom::HTMLMediaElement::MediaLoadListener::OnStartRequest(nsIRequest*, nsISupports*) 	dom/html/HTMLMediaElement.cpp
7 	xul.dll 	mozilla::net::HttpChannelChild::DoOnStartRequest(nsIRequest*, nsISupports*) 	netwerk/protocol/http/HttpChannelChild.cpp
Shows 36 crashes on 42.
Blocks: MSE
It now shows up as 2.5% of 41.0b1 crashes.
(In reply to David Major [:dmajor] from comment #0)
> It claims to be Win64-only but I don't believe that. I bet 32-bit builds
> just got another signature due to inlining differences.

(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #2)
> It now shows up as 2.5% of 41.0b1 crashes.

I've confirmed that there is a 32-bit counterpart to this signature at [@ mozilla::MediaCacheStream::Close() ]. Which makes the total volume more like 4.5%.
Crash Signature: [@ mozilla::ReentrantMonitorAutoEnter::ReentrantMonitorAutoEnter(mozilla::ReentrantMonitor&)] → [@ mozilla::ReentrantMonitorAutoEnter::ReentrantMonitorAutoEnter(mozilla::ReentrantMonitor&)] [@ mozilla::MediaCacheStream::Close() ]
Flags: needinfo?(jyavenard)
[Tracking Requested - why for this release]: media topcrash
First seen in nightly 20150606030209: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0496b5b3e9ef&tochange=4a07e1ac3cdf

Karl, possibly fallout from bug 1116382?
Flags: needinfo?(karlt)
(In reply to David Major [:dmajor] from comment #3)
> I've confirmed that there is a 32-bit counterpart to this signature at [@
> mozilla::MediaCacheStream::Close() ].

Oh, I didn't know they were the same, I thought the other one was bug 1171244. Thanks for clearing that up!
See Also: → 1171244
MSE doesn't use cached data ; mozilla::MediaCacheStream wouldn't be involved
Flags: needinfo?(jyavenard)
(In reply to David Major [:dmajor] from comment #6)
> First seen in nightly 20150606030209:
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=0496b5b3e9ef&tochange=4a07e1ac3cdf
> 
> Karl, possibly fallout from bug 1116382?

Thanks.  Look like https://hg.mozilla.org/mozilla-central/rev/990ac59aeaae#l1.67 may be suspicious.

And FinishDecoderSetup() is on the stack in some versions of
[@ mozilla::MediaCacheStream::Close() ]:

bp-ef2a91e2-7aff-4978-8770-595992150814
Assignee: nobody → karlt
Status: NEW → ASSIGNED
Flags: needinfo?(karlt)
Offset 0x8 in 32 and 64 bit reports is consistent with MediaCache::mReentrantMonitor

http://hg.mozilla.org/releases/mozilla-aurora/annotate/57564319395c/dom/media/MediaCache.cpp#l331

so it seems with have a null gMediaCache in MediaCacheStream::Close().

http://hg.mozilla.org/releases/mozilla-aurora/annotate/57564319395c/dom/media/MediaCache.cpp#l1982
Component: Audio/Video → Audio/Video: Playback
ediaCacheStream::Init() is called from ChannelMediaResource::Open(), which
can fail after calling Init().  ChannelMediaResource::Open() is called from
MediaDecoder::OpenResource(), from MediaDecoder::Load(), from
FinishDecoderSetup()

MediaCacheStream::Init() can fail when !gMediaCache after InitMediaCache().
That can happen if gMediaCache->Init() fails, and Init() fails if
NS_OpenAnonymousTemporaryFile() or FileBlockCache::Open() fails.
FileBlockCache::Open() can fail if a new thread cannot be created.

When MediaCacheStream::Close() is called and Init() has not succeeded, then
gMediaCache may be null, which would lead to this crash.

MediaShutdownManager adds a ref to MediaDecoders in Register() called from
MediaDecoder::Init() called from DecoderTraits::CreateDecoder().
Unregister() not called unless Shutdown() is called.
So MediaDecoders leak until shutdown if not shut down on error in
FinishDecoderSetup, a leak fixed by
http://hg.mozilla.org/releases/mozilla-aurora/annotate/57564319395c/dom/media/MediaCache.cpp#l1982

Also, if FinishDecoderSetup() does not call Shutdown() then
MediaShutdownManager will do that at shutdown, resulting in the crash reported
in bug 1171244, unless a subsequent MediaCacheStream::Init() has been
successful.  This is consistent with crash reports I've seen, where 41.0b1
crashes are under FinishDecoderSetup() but 40.0.x crashes are under
MediaShutdownManager::Shutdown().
Rather than requiring the ChannelMediaResource() to keep track of whether
MediaCacheStream::Init() has been successful, it is easier to do this in
MediaCacheStream().

I'm not sure whether it would safe to also return early on mClosed because
QueueUpdate() is not called from CloseInternal() from Update(), and so this
patch is chosen to be safe enough for beta.
Attachment #8648528 - Flags: review?(roc)
Blocks: 1171244
See Also: 1171244
https://hg.mozilla.org/mozilla-central/rev/c95b78847e3c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
The patch works fine for me, no crashes since it landed, once it always crashes when opening Gmail.
Status: RESOLVED → VERIFIED
Comment on attachment 8648528 [details] [diff] [review]
skip Close() when not initialized r?

Approval Request Comment
[Feature/regressing bug #]: bug 1116382
[User impact if declined]: Maybe always crashes for some webpage, like Gmail.
[Describe test coverage new/current, TreeHerder]: Manually verified, no crashes since it landed.
[Risks and why]: Low, just a small changes.
[String/UUID change made/needed]: No.
Attachment #8648528 - Flags: approval-mozilla-beta?
Attachment #8648528 - Flags: approval-mozilla-aurora?
Karl can you confirm the risk assessment here?
(Uplift requests should generally be written by the patch author)
Flags: needinfo?(karlt)
(In reply to David Major [:dmajor] from comment #17)
> Karl can you confirm the risk assessment here?

Yes, the patch is designed with low risk for beta.

The bug existed in the form of a shutdown crash prior to changes in 
bug 1116382, though changes there may have made crashes more frequent.

No new test coverage because I don't know how to reproduce.
Flags: needinfo?(karlt)
Comment on attachment 8648528 [details] [diff] [review]
skip Close() when not initialized r?

Simple enough fix that can be uplifted to Aurora and Beta at the same time.
Attachment #8648528 - Flags: approval-mozilla-beta?
Attachment #8648528 - Flags: approval-mozilla-beta+
Attachment #8648528 - Flags: approval-mozilla-aurora?
Attachment #8648528 - Flags: approval-mozilla-aurora+
No longer blocks: 1171244
Duplicate of this bug: 1171244
You need to log in before you can comment on or make changes to this bug.