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

VERIFIED FIXED in Firefox 41

Status

()

--
critical
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: dmajor, Assigned: karlt)

Tracking

(Blocks: 1 bug, {crash})

42 Branch
mozilla43
Unspecified
Windows NT
crash
Points:
---

Firefox Tracking Flags

(firefox41+ fixed, firefox42+ fixed, firefox43 fixed)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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: 778617

Comment 2

4 years ago
It now shows up as 2.5% of 41.0b1 crashes.
(Reporter)

Comment 3

4 years ago
(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)
(Reporter)

Comment 4

4 years ago
[Tracking Requested - why for this release]: media topcrash
status-firefox41: --- → affected
status-firefox43: --- → affected
tracking-firefox41: --- → ?
Tracked as it's a top crash.
tracking-firefox41: ? → +
tracking-firefox42: --- → +
(Reporter)

Comment 6

4 years ago
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)

Comment 7

4 years ago
(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!
(Assignee)

Updated

4 years ago
See Also: → bug 1171244
MSE doesn't use cached data ; mozilla::MediaCacheStream wouldn't be involved
Flags: needinfo?(jyavenard)
(Assignee)

Comment 9

4 years ago
(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)

Updated

4 years ago
Assignee: nobody → karlt
Status: NEW → ASSIGNED
Flags: needinfo?(karlt)
(Assignee)

Comment 10

4 years ago
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
(Assignee)

Updated

4 years ago
Component: Audio/Video → Audio/Video: Playback
(Assignee)

Comment 11

4 years ago
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().
(Assignee)

Comment 12

4 years ago
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)
(Assignee)

Updated

4 years ago
Blocks: 1171244
(Assignee)

Updated

4 years ago
See Also: bug 1171244
https://hg.mozilla.org/mozilla-central/rev/c95b78847e3c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43

Comment 15

4 years ago
The patch works fine for me, no crashes since it landed, once it always crashes when opening Gmail.
Status: RESOLVED → VERIFIED

Comment 16

4 years ago
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?
(Reporter)

Comment 17

4 years ago
Karl can you confirm the risk assessment here?
(Uplift requests should generally be written by the patch author)
Flags: needinfo?(karlt)
(Assignee)

Comment 18

4 years ago
(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+
(Assignee)

Updated

4 years ago
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.