1139874 - crash in mozilla::GStreamerReader::GetBuffered(mozilla::dom::TimeRanges*)

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Ludovic Hirlimann [:Usul]]

This bug was filed from the Socorro interface and is 
report bp-fe4c5b6c-b398-4fde-921b-ea8a22150305.
=============================================================
0 	libxul.so 	mozilla::GStreamerReader::GetBuffered(mozilla::dom::TimeRanges*) 	dom/media/gstreamer/GStreamerReader.cpp
1 	libxul.so 	mozilla::MediaDecoder::GetBuffered(mozilla::dom::TimeRanges*) 	dom/media/MediaDecoderStateMachine.h
2 	libxul.so 	mozilla::dom::HTMLMediaElement::Buffered() const 	dom/html/HTMLMediaElement.cpp
3 	libxul.so 	mozilla::dom::HTMLMediaElementBinding::get_buffered 	obj-firefox/dom/bindings/HTMLMediaElementBinding.cpp
4 	libxul.so 	mozilla::dom::GenericBindingGetter(JSContext*, unsigned int, JS::Value*) 	dom/bindings/BindingUtils.cpp
5 	libxul.so 	js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) 	js/src/jscntxtinlines.h
6 	libxul.so 	js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp
7 	libxul.so 	js::NativeGetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp
8 	libxul.so 	Interpret 	js/src/vm/NativeObject.h
9 	libxul.so 	js::RunScript(JSContext*, js::RunState&) 	js/src/vm/Interpreter.cpp
10 	libxul.so 	js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
11 	libxul.so 	js::CallOrConstructBoundFunction(JSContext*, unsigned int, JS::Value*) 	js/src/jsfun.cpp
12 	libxul.so 	js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) 	js/src/jscntxtinlines.h
13 	libxul.so 	Interpret 	js/src/vm/Interpreter.cpp
14 	libxul.so 	js::RunScript(JSContext*, js::RunState&) 	js/src/vm/Interpreter.cpp
15 	libxul.so 	js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
16 	libxul.so 	js::CallOrConstructBoundFunction(JSContext*, unsigned int, JS::Value*) 	js/src/jsfun.cpp
17 	libxul.so 	js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) 	js/src/jscntxtinlines.h
18 	libxul.so 	js::fun_call(JSContext*, unsigned int, JS::Value*) 	js/src/jsfun.cpp
19 	libxul.so 	js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) 	js/src/jscntxtinlines.h
20 	libxul.so 	Interpret 	js/src/vm/Interpreter.cpp
21 	libxul.so 	js::RunScript(JSContext*, js::RunState&) 	js/src/vm/Interpreter.cpp
22 	libxul.so 	js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
23 	libxul.so 	js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp
24 	libxul.so 	JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) 	js/src/jsapi.cpp
25 	libxul.so 	mozilla::dom::EventListener::HandleEvent(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) 	obj-firefox/dom/bindings/EventListenerBinding.cpp
26 	libxul.so 	void mozilla::dom::EventListener::HandleEvent<mozilla::dom::EventTarget*>(mozilla::dom::EventTarget* const&, mozilla::dom::Event&, mozilla::ErrorResult&, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*) 	obj-firefox/dist/include/mozilla/dom/EventListenerBinding.h
27 	libxul.so 	mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) 	dom/events/EventListenerManager.cpp
28 	libxul.so 	mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) 	dom/events/EventListenerManager.cpp
29 	libxul.so 	mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) 	dom/events/EventListenerManager.h
30 	libxul.so 	mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) 	dom/events/EventDispatcher.cpp
31 	libxul.so 	mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) 	dom/events/EventDispatcher.cpp
32 	libxul.so 	nsINode::DispatchEvent(nsIDOMEvent*, bool*) 	dom/base/nsINode.cpp
33 	libxul.so 	nsContentUtils::DispatchEvent(nsIDocument*, nsISupports*, nsAString_internal const&, bool, bool, bool, bool*, bool) 	dom/base/nsContentUtils.cpp
34 	libxul.so 	nsContentUtils::DispatchTrustedEvent(nsIDocument*, nsISupports*, nsAString_internal const&, bool, bool, bool*) 	dom/base/nsContentUtils.cpp
35 	libxul.so 	mozilla::dom::HTMLMediaElement::DispatchEvent(nsAString_internal const&) 	dom/html/HTMLMediaElement.cpp
36 	libxul.so 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp
37 	libxul.so 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/glue/nsThreadUtils.cpp
38 	libxul.so 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
39 	libxul.so 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc
40 	libxul.so 	nsBaseAppShell::Run() 	widget/nsBaseAppShell.cpp
41 	libxul.so 	XRE_RunAppShell 	toolkit/xre/nsEmbedFunctions.cpp
42 	libxul.so 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc
43 	libxul.so 	XRE_InitChildProcess 	toolkit/xre/nsEmbedFunctions.cpp
44 	libxul.so 	js::jit::CreateDerivedTypedObj(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, int) 	js/src/jit/VMFunctions.cpp
45 		@0x7f9b6f231eff 	
46 	libxul.so 	_fini

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Bug 1091008 _may_ have caused this, or perhaps the issue is something older.

