1516289 - Assertion failure: xpc::IsInContentXBLScope(obj) || !xpc::UseContentXBLScope(JS::GetObjectRealmOrNull(obj)), at src/dom/base/nsINode.cpp:2664 due to Event.composedPath failing to hide a closed shadow root.

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Assertion failure: xpc::IsInContentXBLScope(obj) || !xpc::UseContentXBLScope(JS::GetObjectRealmOrNull(obj)), at src/dom/base/nsINode.cpp:2664

#0 nsINode::WrapObject(JSContext*, JS::Handle<JSObject*>) src/dom/base/nsINode.cpp:2662:3
#1 mozilla::dom::Element::WrapObject(JSContext*, JS::Handle<JSObject*>) src/dom/base/Element.cpp:550:43
#2 bool mozilla::dom::binding_detail::DoGetOrCreateDOMReflector<mozilla::dom::EventTarget, (mozilla::dom::binding_detail::GetOrCreateReflectorWrapBehavior)0>(JSContext*, mozilla::dom::EventTarget*, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) src/obj-firefox/dist/include/mozilla/dom/BindingUtils.h:968:18
#3 mozilla::dom::Event_Binding::composedPath(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Event*, JSJitMethodCallArgs const&) src/obj-firefox/dom/bindings/EventBinding.cpp:391:14
#4 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:3062:13
#5 CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) src/js/src/vm/Interpreter.cpp:443:13
#6 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:535:12
#7 InternalCall(JSContext*, js::AnyInvokeArgs const&) src/js/src/vm/Interpreter.cpp:590:10
#8 Interpret(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:3320:16
#9 js::RunScript(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:423:10
#10 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:563:13
#11 InternalCall(JSContext*, js::AnyInvokeArgs const&) src/js/src/vm/Interpreter.cpp:590:10
#12 js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) src/js/src/vm/Interpreter.cpp:606:8
#13 JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) src/js/src/jsapi.cpp:2649:10
#14 mozilla::dom::EventListener::HandleEvent(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) src/obj-firefox/dom/bindings/EventListenerBinding.cpp:52:8
#15 void mozilla::dom::EventListener::HandleEvent<mozilla::dom::EventTarget*>(mozilla::dom::EventTarget* const&, mozilla::dom::Event&, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*) src/obj-firefox/dist/include/mozilla/dom/EventListenerBinding.h:66:12
#16 mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) src/dom/events/EventListenerManager.cpp:1039:43
#17 mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) src/dom/events/EventListenerManager.cpp:1238:17
#18 mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:350:17
#19 mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:588:14
#20 mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) src/dom/events/EventDispatcher.cpp:1042:11
#21 mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsPresContext*, nsEventStatus*) src/dom/events/EventDispatcher.cpp
#22 nsINode::DispatchEvent(mozilla::dom::Event&, mozilla::dom::CallerType, mozilla::ErrorResult&) src/dom/base/nsINode.cpp:1029:17
#23 mozilla::dom::EventTarget::DispatchEvent(mozilla::dom::Event&) src/dom/events/EventTarget.cpp:176:13
#24 mozilla::AsyncEventDispatcher::Run() src/dom/events/AsyncEventDispatcher.cpp:69:12
#25 nsContentUtils::RemoveScriptBlocker() src/dom/base/nsContentUtils.cpp:5196:15
#26 nsDocument::EndUpdate() src/dom/base/nsDocument.cpp:4642:3
#27 nsHTMLDocument::EndUpdate() src/dom/html/nsHTMLDocument.cpp:2025:15
#28 mozAutoDocUpdate::~mozAutoDocUpdate() src/dom/base/mozAutoDocUpdate.h:34:18
#29 nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, mozilla::ErrorResult&) src/dom/base/nsINode.cpp:2404:1
#30 mozilla::dom::Element::InsertAdjacent(nsTSubstring<char16_t> const&, nsINode*, mozilla::ErrorResult&) src/dom/base/Element.cpp:3715:34
#31 mozilla::dom::Element::InsertAdjacentElement(nsTSubstring<char16_t> const&, mozilla::dom::Element&, mozilla::ErrorResult&) src/dom/base/Element.cpp:3736:22
#32 mozilla::dom::Element_Binding::insertAdjacentElement(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitMethodCallArgs const&) src/obj-firefox/dom/bindings/ElementBinding.cpp:1985:59
#33 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:3062:13
#34 CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) src/js/src/vm/Interpreter.cpp:443:13
#35 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:535:12
#36 InternalCall(JSContext*, js::AnyInvokeArgs const&) src/js/src/vm/Interpreter.cpp:590:10
#37 Interpret(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:3320:16
#38 js::RunScript(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:423:10
#39 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:563:13
#40 InternalCall(JSContext*, js::AnyInvokeArgs const&) src/js/src/vm/Interpreter.cpp:590:10
#41 js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) src/js/src/vm/Interpreter.cpp:606:8
#42 JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) src/js/src/jsapi.cpp:2649:10
#43 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:265:37
#44 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
#45 mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event*) src/dom/events/JSEventHandler.cpp:205:12
#46 mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) src/dom/events/EventListenerManager.cpp:1043:51
#47 mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) src/dom/events/EventListenerManager.cpp:1238:17
#48 mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:350:17
#49 mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:552:16
#50 mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) src/dom/events/EventDispatcher.cpp:1042:11
#51 mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsPresContext*, nsEventStatus*) src/dom/events/EventDispatcher.cpp
#52 nsINode::DispatchEvent(mozilla::dom::Event&, mozilla::dom::CallerType, mozilla::ErrorResult&) src/dom/base/nsINode.cpp:1029:17
#53 mozilla::dom::EventTarget::DispatchEvent(mozilla::dom::Event&) src/dom/events/EventTarget.cpp:176:13
#54 mozilla::AsyncEventDispatcher::Run() src/dom/events/AsyncEventDispatcher.cpp:69:12
#55 mozilla::SchedulerGroup::Runnable::Run() src/xpcom/threads/SchedulerGroup.cpp:299:32
#56 nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1157:14
#57 NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:468:10
#58 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:88:21
#59 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:314:10
#60 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:289:3
#61 nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:137:27
#62 XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:915:20
#63 mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:238:9
#64 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:314:10
#65 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:289:3
#66 XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:753:34
#67 content_process_main(mozilla::Bootstrap*, int, char**) src/browser/app/../../ipc/contentproc/plugin-container.cpp:49:28
#68 main src/browser/app/nsBrowserApp.cpp:265:18
#69 __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
#70 _start (firefox+0x349f4)

