1460740 - Assertion failure: !aRoot || aNotInsertedYet || (nsContentUtils::ContentIsDescendantOf(aStart.Container(), aRoot) && ... (Wrong root), at src/dom/base/nsRange.cpp:976

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect, P2)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Log from m-c:
BuildID=20180509093426
SourceStamp=9294f67b3f3bd4a3dd898961148cecd8bfc1ce9c

Assertion failure: !aRoot || aNotInsertedYet || (nsContentUtils::ContentIsDescendantOf(aStart.Container(), aRoot) && nsContentUtils::ContentIsDescendantOf(aEnd.Container(), aRoot) && aRoot == IsValidBoundary(aStart.Container()) && aRoot == IsValidBoundary(aEnd.Container())) (Wrong root), at src/dom/base/nsRange.cpp:976

#0 nsRange::DoSetRange(mozilla::RangeBoundaryBase<nsINode*, nsIContent*> const&, mozilla::RangeBoundaryBase<nsINode*, nsIContent*> const&, nsINode*, bool) src/dom/base/nsRange.cpp:1045:37
#1 nsRange::CloneRange() const src/dom/base/nsRange.cpp:2615:10
#2 mozilla::dom::Selection::Extend(nsINode&, unsigned int, mozilla::ErrorResult&) src/dom/base/Selection.cpp:2675:46
#3 mozilla::dom::Selection::ExtendJS(nsINode&, unsigned int, mozilla::ErrorResult&) src/dom/base/Selection.cpp:2621:3
#4 mozilla::dom::SelectionBinding::extend(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Selection*, JSJitMethodCallArgs const&) src/obj-firefox/dom/bindings/SelectionBinding.cpp:673:9
#5 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:3260:13
#6 js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) src/js/src/vm/JSContext-inl.h:280:15
#7 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:467:16
#8 InternalCall(JSContext*, js::AnyInvokeArgs const&) src/js/src/vm/Interpreter.cpp:516:12
#9 Interpret(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:3086:18
#10 js::RunScript(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:417:12
#11 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:489:15
#12 InternalCall(JSContext*, js::AnyInvokeArgs const&) src/js/src/vm/Interpreter.cpp:516:12
#13 js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) src/js/src/vm/Interpreter.cpp:535:10
#14 JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) src/js/src/jsapi.cpp:2989:12
#15 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
#16 void mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(nsISupports* const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*) src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:363:12
#17 mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event*) src/dom/events/JSEventHandler.cpp:214:12
#18 mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) src/dom/events/EventListenerManager.cpp:1121:52
#19 mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*) src/dom/events/EventListenerManager.cpp:1288:20
#20 mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:348:17
#21 mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:528:16
#22 mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) src/dom/events/EventDispatcher.cpp:961:9
#23 mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsPresContext*, nsEventStatus*) src/dom/events/EventDispatcher.cpp
#24 nsINode::DispatchEvent(mozilla::dom::Event&, mozilla::dom::CallerType, mozilla::ErrorResult&) src/dom/base/nsINode.cpp:1079:5
#25 mozilla::dom::EventTarget::DispatchEvent(mozilla::dom::Event&) src/dom/events/EventTarget.cpp:204:13
#26 mozilla::AsyncEventDispatcher::Run() src/dom/events/AsyncEventDispatcher.cpp:68:12
#27 mozilla::SchedulerGroup::Runnable::Run() src/xpcom/threads/SchedulerGroup.cpp:337:32
#28 nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1090:14
#29 NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:519:10
#30 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
#31 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:326:10
#32 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:299:3
#33 nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:157:27
#34 XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:893:22
#35 mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:269:9
#36 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:326:10
#37 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:299:3
#38 XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:719:34
#39 content_process_main(mozilla::Bootstrap*, int, char**) src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
#40 main src/browser/app/nsBrowserApp.cpp:282:18
#41 __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
#42 _start (firefox+0x423444)

Comment 1

[Kagami Rosylight [:saschanaz] (they/them)]

