1511563 - Assertion failure: !mRootContent->IsText(), at src/dom/events/ContentEventHandler.cpp:1202

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Reduced with m-c:
BuildID=20181130200939
SourceStamp=950f6d29da967b9999ce709e94bf35b244f79100

Assertion failure: !mRootContent->IsText(), at src/dom/events/ContentEventHandler.cpp:1202

#0 mozilla::ContentEventHandler::SetRawRangeFromFlatTextOffset(mozilla::ContentEventHandler::RawRange*, unsigned int, unsigned int, mozilla::LineBreakType, bool, unsigned int*, nsIContent**) src/dom/events/ContentEventHandler.cpp:1202:5
#1 mozilla::ContentEventHandler::OnQueryTextContent(mozilla::WidgetQueryContentEvent*) src/dom/events/ContentEventHandler.cpp:1409:8
#2 mozilla::IMEContentObserver::HandleQueryContentEvent(mozilla::WidgetQueryContentEvent*) src/dom/events/IMEContentObserver.cpp:756:25
#3 mozilla::EventStateManager::HandleQueryContentEvent(mozilla::WidgetQueryContentEvent*) src/dom/events/EventStateManager.cpp:902:22
#4 mozilla::EventStateManager::PreHandleEvent(nsPresContext*, mozilla::WidgetEvent*, nsIFrame*, nsIContent*, nsEventStatus*, nsIContent*) src/dom/events/EventStateManager.cpp:493:5
#5 mozilla::PresShell::HandleEventInternal(mozilla::WidgetEvent*, nsEventStatus*, bool, nsIContent*) src/layout/base/PresShell.cpp:7238:19
#6 mozilla::PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*) src/layout/base/PresShell.cpp:6964:12
#7 nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent*, nsView*, nsEventStatus*) src/view/nsViewManager.cpp:763:14
#8 nsView::HandleEvent(mozilla::WidgetGUIEvent*, bool) src/view/nsView.cpp:1059:9
#9 mozilla::widget::PuppetWidget::DispatchEvent(mozilla::WidgetGUIEvent*, nsEventStatus&) src/widget/PuppetWidget.cpp:379:37
#10 mozilla::ContentCacheInChild::CacheText(nsIWidget*, mozilla::widget::IMENotification const*) src/widget/ContentCache.cpp:234:12
#11 mozilla::ContentCacheInChild::CacheAll(nsIWidget*, mozilla::widget::IMENotification const*) src/widget/ContentCache.cpp:122:7
#12 mozilla::widget::PuppetWidget::NotifyIMEOfFocusChange(mozilla::widget::IMENotification const&) src/widget/PuppetWidget.cpp:755:11
#13 mozilla::widget::TextEventDispatcher::NotifyIME(mozilla::widget::IMENotification const&) src/widget/TextEventDispatcher.cpp:458:40
#14 nsBaseWidget::NotifyIME(mozilla::widget::IMENotification const&) src/widget/nsBaseWidget.cpp:1722:43
#15 mozilla::IMEStateManager::NotifyIME(mozilla::widget::IMENotification const&, nsIWidget*, mozilla::dom::TabParent*) src/dom/events/IMEStateManager.cpp:1628:22
#16 mozilla::IMEContentObserver::IMENotificationSender::SendFocusSet() src/dom/events/IMEContentObserver.cpp:1827:3
#17 mozilla::IMEContentObserver::IMENotificationSender::Run() src/dom/events/IMEContentObserver.cpp:1704:5
#18 nsRefreshDriver::Tick(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:1691:13
#19 mozilla::RefreshDriverTimer::TickRefreshDrivers(mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) src/layout/base/nsRefreshDriver.cpp:304:7
#20 mozilla::RefreshDriverTimer::Tick(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:321:5
#21 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:642:16
#22 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:542:9
#23 mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) src/layout/ipc/VsyncChild.cpp:66:16
#24 mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:167:20
#25 mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:2721:28
#26 mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) src/ipc/glue/MessageChannel.cpp:2124:21
#27 mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) src/ipc/glue/MessageChannel.cpp:2051:9
#28 mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) src/ipc/glue/MessageChannel.cpp:1900:3
#29 mozilla::ipc::MessageChannel::MessageTask::Run() src/ipc/glue/MessageChannel.cpp:1931:13
#30 nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1157:14
#31 NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:468:10
#32 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:88:21
#33 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:314:10
#34 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:289:3
#35 nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:137:27
#36 XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:915:20
#37 mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:238:9
#38 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:314:10
#39 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:289:3
#40 XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:753:34
#41 content_process_main(mozilla::Bootstrap*, int, char**) src/browser/app/../../ipc/contentproc/plugin-container.cpp:49:28
#42 main src/browser/app/nsBrowserApp.cpp:265:18
#43 __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
#44 _start (firefox+0x349f4)