Comment 1

[Alex Gaynor [:Alex_Gaynor]]

Emilio, you fixed a similar assert a while back, can you take a look?

Also, if this assert is security sensitive, is it possible to make it a release assert?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #1)
> Emilio, you fixed a similar assert a while back, can you take a look?

Sure.

I couldn't repro this on a recent-ish debug build (https://hg.mozilla.org/mozilla-unified/rev/af22225148f7cb57718a9b3f4428d57ba93f6842).

Also, it doesn't look like the kind of test-case that would trigger this assertion (this assertion means that we're exposing a chrome-only node to content, which is not good, but in this test-case no content script is running)!

> Also, if this assert is security sensitive, is it possible to make it a
> release assert?

Probably not, this is a very very hot code-path.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Tyson, this looks like instead it crashes WR with MOZ_RELEASE_ASSERT(!aRect.IsEmpty()).

That sounds like a much more plausible assertion given the test-case! Should this test-case be in another bug, and another test-case be here?

Comment 4

[Tyson Smith [:tsmith]]

Attachment: testcase.html

So many WR testcases lately :P

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Aye, that testcase looks much funnier :)

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Testcase that shows the underlying bug.

This test-case doesn't have a marquee, and thus doesn't trigger this assertion.

This is a bug in the implementation of Event.composedPath. It should be hiding closed shadow roots, but it's not doing that in this case.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(Didn't mean to drop the ni?, will take a look)

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Well, I implemented composedPath() to the letter and it took me a bit to figure out why all the tests were failing: https://github.com/whatwg/dom/pull/727

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1516289 - Fix composedPath implementation when slots are present. r=smaug

Comment 10

[Emilio Cobos Álvarez (:emilio)]

I should just land this patch as part as bug 1510204.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9033657 [details]
Bug 1516289 - Fix composedPath implementation when slots are present. r=smaug

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: hard. Indeed I could just land the patch in bug 1510204 and it'd look completely innocent.

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?: 64+

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

Do you have backports for the affected branches?: No

If not, how different, hard to create, and risky will they be?: Shouldn't be hard. But overall I don't think this is a sec-high. We're exposing some DOM nodes incorrectly, but we still don't allow you to touch them. So I think being consistent with bug 1505887 this would be a sec-moderate at most.

How likely is this patch to cause regressions; how much testing does it need?: Not likely, this is fixing a recently-introduced web API, and it's well tested.

Comment 12

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

Comment on attachment 9033657 [details]
Bug 1516289 - Fix composedPath implementation when slots are present. r=smaug

Making this a sec-moderate after discussing this with Tyson as well. Clearing the sec-approval flag as it is no longer necessary. Please go ahead and check this into mozilla-central.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Fixed by bug 1516289.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9033657 [details]
Bug 1516289 - Fix composedPath implementation when slots are present. r=smaug

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Shadow DOM

User impact if declined: Bogus implementation of web-exposed APIs which can expose internal nodes.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): It just modifies a well-tested API (we were failing pre-existing web-platform-tests), and fixing this bug. The patch is not totally trivial, but it's not risky I'd say.

String changes made/needed: none

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9033657 [details]
Bug 1516289 - Fix composedPath implementation when slots are present. r=smaug

[Triage Comment]
Fixes a spec compliance issue with Shadow DOM with possible security implications. Covered by automated tests. Approved for 65.0b9.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c7cda6be6a94