Somehow I couldn't repro bug 1617278 but can repro this one. Continuing from https://bugzilla.mozilla.org/show_bug.cgi?id=1617278#c14:

I think this is more about <details> itself. Selection::mAnchorFocusRange incorrectly includes text node from disconnected internal <summary> element, which is why the assertion fails. It seems the disconnection happens without correctly updating selection range. FlushPendingNotifications does not help this time.

Emilio, do you have a quick idea about selection update in tree change?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Not off the top of my head... How does selection usually get updated when an element is unbound from the tree? The anonymous summary should be unbound when being removed from the frame tree via nsIFrame::DestroyAnonymousContent.

Comment 3

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: bug1460740.html

A smaller repro. (Click the element.)

Comment 4

[Kagami Rosylight [:saschanaz] (they/them)]

While it seems nsRange::ContentRemoved can do the work, why does <details> need to destruct and create new <summary> every time it gets a click? Could we just cache it?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Well that would solve the click case but not the display: none case. Bug 1308080 would fix both I guess, but it seems there's a bunch of other elements that would have the same issue.

Comment 6

[Kagami Rosylight [:saschanaz] (they/them)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)

Well that would solve the click case but not the display: none case. Bug 1308080 would fix both I guess, but it seems there's a bunch of other elements that would have the same issue.

Do you have an example where anonymous contents are selectable?

