Tyson Smith [:tsmith] Reporter
	Description • 6 years ago

This was triggered while playing a video on youtube when built with -fsanitize=vptr

Found with mozilla-central changeset: 409459:8bf380faae74

dom/html/HTMLMediaElement.cpp:4515:35: runtime error: member call on address 0x61a0001aa680 which does not point to an object of type 'mozilla::dom::HTMLVideoElement'
0x61a0001aa680: note: object is of type 'mozilla::dom::HTMLMediaElement'
 d1 04 00 76  b0 3b 09 90 41 7f 00 00  b0 40 09 90 41 7f 00 00  00 00 00 00 00 00 00 00  00 00 08 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'mozilla::dom::HTMLMediaElement'
    #0 0x7f4181621669 in mozilla::dom::HTMLMediaElement::ReportTelemetry() dom/html/HTMLMediaElement.cpp:4515:35
    #1 0x7f4181636e08 in mozilla::dom::HTMLMediaElement::SuspendOrResumeElement(bool, bool) dom/html/HTMLMediaElement.cpp:6365:7
    #2 0x7f41815f7b35 in mozilla::dom::HTMLMediaElement::NotifyOwnerDocumentActivityChanged() dom/html/HTMLMediaElement.cpp:6423:3
    #3 0x7f4181612708 in mozilla::dom::HTMLMediaElement::HTMLMediaElement(already_AddRefed<mozilla::dom::NodeInfo>&) dom/html/HTMLMediaElement.cpp:3866:3
    #4 0x7f418171c463 in mozilla::dom::HTMLVideoElement::HTMLVideoElement(already_AddRefed<mozilla::dom::NodeInfo>&) dom/html/HTMLVideoElement.cpp:48:5
    #5 0x7f418171bfc1 in NS_NewHTMLVideoElement(already_AddRefed<mozilla::dom::NodeInfo>&&, mozilla::dom::FromParser) dom/html/HTMLVideoElement.cpp:38:1
    #6 0x7f417c319f6b in nsHtml5TreeOperation::CreateHTMLElement(nsAtom*, nsHtml5HtmlAttributes*, mozilla::dom::FromParser, nsNodeInfoManager*, nsHtml5DocumentBuilder*, nsGenericHTMLElement* (*)(already_AddRefed<mozilla::dom::NodeInfo>&&, mozilla::dom::FromParser)) parser/html/nsHtml5TreeOperation.cpp:449:20
    #7 0x7f417c2ed8dd in nsHtml5TreeBuilder::createElement(int, nsAtom*, nsHtml5HtmlAttributes*, void*, nsHtml5ContentCreatorFunction) parser/html/nsHtml5TreeBuilderCppSupplement.h:100:14
    #8 0x7f417c30131f in nsHtml5TreeBuilder::appendToCurrentNodeAndPushElementMayFoster(nsHtml5ElementName*, nsHtml5HtmlAttributes*) parser/html/nsHtml5TreeBuilder.cpp:4453:11
    #9 0x7f417c2dc3dc in nsHtml5TreeBuilder::startTag(nsHtml5ElementName*, nsHtml5HtmlAttributes*, bool) parser/html/nsHtml5TreeBuilder.cpp
    #10 0x7f417c2c3af2 in nsHtml5Tokenizer::emitCurrentTagToken(bool, int) parser/html/nsHtml5Tokenizer.cpp:384:21
    #11 0x7f417c35875b in int nsHtml5Tokenizer::stateLoop<nsHtml5SilentPolicy>(int, char16_t, int, char16_t*, bool, int, int) parser/html/nsHtml5Tokenizer.cpp:1044:30
    #12 0x7f417c2ae469 in nsHtml5Tokenizer::tokenizeBuffer(nsHtml5UTF16Buffer*) parser/html/nsHtml5Tokenizer.cpp:494:11
    #13 0x7f417c2bda51 in nsHtml5StringParser::Tokenize(nsTSubstring<char16_t> const&, nsIDocument*, bool) parser/html/nsHtml5StringParser.cpp:108:33
    #14 0x7f417d641305 in nsContentUtils::ParseFragmentHTML(nsTSubstring<char16_t> const&, nsIContent*, nsAtom*, int, bool, bool, nsContentUtils::SanitizeFragments) dom/base/nsContentUtils.cpp:5228:26
    #15 0x7f417d63ffcd in nsContentUtils::CreateContextualFragment(nsINode*, nsTSubstring<char16_t> const&, bool, nsContentUtils::SanitizeFragments, mozilla::ErrorResult&) dom/base/nsContentUtils.cpp:5095:13
    #16 0x7f417d63d921 in CreateContextualFragment dom/base/nsContentUtils.h:1654:12
    #17 0x7f417d63d921 in nsContentUtils::CreateContextualFragment(nsINode*, nsTSubstring<char16_t> const&, bool, nsIDOMDocumentFragment**) dom/base/nsContentUtils.cpp:5055
    #18 0x7f417d96e0de in mozilla::dom::Element::InsertAdjacentHTML(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, mozilla::ErrorResult&) dom/base/Element.cpp:4001:12
    #19 0x7f417ff57104 in mozilla::dom::ElementBinding::insertAdjacentHTML(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitMethodCallArgs const&) objdir-ff-vptr/dom/bindings/ElementBinding.cpp:3714:9
    #20 0x7f418080b147 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) dom/bindings/BindingUtils.cpp:3031:13
    #21 0x7f4189b693cf in CallJSNative js/src/vm/JSContext-inl.h:290:15
    #22 0x7f4189b693cf in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js/src/vm/Interpreter.cpp:467
    #23 0x7f4189b4654b in CallFromStack js/src/vm/Interpreter.cpp:522:12
    #24 0x7f4189b4654b in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:3084
    #25 0x7f4189b39bbb in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:417:12
    #26 0x7f4189b692b8 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js/src/vm/Interpreter.cpp:489:15
    #27 0x7f4189b6a152 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:535:10
    #28 0x7f418a8095bb in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) js/src/jsapi.cpp:3011:12
    #29 0x7f417fdfcd52 in mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) objdir-ff-vptr/dom/bindings/EventHandlerBinding.cpp:260:37
    #30 0x7f418123c3a7 in void mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(nsISupports* const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*) objdir-ff-vptr/dist/include/mozilla/dom/EventHandlerBinding.h:363:12
    #31 0x7f418121d13b in mozilla::JSEventHandler::HandleEvent(nsIDOMEvent*) dom/events/JSEventHandler.cpp:215:12
    #32 0x7f41811cfa2e in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) dom/events/EventListenerManager.cpp:1090:51
    #33 0x7f41811d1768 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) dom/events/EventListenerManager.cpp:1259:20
    #34 0x7f41811b52b2 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) dom/events/EventDispatcher.cpp:527:16
    #35 0x7f41811ba980 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) dom/events/EventDispatcher.cpp:917:9
    #36 0x7f41811bde6e in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) dom/events/EventDispatcher.cpp:996:12
    #37 0x7f418115c269 in mozilla::DOMEventTargetHelper::DispatchEvent(nsIDOMEvent*, bool*) dom/events/DOMEventTargetHelper.cpp:269:5
    #38 0x7f418362812d in mozilla::dom::XMLHttpRequestMainThread::DispatchOrStoreEvent(mozilla::DOMEventTargetHelper*, mozilla::dom::Event*) dom/xhr/XMLHttpRequestMainThread.cpp:1336:12
    #39 0x7f418362224a in mozilla::dom::XMLHttpRequestMainThread::DispatchProgressEvent(mozilla::DOMEventTargetHelper*, mozilla::dom::XMLHttpRequestMainThread::ProgressEventType, long, long) dom/xhr/XMLHttpRequestMainThread.cpp:1299:3
    #40 0x7f41836225ef in mozilla::dom::XMLHttpRequestMainThread::DispatchProgressEvent(mozilla::DOMEventTargetHelper*, mozilla::dom::XMLHttpRequestMainThread::ProgressEventType, long, long) dom/xhr/XMLHttpRequestMainThread.cpp:1317:5
    #41 0x7f4183630684 in mozilla::dom::XMLHttpRequestMainThread::ChangeStateToDone() dom/xhr/XMLHttpRequestMainThread.cpp
    #42 0x7f418363c6e1 in mozilla::dom::XMLHttpRequestMainThread::OnStopRequest(nsIRequest*, nsISupports*, nsresult) dom/xhr/XMLHttpRequestMainThread.cpp
    #43 0x7f417a47663a in nsCORSListenerProxy::OnStopRequest(nsIRequest*, nsISupports*, nsresult) netwerk/protocol/http/nsCORSListenerProxy.cpp:651:27
    #44 0x7f4179f4777c in mozilla::net::nsHTTPCompressConv::OnStopRequest(nsIRequest*, nsISupports*, nsresult) netwerk/streamconv/converters/nsHTTPCompressConv.cpp:169:20
    #45 0x7f417a38da6b in mozilla::net::HttpChannelChild::DoOnStopRequest(nsIRequest*, nsresult, nsISupports*) netwerk/protocol/http/HttpChannelChild.cpp:1298:16
    #46 0x7f417a39d063 in mozilla::net::HttpChannelChild::OnStopRequest(nsresult const&, mozilla::net::ResourceTimingStruct const&, mozilla::net::nsHttpHeaderArray const&) netwerk/protocol/http/HttpChannelChild.cpp:1176:5
    #47 0x7f417a7259a3 in mozilla::net::ChannelEventQueue::FlushQueue() netwerk/ipc/ChannelEventQueue.cpp:93:12
    #48 0x7f417a734dd4 in MaybeFlushQueue objdir-ff-vptr/dist/include/mozilla/net/ChannelEventQueue.h:329:5
    #49 0x7f417a734dd4 in CompleteResume objdir-ff-vptr/dist/include/mozilla/net/ChannelEventQueue.h:306
    #50 0x7f417a734dd4 in mozilla::net::ChannelEventQueue::ResumeInternal()::CompleteResumeRunnable::Run() netwerk/ipc/ChannelEventQueue.cpp:161
    #51 0x7f4179111706 in mozilla::SchedulerGroup::Runnable::Run() xpcom/threads/SchedulerGroup.cpp:413:25
    #52 0x7f4179135fde in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:1096:14
    #53 0x7f4179177d3e in NS_ProcessNextEvent(nsIThread*, bool) xpcom/threads/nsThreadUtils.cpp:517:10
    #54 0x7f417aaaebe8 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:97:21
    #55 0x7f417a934a8d in RunHandler ipc/chromium/src/base/message_loop.cc:319:3
    #56 0x7f417a934a8d in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:299
    #57 0x7f4183a87856 in nsBaseAppShell::Run() widget/nsBaseAppShell.cpp:157:27
    #58 0x7f41897bf314 in XRE_RunAppShell() toolkit/xre/nsEmbedFunctions.cpp:893:22
    #59 0x7f417a934a8d in RunHandler ipc/chromium/src/base/message_loop.cc:319:3
    #60 0x7f417a934a8d in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:299
    #61 0x7f41897be457 in XRE_InitChildProcess(int, char**, XREChildData const*) toolkit/xre/nsEmbedFunctions.cpp:719:34
    #62 0x516234 in content_process_main(mozilla::Bootstrap*, int, char**) browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
    #63 0x516a50 in main browser/app/nsBrowserApp.cpp:280:18
    #64 0x7f41a04521c0 in __libc_start_main /build/glibc-itYbWN/glibc-2.26/csu/../csu/libc-start.c:308
    #65 0x41eef9 in _start (objdir-ff-vptr/dist/bin/firefox+0x41eef9)

	Andrew McCreight [:mccr8]
	Comment 1 • 6 years ago

