1466991 - Assertion failure: slowNode == node (These should always be in sync!), at src/dom/base/nsINode.cpp:317

Status: VERIFIED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Reduced with m-c:
BuildID=20180604095036
SourceStamp=fa5724780fe76d6ccbbd08d978342a1db6a43d49

Note: This testcase may require a couple reloads to trigger the assertion.

Assertion failure: slowNode == node (These should always be in sync!), at src/dom/base/nsINode.cpp:317

#0 nsINode::SubtreeRoot() const src/dom/base/nsINode.cpp:313:3
#1 nsIContent::IsInAnonymousSubtree() const src/obj-firefox/dist/include/nsIContentInlines.h:189:3
#2 mozilla::DoesNotParticipateInAutoDirection(mozilla::dom::Element const*) src/dom/base/DirectionalityUtils.cpp:244:22
#3 mozilla::DoesNotAffectDirectionOfAncestors(mozilla::dom::Element const*) src/dom/base/DirectionalityUtils.cpp:255:11
#4 mozilla::SetDirOnBind(mozilla::dom::Element*, nsIContent*) src/dom/base/DirectionalityUtils.cpp:1026:13
#5 mozilla::dom::Element::BindToTree(nsIDocument*, nsIContent*, nsIContent*, bool) src/dom/base/Element.cpp:1678:5
#6 nsGenericHTMLElement::BindToTree(nsIDocument*, nsIContent*, nsIContent*, bool) src/dom/html/nsGenericHTMLElement.cpp:427:43
#7 nsINode::doInsertChildAt(nsIContent*, unsigned int, bool, nsAttrAndChildArray&) src/dom/base/nsINode.cpp:1358:14
#8 nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, mozilla::ErrorResult&) src/dom/base/nsINode.cpp:2264:14
#9 mozilla::dom::NodeBinding::appendChild(JSContext*, JS::Handle<JSObject*>, nsINode*, JSJitMethodCallArgs const&) src/obj-firefox/dom/bindings/NodeBinding.cpp:943:45
#10 bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) src/dom/bindings/BindingUtils.cpp:3285:13
#11 js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) src/js/src/vm/JSContext-inl.h:274:15
#12 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:471:16
#13 InternalCall(JSContext*, js::AnyInvokeArgs const&) src/js/src/vm/Interpreter.cpp:520:12
#14 Interpret(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:3122:18
#15 js::RunScript(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:421:12
#16 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:493:15
#17 InternalCall(JSContext*, js::AnyInvokeArgs const&) src/js/src/vm/Interpreter.cpp:520:12
#18 js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) src/js/src/vm/Interpreter.cpp:539:10
#19 JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) src/js/src/jsapi.cpp:2969:12
#20 mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) src/obj-firefox/dom/bindings/EventHandlerBinding.cpp:264:37
#21 void mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(nsISupports* const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*) src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:363:12
#22 mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event*) src/dom/events/JSEventHandler.cpp:214:12
#23 mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) src/dom/events/EventListenerManager.cpp:1124:52
#24 mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*) src/dom/events/EventListenerManager.cpp:1298:20
#25 mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:406:17
#26 mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:620:16
#27 mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) src/dom/events/EventDispatcher.cpp:1086:9
#28 nsDocumentViewer::LoadComplete(nsresult) src/layout/base/nsDocumentViewer.cpp:1166:7
#29 nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) src/docshell/base/nsDocShell.cpp:7161:21
#30 nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp:6954:7
#31 non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp
#32 nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) src/uriloader/base/nsDocLoader.cpp:1309:3
#33 nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) src/uriloader/base/nsDocLoader.cpp:852:14
#34 nsDocLoader::DocLoaderIsEmpty(bool) src/uriloader/base/nsDocLoader.cpp:741:9
#35 nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) src/uriloader/base/nsDocLoader.cpp:627:5
#36 non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) src/uriloader/base/nsDocLoader.cpp
#37 mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) src/netwerk/base/nsLoadGroup.cpp:629:28
#38 nsIDocument::DoUnblockOnload() src/dom/base/nsDocument.cpp:8343:18
#39 nsDocument::UnblockOnload(bool) src/dom/base/nsDocument.cpp:8265:9
#40 nsIDocument::DispatchContentLoadedEvents() src/dom/base/nsDocument.cpp:5223:3
#41 mozilla::detail::RunnableMethodImpl<nsIDocument*, void (nsIDocument::*)(), true, (mozilla::RunnableKind)0>::Run() src/obj-firefox/dist/include/nsThreadUtils.h:1216:13
#42 mozilla::SchedulerGroup::Runnable::Run() src/xpcom/threads/SchedulerGroup.cpp:337:32
#43 nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1088:14
#44 NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:519:10
#45 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
#46 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:325:10
#47 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298:3
#48 nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:157:27
#49 XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:893:22
#50 mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:269:9
#51 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:325:10
#52 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298:3
#53 XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:719:34
#54 content_process_main(mozilla::Bootstrap*, int, char**) src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
#55 main src/browser/app/nsBrowserApp.cpp:282:18
#56 __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
#57 _start (firefox+0x423544)