BTW I found nsINode::IsMaybeSelected returns false for internal summary element even when it is selectable, is this expected or should I consider it as a bug?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(In reply to Kagami :saschanaz from comment #6)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)

Well that would solve the click case but not the display: none case. Bug 1308080 would fix both I guess, but it seems there's a bunch of other elements that would have the same issue.

Do you have an example where anonymous contents are selectable

<input type=text>? Though those have their own selection so maybe not a concern.

Other than that i don't know, we don't have that many nsIAnonymousContentCreator implementations.

BTW I found nsINode::IsMaybeSelected returns false for internal summary element even when it is selectable, is this expected or should I consider it as a bug?

Not sure, in general the interaction of selection with shadow dom / anon content is pretty messy... I suspect we clear those flags on the dom and not properly clear them for anonymous kids.

Maybe we should just make the anonymous content not selectable for now, if we believe <details> is the only element hitting this issue. So something like details > summary:-moz-native-anonymous { user-select: none } around here.

Comment 8

[Kagami Rosylight [:saschanaz] (they/them)]

It seems user-select: none is the way to go for now and ultimately shadow DOM. It already has a weird selection behavior, the anonymous summary can be selected but not together with other text node, virtually making it unselectable.

Comment 9

[Kagami Rosylight [:saschanaz] (they/them)]

Just for the record, this was my intended patch:

void nsIFrame::DestroyAnonymousContent(
    nsPresContext* aPresContext, already_AddRefed<nsIContent>&& aContent) {
  if (nsCOMPtr<nsIContent> content = aContent) {
    // XXX(krosylight): Why can't we use content->IsMaybeSelected()?
    // if (content->IsAnyOfHTMLElements(nsGkAtoms::summary)) {
    //   if (Selection* selection = aPresContext->PresShell()->GetSelection(nsISelectionController::SELECTION_NORMAL)) {
    //     for (uint32_t i = 0; i < selection->RangeCount(); i++) {
    //       selection->GetRangeAt(i)->ContentRemoved(content, content->GetPreviousSibling());
    //     }
    //   }
    // }

    aPresContext->EventStateManager()->NativeAnonymousContentRemoved(content);
    aPresContext->PresShell()->NativeAnonymousContentRemoved(content);
    content->UnbindFromTree();
  }
}

Comment 10

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1460740 - Disable selection within anonymous summary element r=emilio

It has already been impossible to select it together with other nodes.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e3f87c81e43e
Disable selection within anonymous summary element r=emilio

Comment 12

[Natalia Csoregi [:nataliaCs]]

Backed out for causing selection wpt failures

backout: https://hg.mozilla.org/integration/autoland/rev/0a11625a91651e6507d8b3e4de1e395d77179f88

push where it appeared: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=6b9978d2a957cb6ec9d6bd0e0155d16db887a186&searchStr=wpt&test_paths=%2Fselection

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=e3f87c81e43ec66adab90e8d89b5928607e124eb

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=319147433&repo=autoland&lineNumber=8735

[task 2020-10-20T16:13:41.819Z] 16:13:41 INFO - TEST-START | /selection/select-internal.html
[task 2020-10-20T16:13:41.819Z] 16:13:41 INFO - Closing window 108
[task 2020-10-20T16:13:42.315Z] 16:13:42 INFO - {'actions': [{u'type': u'none', u'id': u'0', u'actions': [{u'duration': 16, u'type': u'pause'}, {u'duration': 16, u'type': u'pause'}, {u'duration': 16, u'type': u'pause'}, {u'duration': 16, u'type': u'pause'}]}, {u'type': u'pointer', u'actions': [{u'y': 5, u'x': 5, u'type': u'pointerMove', u'origin': {'element-6066-11e4-a52e-4f735466cecf': u'9468cd2f-838f-42c2-849a-3a3a741cf9d8', 'chromeelement-9fc5-4b51-a3c8-01716eedeb04': u'9468cd2f-838f-42c2-849a-3a3a741cf9d8'}}, {u'button': 0, u'type': u'pointerDown'}, {u'y': 50, u'x': 50, u'type': u'pointerMove', u'origin': u'viewport'}, {u'button': 0, u'type': u'pointerUp'}], u'parameters': {u'pointerType': u'mouse'}, u'id': u'1'}]}
[task 2020-10-20T16:13:42.371Z] 16:13:42 INFO - PID 4015 | [Parent 4015, Main Thread] WARNING: NS_ENSURE_TRUE(rootFrame) failed: file /builds/worker/checkouts/gecko/dom/base/nsGlobalWindowOuter.cpp:4284
[task 2020-10-20T16:13:42.378Z] 16:13:42 INFO - PID 4015 | console.warn: SearchSettings: "get: No settings file exists, new profile?" (new Error("", "(unknown module)"))
[task 2020-10-20T16:13:42.455Z] 16:13:42 INFO - PID 1194 | [Child 3964, Main Thread] WARNING: NS_ENSURE_TRUE(info) failed: file /builds/worker/checkouts/gecko/extensions/permissions/PermissionDelegateHandler.cpp:348
[task 2020-10-20T16:13:42.455Z] 16:13:42 INFO - PID 1194 | [Child 1469, Main Thread] WARNING: NS_ENSURE_TRUE(info) failed: file /builds/worker/checkouts/gecko/extensions/permissions/PermissionDelegateHandler.cpp:348
[task 2020-10-20T16:13:42.515Z] 16:13:42 INFO - PID 4015 | [Parent 4015, Main Thread] WARNING: Failed to retarget HTML data delivery to the parser thread.: file /builds/worker/checkouts/gecko/parser/html/nsHtml5StreamParser.cpp:1132
[task 2020-10-20T16:13:42.591Z] 16:13:42 INFO -
[task 2020-10-20T16:13:42.591Z] 16:13:42 INFO - TEST-UNEXPECTED-FAIL | /selection/select-internal.html | Selecting the default summary of <details> should report a DOM-visible ancestor - promise_test: Unhandled rejection with value: object "TypeError: can't access property "constructor", selection.anchorNode is null"
[task 2020-10-20T16:13:42.591Z] 16:13:42 INFO - TEST-OK | /selection/select-internal.html | took 774ms

Comment 13

[Kagami Rosylight [:saschanaz] (they/them)]

Ah, so it conflicts with my earlier test. Well, sure it does, and I have no other way to test it 🤔. Maybe we should disable it for now...

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/53d498aa9bf2
Disable selection within anonymous summary element r=emilio

Comment 15

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/26255 for changes under testing/web-platform/tests

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/53d498aa9bf2

Comment 17

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot