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

Status: VERIFIED FIXED

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

Description

[(Away)]

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

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Shows 36 crashes on 42.

Comment 2

[Robert Kaiser]

It now shows up as 2.5% of 41.0b1 crashes.

Comment 3

[(Away)]

(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 (:[e-mail]) 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%.

Comment 4

[(Away)]

[Tracking Requested - why for this release]: media topcrash

Comment 5

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked as it's a top crash.

Comment 6

[(Away)]

First seen in nightly 20150606030209: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0496b5b3e9ef&tochange=4a07e1ac3cdf

Karl, possibly fallout from bug 1116382?

Comment 7

[Robert Kaiser]

(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!

Comment 8

[Jean-Yves Avenard [:jya]]

MSE doesn't use cached data ; mozilla::MediaCacheStream wouldn't be involved

Comment 9

[Karl Tomlinson (:karlt)]

(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

Comment 10

[Karl Tomlinson (: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

Comment 11

[Karl Tomlinson (:karlt)]

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().

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: skip Close() when not initialized r?

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.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c95b78847e3c

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[YF (Yang)]

The patch works fine for me, no crashes since it landed, once it always crashes when opening Gmail.

Comment 16

[YF (Yang)]

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.

Comment 17

[(Away)]

Karl can you confirm the risk assessment here?
(Uplift requests should generally be written by the patch author)

Comment 18

[Karl Tomlinson (: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.

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/be5138e9b849

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/12a8098d1945