1400599 - Assertion failure: this != presContext->GetViewportScrollbarStylesOverrideElement in [@ mozilla::dom::Element::UnbindFromTree]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Assertion failure: this != presContext->GetViewportScrollbarStylesOverrideElement() (Leaving behind a raw pointer to this element (as having propagated scrollbar styles) - that's dangerous...), at /src/dom/base/Element.cpp:1878

#0 0x7efd3eec0a65 in mozilla::dom::Element::UnbindFromTree(bool, bool) /src/dom/base/Element.cpp:1908:7
#1 0x7efd40e3297a in nsGenericHTMLElement::UnbindFromTree(bool, bool) /src/dom/html/nsGenericHTMLElement.cpp:537:20
#2 0x7efd3eec0834 in mozilla::dom::Element::UnbindFromTree(bool, bool) /src/dom/base/Element.cpp:1992:37
#3 0x7efd40e3297a in nsGenericHTMLElement::UnbindFromTree(bool, bool) /src/dom/html/nsGenericHTMLElement.cpp:537:20
#4 0x7efd40de6c09 in mozilla::dom::HTMLSharedElement::UnbindFromTree(bool, bool) /src/dom/html/HTMLSharedElement.cpp:288:25
#5 0x7efd3f115cf8 in nsINode::doRemoveChildAt(unsigned int, bool, nsIContent*, nsAttrAndChildArray&) /src/dom/base/nsINode.cpp:1945:9
#6 0x7efd3f052452 in nsDocument::RemoveChildAt(unsigned int, bool) /src/dom/base/nsDocument.cpp:4429:3
#7 0x7efd3f117147 in nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, mozilla::ErrorResult&) /src/dom/base/nsINode.cpp:2432:5
#8 0x7efd3f692a30 in mozilla::dom::NodeBinding::replaceChild(JSContext*, JS::Handle<JSObject*>, nsINode*, JSJitMethodCallArgs const&) /src/obj-firefox/dom/bindings/NodeBinding.cpp:955:45
#9 0x7efd4083f7be in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /src/dom/bindings/BindingUtils.cpp:3055:13
#10 0x7efd4580f3f1 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) /src/js/src/jscntxtinlines.h:293:15
#11 0x7efd4580ef9d in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /src/js/src/vm/Interpreter.cpp:495:16
#12 0x7efd4580fe35 in InternalCall(JSContext*, js::AnyInvokeArgs const&) /src/js/src/vm/Interpreter.cpp:540:12
#13 0x7efd45804895 in Interpret(JSContext*, js::RunState&) /src/js/src/vm/Interpreter.cpp:3084:18
#14 0x7efd457f04f1 in js::RunScript(JSContext*, js::RunState&) /src/js/src/vm/Interpreter.cpp:435:12
#15 0x7efd4580ef20 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /src/js/src/vm/Interpreter.cpp:513:15
#16 0x7efd4580fe35 in InternalCall(JSContext*, js::AnyInvokeArgs const&) /src/js/src/vm/Interpreter.cpp:540:12
#17 0x7efd4581004c in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /src/js/src/vm/Interpreter.cpp:559:10
#18 0x7efd460c580b in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /src/js/src/jsapi.cpp:2965:12
#19 0x7efd40326519 in mozilla::dom::EventListener::HandleEvent(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) /src/obj-firefox/dom/bindings/EventListenerBinding.cpp:47:8
#20 0x7efd40b68a31 in void mozilla::dom::EventListener::HandleEvent<mozilla::dom::EventTarget*>(mozilla::dom::EventTarget* const&, mozilla::dom::Event&, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*) /src/obj-firefox/dist/include/mozilla/dom/EventListenerBinding.h:65:12
#21 0x7efd40b685b6 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) /src/dom/events/EventListenerManager.cpp:1109:9
#22 0x7efd40b69915 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) /src/dom/events/EventListenerManager.cpp:1283:20
#23 0x7efd40b5622b in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) /src/dom/events/EventDispatcher.cpp:313:17
#24 0x7efd40b558ef in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /src/dom/events/EventDispatcher.cpp:462:16
#25 0x7efd40b573cd in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) /src/dom/events/EventDispatcher.cpp:822:9
#26 0x7efd40b38330 in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) /src/dom/events/EventDispatcher.cpp:888:12
#27 0x7efd3f11343e in nsINode::DispatchEvent(nsIDOMEvent*, bool*) /src/dom/base/nsINode.cpp:1341:5
#28 0x7efd3ed36fc4 in nsContentUtils::DispatchEvent(nsIDocument*, nsISupports*, nsTSubstring<char16_t> const&, bool, bool, bool, bool*, bool) /src/dom/base/nsContentUtils.cpp:4514:18
#29 0x7efd3ed36d6b in nsContentUtils::DispatchTrustedEvent(nsIDocument*, nsISupports*, nsTSubstring<char16_t> const&, bool, bool, bool*) /src/dom/base/nsContentUtils.cpp:4482:10
#30 0x7efd3f05c423 in nsDocument::DispatchContentLoadedEvents() /src/dom/base/nsDocument.cpp:5505:3
#31 0x7efd3f0d52b5 in mozilla::detail::RunnableMethodImpl<nsDocument*, void (nsDocument::*)(), true, (mozilla::RunnableKind)0>::Run() /src/obj-firefox/dist/include/nsThreadUtils.h:1192:13
#32 0x7efd3cbe620f in nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1039:14
#33 0x7efd3cbeb580 in NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:521:10
#34 0x7efd3d75c2e5 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:97:21
#35 0x7efd3d6b1227 in MessageLoop::RunInternal() /src/ipc/chromium/src/base/message_loop.cc:326:10
#36 0x7efd3d6b10b9 in MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299:3
#37 0x7efd4206d3fa in nsBaseAppShell::Run() /src/widget/nsBaseAppShell.cpp:158:27
#38 0x7efd45288b91 in nsAppStartup::Run() /src/toolkit/components/startup/nsAppStartup.cpp:288:30
#39 0x7efd453ed021 in XREMain::XRE_mainRun() /src/toolkit/xre/nsAppRunner.cpp:4701:22
#40 0x7efd453eecea in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4865:8
#41 0x7efd453efbd8 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4960:21
#42 0x4ed398 in do_main(int, char**, char**) /src/browser/app/nsBrowserApp.cpp:236:22
#43 0x4eccb0 in main /src/browser/app/nsBrowserApp.cpp:309:16
#44 0x7efd5bd3a82f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
#45 0x41e9e4 in _start (/home/user/workspace/browsers/m-c-1505582644-asan-debug/firefox+0x41e9e4)

Comment 1

[Andrew McCreight [:mccr8]]

Any ideas, Boris? It looks like you touched the code at the top of this stack recently, in bug 1398500. Thanks.

Comment 2

[Daniel Holbert [:dholbert]]

[Tracking Requested - why for this release]:
Recent regression with security implications.

(The assertion failure indicates we're leaving behind a dangling pointer to a soon-to-be-deleted object, so it's a bit scary.  Fortunately, I think the dangling pointer will only be used for equality comparisons -- it probably won't be dereferenced -- so it's not *that* dangerous at this point.  But, still worth worrying about; at best, this is a logic bug which will break scrollbars in some cases, and at worst it may become a UAF if we start doing more with this pointer in the future.)

Comment 3

[Boris Zbarsky [:bzbarsky]]

OK, so this is in fact a regression from bug 1398500.

The problem is that we're doing a ContentRemoved on an _ancestor_ of the scrollbar override element.  And I didn't consider that case in the patches there, because the old code seemed to be checking for "could be a scrollbar override element".  Fix is simple and is coming up.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Make sure to properly handle removal of the root when the body propagates scrollbar styles to the viewport

MozReview-Commit-ID: FixsFFtNOfQ

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8909467 [details] [diff] [review]
Make sure to properly handle removal of the root when the body propagates scrollbar styles to the viewport

[Security approval request comment]
How easily could an exploit be constructed based on the patch? No known way at this time.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  Yes.  Of course, so does the patch itself: it obviously has to do with removals of the root.

Which older supported branches are affected by this flaw?  Beta 57.

If not all supported branches, which bug introduced the flaw? Bug 1398500.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  This should apply directly to beta 57.

How likely is this patch to cause regressions; how much testing does it need?  Not very and not much.  We should just make sure we land it this week before the next beta merge...

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8909467 [details] [diff] [review]
Make sure to properly handle removal of the root when the body propagates scrollbar styles to the viewport

Review of attachment 8909467 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, with possible comment tweak:

::: layout/base/nsCSSFrameConstructor.cpp
@@ +8616,5 @@
>    MOZ_ASSERT(presContext, "Our presShell should have a valid presContext");
>  
> +  // We want to detect when the viewport override element is in the subtree
> +  // being removed.  The override element is always either the root element or a
> +  // <body> child of the root element.  So we can only be removing the override

The second sentence here (RE the override *always* being either the root or a <body> child) isn't quite correct, I think.   The override element could also be some arbitrary fullscreened element (which is an arbitrarily-far-away descendant).

Fortunately that's not a case we have to consider here, because (as noted below) we handle that in Element::UnbindFromTree.  But we don't want the comment to make it sound like that scenario is impossible.

Maybe just adjust the beginning of that sentence to: "When we get here, the override element must be either [...]"?  That softens it a bit so as not to rule out the possibility of fullscreen-elems providing override scrollbars elsewhere.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> The override element could also be some arbitrary fullscreened element 

The member in the prescontext could not, which is what we really care about...  I'll adjust the comments to make that clearer.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8909467 [details] [diff] [review]
Make sure to properly handle removal of the root when the body propagates scrollbar styles to the viewport

This is trunk-only, so sec-approval isn't needed for landing.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

(And the second merge from m-c to m-b is on Thursday, so it should have no problem getting over to Beta in time if it lands today)

Comment 11

[Boris Zbarsky [:bzbarsky]]

Updated comment: 

  // We want to detect when the viewport override element stored in the
  // prescontext is in the subtree being removed.  Except in fullscreen cases
  // (which are handled in Element::UnbindFromTree and do not get stored on the
  // prescontext), the override element is always either the root element or a
  // <body> child of the root element.  So we can only be removing the stored
  // override element if the thing being removed is either the override element
  // itself or the root element (which can be a parent of the override element).

Comment 12

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/13f651129c38f79e0b7efa67892dcb296e41fe1a

Comment 13

[Daniel Holbert [:dholbert]]

Thanks!  The comment-update is quite helpful.  (I'd forgotten that fullscreen elements don't get stored on the prescontext.)

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/13f651129c38