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

RESOLVED FIXED in Firefox 66

Status

()

defect
P2
normal
RESOLVED FIXED
9 months ago
7 months ago

People

(Reporter: tsmith, Assigned: masayuki)

Tracking

(Blocks 1 bug, {assertion, testcase})

unspecified
mozilla66
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox64 wontfix, firefox65 wontfix, firefox66 fixed)

Details

Attachments

(3 attachments)

Posted file 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)
Flags: in-testsuite?
Hi Masayuki,
Do you have ideas of where can go wrong?
Flags: needinfo?(masayuki)
No idea, but this is my fault, I'll investigate this.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Flags: needinfo?(masayuki)
Priority: -- → P2
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>).
mats, do you think you could review this?
(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.
I agree with that. Also, is this a regression? I've done a few caret-related changes recently that could affect this.
(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().
Flags: needinfo?(mats)
Flags: needinfo?(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?
Flags: needinfo?(emilio) → needinfo?(masayuki)
(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.
Flags: needinfo?(masayuki)

(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?

Flags: needinfo?(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.

See Also: → 805668

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;
 }
Flags: needinfo?(emilio) → needinfo?(masayuki)

(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.

Flags: needinfo?(masayuki)
I think this is a more consistent fix for the issue. Will add a
crashtest for it before landing.

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.

Flags: needinfo?(masayuki)

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

(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...

Flags: needinfo?(masayuki)

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.)

Will do, thanks a lot!

Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1285bd13257d
Don't make NAC editable by default. r=masayuki
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Flags: needinfo?(mats)
Flags: in-testsuite?
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.