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

VERIFIED FIXED in Firefox 38

Status

()

--
critical
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: Usul, Assigned: karlt)

Tracking

({crash})

unspecified
mozilla39
All
Linux
crash
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox37 unaffected, firefox38 verified, firefox39 verified)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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
Component: DOM → Video/Audio
Blocks: 1091008
Bug 1091008 _may_ have caused this, or perhaps the issue is something older.
At a guess, mDecoder is null?

Bobby, you've been in this code recently..
Flags: needinfo?(bobbyholley)
I'm swamped, sorry. Karl, do you have the cycles to look at this?
Flags: needinfo?(bobbyholley) → needinfo?(karlt)
(Assignee)

Comment 4

4 years ago
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.
Assignee: nobody → karlt
Blocks: 1095251
No longer blocks: 1091008
Flags: needinfo?(karlt)
(Assignee)

Comment 5

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

Comment 6

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

Comment 7

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

Comment 8

4 years ago
Created attachment 8575048 [details] [diff] [review]
check for shutdown before asking the reader for buffered
Attachment #8575048 - Flags: review?(bobbyholley)
Attachment #8575048 - Flags: review?(bobbyholley) → review+
(Assignee)

Comment 9

4 years ago
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.
Flags: in-testsuite?
(Assignee)

Updated

4 years ago
status-firefox37: --- → unaffected
status-firefox38: --- → affected

Comment 10

4 years ago
https://hg.mozilla.org/mozilla-central/rev/c06350801711
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Assignee)

Comment 11

4 years ago
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
Attachment #8575048 - Flags: approval-mozilla-aurora?
Attachment #8575048 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Status: RESOLVED → VERIFIED
status-firefox38: fixed → verified
status-firefox39: fixed → verified
You need to log in before you can comment on or make changes to this bug.