Comment 1

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

I wonder if this warns in older builds. There used to be NS_ASSERTION.

Comment 2

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

oh, the assertion fires because we have some debug code using SubtreeRoot.

Comment 3

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

Yeah, this is on beta
###!!! ASSERTION: Must have binding parent when in native anonymous subtree which is in document.
Native anonymous subtree which is not in document must have native anonymous root.: '!IsInNativeAnonymousSubtree() || GetBindingParent() || (!IsInUncomposedDoc() && static_cast<nsIContent*>(SubtreeRoot())->IsInNativeAnonymousSubtree())', file /home/smaug/mozilla/hg/mozilla-beta/ff_build/dist/include/nsIContentInlines.h, line 191

Comment 4

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

Attachment: testcase showing NAC access

Comment 5

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

The issue is that element is created in NAC-XBL by JS, so it is wrapped before it is bound to NAC.

I'm pondering if we should just force one to create all the NAC elements in this kind of case by placing them under <content>

Comment 6

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

videocontrols and type=time/date at least seem to create elements in NAC-XBL.
bz suggested reparenting nodes when adding them to NAC tree.

Comment 7

[Daniel Veditz [:dveditz]]

What are the security implications here? Can you suggest a rating?

Comment 8

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

sec-crit. We let web pages to touch native anonymous content.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Factor out ShouldUseXBLScope

We're going to add another consumer in the next changeset.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Reparent nodes when they start being in the XBL scope

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8984642 [details] [diff] [review]
part 2.  Reparent nodes when they start being in the XBL scope

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not easily at all.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No.

Which older supported branches are affected by this flaw? Probably all of them.

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

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

How likely is this patch to cause regressions; how much testing does it need?  I'd say medium risk.  I'd definitely want several weeks of testing before ship.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

We're making RC builds for 61 next week. If we want several weeks of testing before ship, we need to not take this on 61. Do you think it makes sense to take this on trunk (62) and not backport it until later in the next cycle, Boris?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Yes.  I would not be terribly comfortable backporting this to 61 at this point.

For that matter, I would probably be OK fixing this on 62 and ESR and not touching 61 at all...

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 8984642 [details] [diff] [review]
part 2.  Reparent nodes when they start being in the XBL scope

sec-approval=dveditz for nightly (62). Not going to make 61.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8984642 [details] [diff] [review]
part 2.  Reparent nodes when they start being in the XBL scope

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-crit
User impact if declined: Web pages might be able to run arbitrary code...
Fix Landed on Version: 62
Risk to taking this patch (and alternatives if risky): Medium risk, but this was
   the safest option we came up with.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 16

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c8ca5323b16bf17c39b783d6e50b1748e8c8697
https://hg.mozilla.org/integration/mozilla-inbound/rev/419f6555e669c1505e9c216df1b0c2ca809bcfeb

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 backported to esr60

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9c8ca5323b16
https://hg.mozilla.org/mozilla-central/rev/419f6555e669

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8984641 [details] [diff] [review]
part 1.  Factor out ShouldUseXBLScope

Fixes a sec-crit. Approved for ESR 60.2.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/078985dd8656
https://hg.mozilla.org/releases/mozilla-esr60/rev/0915c8e08aa9

Comment 21

[Cristian Fogel [:cfogel]]

Hey Boris, could you post some STR for manual testing?
Thank you!

Comment 22

[Boris Zbarsky [:bzbarsky]]

You just load the attached testcase in a debug build.

Comment 23

[Cristian Fogel [:cfogel]]

Managed to get the crash on a build from 20180604095036.
Verified on Ubuntu 18.04 for 20180827144429 for RC and 20180828172101 ESR builds and can confirm the issue is no longer present.
Tested on the latest ESR/RC builds over win-10 and macOS10.13 and did not encounter any issues.