Comment 2

[Boris Zbarsky [:bzbarsky]]

At a guess, mDecoder is null?

Bobby, you've been in this code recently..

Comment 3

[Bobby Holley (:bholley)]

I'm swamped, sorry. Karl, do you have the cycles to look at this?

Comment 4

[Karl Tomlinson (:karlt)]

It seems either the reader's mDecoder or the decoder's mResource is null.

The reader's mDecoder might be null if the reader has been Shutdown, but the decoder has not been disconnected from the media element, if such a situation can arise.

I don't see how decoder's mResource could be null.

This is more likely a regression from changes in bug 1095251.
I likely won't get to this today, but will chase up next week.

Comment 5

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #4)
> The reader's mDecoder might be null if the reader has been Shutdown, but the
> decoder has not been disconnected from the media element, if such a
> situation can arise.

Each of HTMLMediaElement::NetworkError() and ResetConnectionState() receive
notifications from the MediaDecoder before it calls Shutdown(), but do
nothing to disconnect from the MediaDecoder.

Similarly, I don't see any disconnection on invocation of MDecoder::Shutdown()
from NS_XPCOM_SHUTDOWN_OBSERVER_ID, though the stack trace here is not in xpcom shutdown.

Some options are:
a) a general shutdown notification from the MDecoder to its owner, at which point
   the HTMLMediaElement can clear mDecoder.
b) MediaDecoderStateMachine::GetBuffered() can check DECODER_STATE_SHUTDOWN before
   using mReader.

Comment 6

[Karl Tomlinson (:karlt)]

mReadyState is > HAVE_NOTHING on fatal network errors because the spec doesn't
say that this should be changed.

"Fatal network errors that occur after the user agent has established whether
the current media resource is usable (i.e. once the media element's readyState
attribute is no longer HAVE_NOTHING) must cause the user agent to execute the
following steps:

     1. The user agent should cancel the fetching process.

     2. Set the error attribute to a new MediaError object whose code
        attribute is set to MEDIA_ERR_NETWORK.

     3. Fire a simple event named error at the media element.

     4. Set the element's networkState attribute to the NETWORK_IDLE value.

     5. Set the element's delaying-the-load-event flag to false. This stops
        delaying the load event.

     6. Abort the overall resource selection algorithm."

Comment 7

[Karl Tomlinson (:karlt)]

Also, HTMLMediaElement uses mDecoder for GetCurrentPrincipal(), so I hesitate to clear mDecoder.

Instead, it is simple to check for shutdown before asking the reader for buffered.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=031e9978bb1e
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c99eff4fe6c8

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: check for shutdown before asking the reader for buffered

Comment 9

[Karl Tomlinson (:karlt)]

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

Possibly could make a crashtest if we can generate a network error and then get the buffered attribute.

Comment 10

[Ekanan Ketunuti]

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

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 8575048 [details] [diff] [review]
check for shutdown before asking the reader for buffered

Approval Request Comment
[Feature/regressing bug #]: bug 1095251 
[User impact if declined]: null deref
[Describe test coverage new/current, TreeHerder]: no testcase at this stage.
[Risks and why]: simple boolean check, low risk
[String/UUID change made/needed]: none

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Socorro [1] reports zero crashes after this fix landed.

[1] - https://crash-stats.mozilla.com/report/list?range_unit=days&range_value=28&signature=mozilla%3A%3AGStreamerReader%3A%3AGetBuffered%28mozilla%3A%3Adom%3A%3ATimeRanges%2A%29#tab-reports