Comment 1

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

Hi Masayuki,
Do you have ideas of where can go wrong?

Comment 2

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

No idea, but this is my fault, I'll investigate this.

Comment 3

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=146feed3ac79af8acc342240f9d920d04c65e521

Comment 4

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d01a3170d68e5b063839fe18b5a972b4a8d6722f

Comment 5

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=80de731764c9cb15712a7e84b6bf681a0deb4e2b

Comment 6

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=72d1a462516f4c7580e4bc9283c6d442817c8d9a

Comment 7

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cf3bb7e04c1ac53f39cdeb1adad818845930b82

Comment 8

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

Attachment: Bug 1511563 - nsFrameSelection shouldn't set selection into text node in <select> element

The cause of bug 1511563 is, nsINode::GetSelectionRootContent() returns a text
node in <select> (which represents <keygen>) because selection has been
set into a text node in it.

Investigating with debugger, both nsFrameSelection::MoveCaret() and
PresShell::CompleteMove() do that with nsFrameSelection::HandleClick().

If we could avoid this in nsFrameSelection::HandleClick(), it'd be better,
but looks like that it's too risky since some callers might want to put
selection into binding child like <input> element in contentediable.
 Therefore, this patch fixes each caller.

nsFrameSelection::MoveCaret() uses result of nsIFrame::PeekOffset() directly.
However, this is generic layout API.  So, MoveCaret() needs adjust expanding
limit by itself for making this change safer.  Therefore, this needs to use
nsIFrame::GetExtremeCaretPosition() to retrieve the good position to move
focus node.  This is tested by new tests in test_selection_expanding.html.

