1883802 - Assertion failure: mFrameSelection->GetPresShell()->GetDocument() == content->GetComposedDoc(), at /builds/worker/checkouts/gecko/dom/base/Selection.cpp:1619

Status: VERIFIED FIXED

(Core :: DOM: Selection, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Found while fuzzing m-c 20240228-38dd14d1a2a5 (--enable-debug --enable-fuzzing)

To reproduce via Grizzly Replay:

$ pip install fuzzfetch grizzly-framework
$ python -m fuzzfetch -d --fuzzing -n firefox
$ python -m grizzly.replay.bugzilla ./firefox/firefox <bugid>

Assertion failure: mFrameSelection->GetPresShell()->GetDocument() == content->GetComposedDoc(), at /builds/worker/checkouts/gecko/dom/base/Selection.cpp:1619

#0 0x7fdd45ccb53c in mozilla::dom::Selection::GetPrimaryFrameForCaretAtFocusNode(bool) const /builds/worker/checkouts/gecko/dom/base/Selection.cpp:1618:3
#1 0x7fdd45cd84e2 in mozilla::dom::Selection::Modify(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, mozilla::ErrorResult&) /builds/worker/checkouts/gecko/dom/base/Selection.cpp:3778:7
#2 0x7fdd466a3b01 in mozilla::dom::Selection_Binding::modify(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) /builds/worker/workspace/obj-build/dom/bindings/./SelectionBinding.cpp:1073:24
#3 0x7fdd4707649e in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/checkouts/gecko/dom/bindings/BindingUtils.cpp:3258:13
#4 0x7fdd4b5966d4 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:479:13
#5 0x7fdd4b59602b in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:573:12
#6 0x7fdd4b5a5948 in CallFromStack /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:645:10
#7 0x7fdd4b5a5948 in js::Interpret(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:3060:16
#8 0x7fdd4b5955b2 in js::RunScript(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:451:13
#9 0x7fdd4b596048 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:605:13
#10 0x7fdd4b5972fd in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:672:8
#11 0x7fdd4b6b3ec4 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/vm/CallAndConstruct.cpp:119:10
#12 0x7fdd45ef4fff in mozilla::dom::FrameRequestCallback::Call(mozilla::dom::BindingCallContext&, JS::Handle<JS::Value>, double, mozilla::ErrorResult&) /builds/worker/workspace/obj-build/dom/bindings/./AnimationFrameProviderBinding.cpp:44:8
#13 0x7fdd456f3255 in mozilla::dom::FrameRequestCallback::Call(double, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*) /builds/worker/workspace/obj-build/dist/include/mozilla/dom/AnimationFrameProviderBinding.h:85:12
#14 0x7fdd498369a4 in Call /builds/worker/workspace/obj-build/dist/include/mozilla/dom/AnimationFrameProviderBinding.h:98:12
#15 0x7fdd498369a4 in nsRefreshDriver::RunFrameRequestCallbacks(mozilla::TimeStamp) /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:2393:44
#16 0x7fdd4983783a in nsRefreshDriver::TickObserverArray(unsigned int, mozilla::TimeStamp) /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:2482:5
#17 0x7fdd49834488 in nsRefreshDriver::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp, nsRefreshDriver::IsExtraTick) /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:2735:28
#18 0x7fdd4983dfd1 in TickDriver /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:367:13
#19 0x7fdd4983dfd1 in mozilla::RefreshDriverTimer::TickRefreshDrivers(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver>>&) /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:345:7
#20 0x7fdd4983ded0 in mozilla::RefreshDriverTimer::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:361:5
#21 0x7fdd4983dd6d in mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:951:5
#22 0x7fdd4983d00c in mozilla::VsyncRefreshDriverTimer::TickRefreshDriver(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:861:5
#23 0x7fdd4983c279 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsyncTimerOnMainThread() /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:592:14
#24 0x7fdd48b556fb in mozilla::dom::VsyncMainChild::RecvNotify(mozilla::VsyncEvent const&, float const&) /builds/worker/checkouts/gecko/dom/ipc/VsyncMainChild.cpp:66:15
#25 0x7fdd48e4545d in mozilla::dom::PVsyncChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/obj-build/ipc/ipdl/PVsyncChild.cpp:237:78
#26 0x7fdd48d2d3e0 in mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/obj-build/ipc/ipdl/PContentChild.cpp:8277:32
#27 0x7fdd44bf311f in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:1812:25
#28 0x7fdd44befe72 in mozilla::ipc::MessageChannel::DispatchMessage(mozilla::ipc::ActorLifecycleProxy*, mozilla::UniquePtr<IPC::Message, mozilla::DefaultDelete<IPC::Message>>) /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:1731:9
#29 0x7fdd44bf0af2 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::ActorLifecycleProxy*, mozilla::ipc::MessageChannel::MessageTask&) /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:1524:3
#30 0x7fdd44bf1c3f in mozilla::ipc::MessageChannel::MessageTask::Run() /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:1622:14
#31 0x7fdd43ef3377 in mozilla::RunnableTask::Run() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:578:16
#32 0x7fdd43ee8ae6 in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:905:26
#33 0x7fdd43ee72c7 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:728:15
#34 0x7fdd43ee7745 in mozilla::TaskController::ProcessPendingMTTask(bool) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:514:36
#35 0x7fdd43ef7316 in operator() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:232:37
#36 0x7fdd43ef7316 in mozilla::detail::RunnableFunction<mozilla::TaskController::TaskController()::$_0>::Run() /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.h:548:5
#37 0x7fdd43f0c682 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1199:16
#38 0x7fdd43f137cd in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:480:10
#39 0x7fdd44bf9065 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:85:21
#40 0x7fdd44b0f411 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:363:3
#41 0x7fdd44b0f411 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:345:3
#42 0x7fdd49468158 in nsBaseAppShell::Run() /builds/worker/checkouts/gecko/widget/nsBaseAppShell.cpp:148:27
#43 0x7fdd4952abb8 in nsAppShell::Run() /builds/worker/checkouts/gecko/widget/gtk/nsAppShell.cpp:470:33
#44 0x7fdd4b36282b in XRE_RunAppShell() /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:712:20
#45 0x7fdd44bf9f46 in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:235:9
#46 0x7fdd44b0f411 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:363:3
#47 0x7fdd44b0f411 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:345:3
#48 0x7fdd4b362092 in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:647:34
#49 0x564cac4e33f6 in content_process_main /builds/worker/checkouts/gecko/browser/app/../../ipc/contentproc/plugin-container.cpp:57:28
#50 0x564cac4e33f6 in main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:375:18
#51 0x7fdd58a29d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
#52 0x7fdd58a29e3f in __libc_start_main csu/../csu/libc-start.c:392:3
#53 0x564cac4b9128 in _start (/home/user/workspace/browsers/m-c-20240304165340-fuzzing-debug/firefox-bin+0x59128) (BuildId: 4cd10e79b03551268afff4f2a04b8043968b0ce3)

Comment 1

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20240306044140-099f52f30c39.
The bug appears to have been introduced in the following build range:

Start: 6d9a0abd0a3c26f4337aff723c15795fe91fe884 (20231227091935)
End: 856e86584c4c811f064f93e2b734dd374c787eaf (20231227145854)
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6d9a0abd0a3c26f4337aff723c15795fe91fe884&tochange=856e86584c4c811f064f93e2b734dd374c787eaf

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

The assertion was introduced in Bug 1816581.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1816581

:masayuki, since you are the author of the regressor, bug 1816581, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

At least there are 3 bugs:

1. nsRange does not observe removable of shadow tree which contains the range because it adds itself as a mutation observer to the subtree root.
2. nsRange::ContentRemoved does not check whether the containers in the removed node in the flattened tree.
3. nsRange::ContentRemoved may not be called properly.

I'm still not sure the reason of #3.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, okay, the last one is my bug.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'm still investigating how to fix this. Then, I get a problem now.

When a node is removed, there are these definitions:
4. For each live range whose start node is an inclusive descendant of node, set its start to (parent, index).
5. For each live range whose end node is an inclusive descendant of node, set its end to (parent, index).

The "inclusive descendant" is defined as "An inclusive descendant is an object or one of its descendants".

From these definitions, I thought that the range should be collapsed into the parent tree if the shadow host which contains the range is removed. However, all browsers do not move the range in the shadow (even if its mode is "open") outside the host. I'm not sure whether I'm wrong or the browsers have not taken care of this case yet or the spec is wrong. WDYT smaug?

(I think that the reasonable fix for now is, nsRange should observe mutations under the composed document root instead of the range root, then, if the subtree is removed from the tree, the range should be removed from Selection.)

Comment 7

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

(shadow mode shouldn't matter at all. It is in general supposed to affect only whether one can access element.shadowRoot)

Hmm, it should be enough to observe inside shadow DOM. ParentChainChanged should get called, no?
I wonder if nsRange::ParentChainChanged is doing wrong thing currently. I think it it.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you. I didn't know (or just forgot) the method. However, there is a bug but I have a patch fixing it. I'll post it with some tests.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1883802 - Make `nsRange` observe mutations of composed document r=smaug!,#dom-core

nsRange observers mutation of subtree root if the range is in a subtree.
However, it cannot observer a removal entire the subtree if its inclusive
ancestor of the host is removed from the tree. Therefore, nsRange should
listen to the composed document if it's connected and if it's not in a native
anonymous subtree (e.g., if the range is in selection for TextEditor, it
shouldn't be changed to a range outside the native anonymous subtree).

Note that Chrome does not move selection range in this case. The range keeps
in the uncomposed range. However, it's hard to align the behavior since we
need to the check whether each selection range is in composed tree or not in a
lot of places. Therefore, this patch moves the range as in a light DOM only
when the range is a selection range. This behavior should be standardized later.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1883802 - Make `ShadowRoot` notify mutation observers of "parent chain changed" r=smaug!,#dom-core

Currently, Element notifies the observers of the notification. Then,
Element::UnbindFromTree calls same method of all children in same tree, but
does not call ShadowRoot. Therefore, the method should call it too. Then,
ShadowRoot is not an Element, so, it needs to notify the observers of the
change by itself.

With this change, nsRange starts removing itself from all Selections if its
root becomes disconnected. This behavior is different from Chrome, Chrome
allows the disconnected range in shadow as a selection range. However, it
does not match with the behavior tested in
move-selection-range-into-different-root.tentative.html [1][2]. Therefore, I
think that removing the range from Selection is reasonable for now because
it's not reasonable to add a check to all selection range getters/users to check
whether the range is in the composed tree.

1. https://searchfox.org/mozilla-central/rev/b60cb73160843adb5a5a3ec8058e75a69b46acf7/testing/web-platform/tests/selection/move-selection-range-into-different-root.tentative.html
2. https://wpt.fyi/results/selection/move-selection-range-into-different-root.tentative.html?run_id=5070898326929408&run_id=5147803004698624&run_id=5098392828510208&run_id=5175063246012416

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dda1e753390e
Make `ShadowRoot` notify mutation observers of "parent chain changed" r=smaug

Comment 12

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

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

Comment 13

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/dda1e753390e

Comment 14

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

Upstream PR merged by moz-wptsync-bot

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:masayuki, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox124 to wontfix.

For more information, please visit BugBot documentation.

Comment 16

[Bugmon [:jkratzer for issues]]

Verified bug as fixed on rev mozilla-central 20240312092552-5dbe89396d7f.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This is really an old bug actually and it's too late to uplift.