Boris, this looks like a regression from bug 1447098. Can you take a look? Thanks.

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 6 years ago

Why do you think this is a regression from that bug?

The issue here is that the way C++ constructors work is biting us.  Specifically, the sequence of events is as follows:

1)  HTMLVideoElement constructor is called.
2)  This calls superclass HTMLMediaElement constructor.
3)  That calls HTMLMediaElement::NotifyOwnerDocumentActivityChanged.
4)  That calls HTMLMediaElement::SuspendOrResumeElement.
5)  That calls HTMLMediaElement::ReportTelemetry
6)  That does HTMLVideoElement* vid = HTMLVideoElement::FromNodeOrNull(this)

Now HTMLVideoElement::FromNode is defined in terms of the tag of the element, which comes from the nodeinfo, which was already set up by calling the superclass constructor of HTMLMediaElement.  So the "is this a <video> element" check succeeds, both before and after bug 1447098.

But all this code is running before the HTMLMediaElement constructor finished, so before any of the members of HTMLVideoElement itself, or its vtable, have been initialized.  And then it touches those members.  :( And this code has been in this form for a _long_ time.

The right answer here is the usual "don't run nontrivial code touching |this| in a C++ constructor if you have subclasses"...

Gerald, it looks like you might know something about this code, right?

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 6 years ago

Oh, and I expect this has been a problem ever since this telemetry code got added, in Firefox 50 or something...

	Andrew McCreight [:mccr8]
	Comment 4 • 6 years ago

My mistake, I just saw that that bug had touched related code.

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 6 years ago

All good.  Just wanted to know if there was something I was missing... ;)

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)
	Comment 6 • 6 years ago

Thank you Boris for the analysis.

To add to that:
After step 6, vid->GetFrameStatistics() first null-checks mDecoder (an HTMLMediaElement member, already initialized to nullptr) and bails out, so in this case no yet-uninitialized data from HTMLVideoElement is actually accessed.

I don't think there is any security risk here.
(It seems I'm not allowed to toggle 'Security-Sensitive DOM Bug', someone important will have to help with this!)

Possible solutions:
- The simplest would be to add an mDecoder null-check before HTMLVideoElement::FromNodeOrNull(this).
- More complex, but probably safer in the long run, would be to move HTMLMediaElement constructor work to another function that derived constructors must call -- much as I dislike two-step construction.
- Or NotifyOwnerDocumentActivityChanged could take a `bool aUnderConstruction` parameter and avoid operations that touch the derived object?

Jean-Yves, other comments/suggestions? I'd be happy to take this bug if you don't have the time.


As an aside, `FromNodeOrNull(this)` could just be `FromNode(this)` as `this` should never be null.

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 6 years ago

I agree that there is no problem right now given the handling of mDecoder.  Andrew, do you agree we can remove the security flag?

	Jean-Yves Avenard [:jya]
	Comment 8 • 6 years ago

:gerald, feel free to take this on... thank you

	Maire Reavy [:mreavy]
	Comment 9 • 6 years ago

Is bug 1448202 a dup of this?  Seem like it is.

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)
	Comment 10 • 6 years ago

(In reply to Maire Reavy [:mreavy] Plz needinfo? from comment #9)
> Is bug 1448202 a dup of this?  Seem like it is.

They don't crash at exactly the same spot, but yes they probably have the same cause (working with an HTMLVideoElement object while it is still in the middle of constructing its base class.)

I'll have a look at how to solve this (I previously offered before working on bug 1448494, which may be needed here), and if they can both be fixed the same way, I'll mark this one as dup. In the meantime, I'll make this one dependent on the slightly-older bug 1448202.