Finally, PresShell::CompleteMove() uses nsIFrame::GetExtremeCaretPosition() too.
It's the only one user of the API before this patch.  Therefore, we can correct
its behavior safely.  The method name means that it's useful to retrieve where
is good position to move focus node in the frame.  So, if found frame's user
select style is all or none, we should put caret around the frame rather than
in the found frame.  And if the found frame is the frame itself, it should
return no place since the frame may be root frame of current selection (i.e.,
its caller shouldn't move selection outside of it).

Therefore, this patch makes it return the parent content as content and index
in it for "before", and index + 1 for "after".  Although, it's different from
Chrome whether to put caret before or after <select> element in this case, but
new behavior is closer than current behavior (i.e., putting caret into
<select>).

Comment 9

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

mats, do you think you could review this?

Comment 10

[Mats Palmgren (inactive)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #8)
> nsINode::GetSelectionRootContent() returns a text
> node in <select> (which represents <keygen>) because selection has been
> set into a text node in it.

Hmm, is that the NAC text node for the ComboboxDisplay child frame?

Does DrillDownToSelectionFrame return a child frame of the ComboboxControlFrame?
(which one?)

If so, that seems wrong to me and we should prevent that from
happening in the first place rather than deal with it in callers.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

I agree with that. Also, is this a regression? I've done a few caret-related changes recently that could affect this.

Comment 12

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

(In reply to Mats Palmgren (:mats) from comment #10)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #8)
> > nsINode::GetSelectionRootContent() returns a text
> > node in <select> (which represents <keygen>) because selection has been
> > set into a text node in it.
> 
> Hmm, is that the NAC text node for the ComboboxDisplay child frame?
> 
> Does DrillDownToSelectionFrame return a child frame of the
> ComboboxControlFrame?
> (which one?)

No, it returns nsComboboxControlFrame. Then, GetRangeForFrame returns [HTMLSelectRange, 0]-[HTMLSelectRange, GetChildCount()] in nsIFrame::GetExtremeCaretPosition(). Then, its caller calls nsFrameSelection::HandleClick().

> If so, that seems wrong to me and we should prevent that from
> happening in the first place rather than deal with it in callers.

Actually the following things happen with current build:

1. nsFrameSelection::MoveCaret() is called by Selection::Modify().
2. Then, nsFrameSelection::MoveCaret() calls nsIFrame::PeekOffset() to retrieve next offset.
3. At this time, nsIFrame::PeekOffset() returns anonymous text node in the <select> element.
4. Then, nsFrameSelection::TakeFocus() is called with the text node and it causes selection into it.

I think that nsIFrame::PeekOffset() is not an API only for Selection. Therefore, we need to keep current behavior (or adds new mode with a flag, like nsPeekOffsetStruct::mForSelection? But I don't think that this is good approach because of making its maintenance harder for both selection and layout).  Therefore, I added adjusting selection code into nsFrameSelection::MoveCaret() with making it use nsIFrame::GetExtremeCaretPosition().

Comment 13

[Emilio Cobos Álvarez (:emilio)]

I don't think the anonymous node in <select> (or the options, which in the test-case you can edit, lol) should be editable, right?

nsFrameSelection already skips non-editable nodes inside an editable selection fwiw (see ForceEditableRegion).

Should we just force <select> to be non-editable? Maybe even non-selectable via -moz-user-select: none?

Comment 14

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

(In reply to Emilio Cobos Álvarez (:emilio) from comment #13)
> I don't think the anonymous node in <select> (or the options, which in the
> test-case you can edit, lol) should be editable, right?

Perhaps, yes it is. Editing ability of form controls is undefined.

> nsFrameSelection already skips non-editable nodes inside an editable
> selection fwiw (see ForceEditableRegion).

I'll check it.

> Should we just force <select> to be non-editable? Maybe even non-selectable
> via -moz-user-select: none?

No, the approach makes users cannot select/delete <select> elements.

Comment 15

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

(In reply to Masayuki Nakano [:masayuki] (JST, +0900)(sick) from comment #14)

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

I don't think the anonymous node in <select> (or the options, which in the
test-case you can edit, lol) should be editable, right?

Perhaps, yes it is. Editing ability of form controls is undefined.

nsFrameSelection already skips non-editable nodes inside an editable
selection fwiw (see ForceEditableRegion).

I'll check it.

Hmm, in nsFrameSelection::MoveCaret(), nsIFrame::PeekOffset() returns an anonymous text node which is editable.

So, there are some issues here:

1. nsIFrame::GetExtremeCaretPosition() calls nsIFrame::GetRangeForFrame() even if targetFrame.frame has only anonymous children. Then, GetRangeForFrame() returns anonymous children. Therefore, GetExtremeCaretPosition() should avoid calling GetRangeForFrame() in such case.
2. Anonymous children of <select> may be set to editable.
3. nsIFrame::PeekOffset() may return different anonymous tree node.

Currently, my patch completely fixes only #1. Perhaps, only it should be fixed in another bug first because the other thing are really complicated and if we need to touch nsIFrame::PeekOffset(), I don't have much time to do it for now.

Any ideas?

Comment 16

[Emilio Cobos Álvarez (:emilio)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900)(sick) from comment #15)

Hmm, in nsFrameSelection::MoveCaret(), nsIFrame::PeekOffset() returns an anonymous text node which is editable.

So looks to me that is the bug. Bug 805668 prevented these text nodes from being editable via contenteditable, but I don't see anything in nsINode::IsEditable that would prevent us from consider them editable from designMode. Looks to me like that is the bug.

I can take a closer look in the debugger, so not clearing ni? for now.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

I took a closer look to this.

What do you think of https://hg.mozilla.org/try/rev/5663a9c2d81aa523e2f0c8dca1bae7db2339e39d Masayuki? I'm waiting for try atm, but I think it should be green.

It looks like a cleaner way to properly accomplish what nsIContent::UpdateEditableState is trying to do, and it of course makes the assertion go away.

I think we can still end up with the caret in the <select> element in other cases, but I think that's a different bug: nsComboboxControlFrame should have independent selection IMO, so something like this should do:

diff --git a/layout/forms/nsComboboxControlFrame.cpp b/layout/forms/nsComboboxControlFrame.cpp
index 10a4c7861f91..f74d85c924ac 100644
--- a/layout/forms/nsComboboxControlFrame.cpp
+++ b/layout/forms/nsComboboxControlFrame.cpp
@@ -113,12 +113,7 @@ nsComboboxControlFrame* NS_NewComboboxControlFrame(nsIPresShell* aPresShell,
                                                    ComputedStyle* aStyle,
                                                    nsFrameState aStateFlags) {
   nsComboboxControlFrame* it = new (aPresShell) nsComboboxControlFrame(aStyle);
-
-  if (it) {
-    // set the state flags (if any are provided)
-    it->AddStateBits(aStateFlags);
-  }
-
+  it->AddStateBits(aStateFlags | NS_FRAME_INDEPENDENT_SELECTION);
   return it;
 }

Comment 18

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

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

I took a closer look to this.

What do you think of https://hg.mozilla.org/try/rev/5663a9c2d81aa523e2f0c8dca1bae7db2339e39d Masayuki? I'm waiting for try atm, but I think it should be green.

If this won't break <input type="text"> nor <textarea>, looks like reasonable change.

I think we can still end up with the caret in the <select> element in other cases, but I think that's a different bug: nsComboboxControlFrame should have independent selection IMO, so something like this should do:

diff --git a/layout/forms/nsComboboxControlFrame.cpp b/layout/forms/nsComboboxControlFrame.cpp
index 10a4c7861f91..f74d85c924ac 100644
--- a/layout/forms/nsComboboxControlFrame.cpp
+++ b/layout/forms/nsComboboxControlFrame.cpp
@@ -113,12 +113,7 @@ nsComboboxControlFrame* NS_NewComboboxControlFrame(nsIPresShell* aPresShell,
                                                    ComputedStyle* aStyle,
                                                    nsFrameState aStateFlags) {
   nsComboboxControlFrame* it = new (aPresShell) nsComboboxControlFrame(aStyle);
-
-  if (it) {
-    // set the state flags (if any are provided)
-    it->AddStateBits(aStateFlags);
-  }
-
+  it->AddStateBits(aStateFlags | NS_FRAME_INDEPENDENT_SELECTION);
   return it;
 }

Looks like that nsListControlFrame also has the flag having independent selection, but actually it does not have nsISelectionController object too. So, it seems that this change must be fine.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1511563 - Don't make NAC editable by default.

I think this is a more consistent fix for the issue. Will add a
crashtest for it before landing.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

So this is fully green except for one IME test which checks the IME state of an <input type="file"> inside contenteditable:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=90275753ad3d2f8475fe9770ae97f5dbedc080ad&selectedJob=221754271

It sort of makes sense because <input type="file"> would get its behavior changed (the anonymous <button> would not get the NODE_IS_EDITABLE flag). But I'm not sure about the implications of failing that test.

And what's worse, the test passes locally even if it fails on automation (I'm on Linux as well).

Masayuki, do you know what that test is about? I don't have any idea about IME.

Is it acceptable to just update the expectation? If not, do you know how I could get to repro it or debug it? How is the "ime enabled" value computed? As I mention it passes locally here.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

heh, you're faster reviewing than I am writing :-)

Comment 22

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

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

So this is fully green except for one IME test which checks the IME state of an <input type="file"> inside contenteditable:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=90275753ad3d2f8475fe9770ae97f5dbedc080ad&selectedJob=221754271

It sort of makes sense because <input type="file"> would get its behavior changed (the anonymous <button> would not get the NODE_IS_EDITABLE flag). But I'm not sure about the implications of failing that test.

I built your patch locally and try to check the behavior of <div contenteditable><input type="file"></div>. Then, clicking the <input type="file"> element happens nothing. I mean, the file picker won't be opened, and not selected the element itself, etc. So, even when I type something after clicking it, nothing happens. In this case, i.e., no text inputtable situation, IME should be disabled. So, returning 0 is fine. So, expected value at here should be gUtils.IME_STATUS_DISABLED.

(Note: Chrome behaves almost similar to the patched build, but opens the file picker when I click. Although this is another bug.)

And what's worse, the test passes locally even if it fails on automation (I'm on Linux as well).

Wow, really?? I have no idea what's going on...

Comment 23

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

Ah, might be kEnabledStateOnPasswordField rather than gUtils.IME_STATUS_DISABLED. Please check it on tryserver. (Even though the constant name is odd for <input type="file">, but it's okay to keep using the name.)

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Will do, thanks a lot!

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1285bd13257d
Don't make NAC editable by default. r=masayuki

Comment 26

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/1285bd13257d