Assertion failure: !newEditingHost->IsInNativeAnonymousSubtree(), at /home/worker/workspace/build/src/dom/base/Selection.cpp:3740

NEW
Assigned to

Status

()

Core
Selection
P3
normal
a year ago
2 months ago

People

(Reporter: jkratzer, Assigned: m_kato, NeedInfo)

Tracking

(Blocks: 1 bug, {assertion, testcase})

55 Branch
assertion, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 wontfix, firefox57 wontfix, firefox58 wontfix, firefox59 ?)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
Created attachment 8890395 [details]
trigger.html

Testcase found while fuzzing mozilla-central rev 20170726-e8400551c2e3.

Assertion failure: !newEditingHost->IsInNativeAnonymousSubtree(), at /home/worker/workspace/build/src/dom/base/Selection.cpp:3740

ASAN:DEADLYSIGNAL
=================================================================
==29714==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f71f79f9c4b bp 0x7fffab425bd0 sp 0x7fffab425920 T0)
==29714==The signal is caused by a WRITE memory access.
==29714==Hint: address points to the zero page.
    #0 0x7f71f79f9c4a in mozilla::dom::Selection::NotifySelectionListeners() /home/worker/workspace/build/src/dom/base/Selection.cpp:3742:45
    #1 0x7f71fb226227 in nsFrameSelection::NotifySelectionListeners(mozilla::SelectionType) /home/worker/workspace/build/src/layout/generic/nsFrameSelection.cpp:2067:23
    #2 0x7f71f79f4211 in mozilla::dom::Selection::Collapse(nsINode&, unsigned int, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/base/Selection.cpp:2581:28
    #3 0x7f71f79f4be5 in mozilla::dom::Selection::CollapseToEnd(mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/base/Selection.cpp:2682:3
    #4 0x7f71f79f4d5d in mozilla::dom::Selection::CollapseToEndJS(mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/base/Selection.cpp:2653:3
    #5 0x7f71f85c86d6 in mozilla::dom::SelectionBinding::collapseToEnd(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Selection*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/SelectionBinding.cpp:549:9
    #6 0x7f71f928134e in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3060:13
    #7 0x7f71fe1ae6b1 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) /home/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15
    #8 0x7f71fe1ae25d in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:469:16
    #9 0x7f71fe1af0f5 in InternalCall(JSContext*, js::AnyInvokeArgs const&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:514:12
    #10 0x7f71fe1a3b55 in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3064:18
    #11 0x7f71fe18f7b1 in js::RunScript(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:409:12
    #12 0x7f71fe1b0d32 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:698:15
    #13 0x7f71fe1b18c2 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:730:12
    #14 0x7f71fea1a64f in ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::Value*) /home/worker/workspace/build/src/js/src/jsapi.cpp:4637:12
    #15 0x7f71fea1aeb6 in ExecuteScript(JSContext*, JS::AutoObjectVector&, JS::Handle<JSScript*>, JS::Value*) /home/worker/workspace/build/src/js/src/jsapi.cpp:4656:12
    #16 0x7f71fea1aa9e in JS_ExecuteScript(JSContext*, JS::AutoObjectVector&, JS::Handle<JSScript*>, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jsapi.cpp:4677:12
    #17 0x7f71f7bc706b in nsJSUtils::ExecutionContext::CompileAndExec(JS::CompileOptions&, JS::SourceBufferHolder&, JS::MutableHandle<JSScript*>) /home/worker/workspace/build/src/dom/base/nsJSUtils.cpp:265:8
    #18 0x7f71fa97233f in mozilla::dom::ScriptLoader::EvaluateScript(mozilla::dom::ScriptLoadRequest*) /home/worker/workspace/build/src/dom/script/ScriptLoader.cpp:2191:25
    #19 0x7f71fa96f1f0 in mozilla::dom::ScriptLoader::ProcessRequest(mozilla::dom::ScriptLoadRequest*) /home/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1775:10
    #20 0x7f71fa95d5e1 in mozilla::dom::ScriptLoader::ProcessScriptElement(nsIScriptElement*) /home/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1471:10

Updated

11 months ago
Priority: -- → P3

Updated

10 months ago
status-firefox57: --- → wontfix
status-firefox58: --- → fix-optional
INFO: Last good revision: e36088242facca249d718e6d79a6cfa7007fd42b
INFO: First bad revision: f5da859b433fe40803847ad77cc4a573fbf8dc25
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=e36088242facca249d718e6d79a6cfa7007fd42b&tochange=f5da859b433fe40803847ad77cc4a573fbf8dc25

Regressed by bug 1365092.
Blocks: 1365092
Has Regression Range: --- → yes
status-firefox56: --- → wontfix
status-firefox-esr52: --- → unaffected
Version: unspecified → 55 Branch
Kirk, can you take a look?  If you want me to do it, please let me know!
Flags: needinfo?(ksteuber)

Updated

10 months ago
Assignee: nobody → ksteuber
Flags: needinfo?(ksteuber)
I believe I have tracked down the source of the problem.

Previous behavior:
    |nsGenericHTMLElement::SetContentEditable| indirectly invokes the (virtual) function |nsGenericHTMLElement::SetAttr|.
    |nsGenericHTMLElement::SetAttr| invokes |Element::SetAttr|.
    |Element::SetAttr| creates a |mozAutoDocUpdate| object on the stack.
    |Element::SetAttr| returns, destroying the |mozAutoDocUpdate| object.
    |nsGenericHTMLElement::SetAttr| continues execution, calling |ChangeEditableState(1);|.
    This results in |nsHTMLDocument::mContentEditableCount| being changed from 0 to 1.

New behavior:
    |nsGenericHTMLElement::SetContentEditable| indirectly invokes the function |Element::SetAttr|
    |Element::SetAttr| creates a |mozAutoDocUpdate| object on the stack.
    |Element::SetAttr| calls |Element::SetAttrAndNotify|, which calls the (virtual) function |nsGenericHTMLElement::AfterSetAttr|.
    |nsGenericHTMLElement::AfterSetAttr| calls |ChangeEditableState(1);|.
    This results in |nsHTMLDocument::mContentEditableCount| being changed from 0 to 1.
    |Element::SetAttr| returns, destroying the |mozAutoDocUpdate| object.

|mozAutoDocUpdate::~mozAutoDocUpdate| indirectly calls |nsHTMLDocument::MaybeEditingStateChanged|. This function has no effect unless |(mContentEditableCount > 0) != IsEditingOn()| [1].

Because the new behavior changes |mContentEditableCount| before |mozAutoDocUpdate::~mozAutoDocUpdate| is called (rather than after), this function now has a completely different effect. It now results in |Selection::AddItem| being called.

When examining the previous behavior, I see that |Selection::CollapseToEnd| returns early (here [2]). But with the new behavior, it does not return there, ultimately resulting in the assertion failure described.

I am still looking into what the correct solution to this problem might be, but I wanted to post an update on my progress.

[1] http://searchfox.org/mozilla-central/rev/dca019c94bf3a840ed7ff50261483410cfece24f/dom/html/nsHTMLDocument.cpp#2490
[2] http://searchfox.org/mozilla-central/rev/dca019c94bf3a840ed7ff50261483410cfece24f/dom/base/Selection.cpp#2763
Created attachment 8920298 [details]
Modified testcase that hits the same assert without the patches in bug 1365092

This testcase hits the assert on revision e36088242fac, which is before Kirk's patches.

On the original testcase in this bug, before Kirk's patch, we do ~mozAutoDocUpdate before ChangeEditableState(1) happens, so it doesn't turn on editing mode.  Then we land in nsHTMLDocument::ChangeContentEditableCount, which effectively sync-calls DeferredContentEditableCountChangeEvent::Run, which calls nsHTMLDocument::DeferredContentEditableCountChange.  That bails out early because mParser is false, so we don't turn on editing mode.  Then the selection bits don't see an editable area there, and don't end up in whatever weird state triggers the assert.

In the modified testcase, I simply trigger _another_ ~mozAutoDocUpdate after all of that stuff.  This time the ~mozAutoDocUpdate sees a nonzero mContentEditableCount, does EditingStateChanged(), etc.

All that Kirk's patch did was make the _first_ ~mozAutoDocUpdate trigger EditingStateChanged(); it's not the real culprit here.

Kirk and I looked at this for a bit, and we don't really understand the state handling in this code.  DeferredContentEditableCountChange checks mParser (and relies on EndLoad to turn on editing state), but none of the other callers of EditingStateChanged check mParser.  What are the invariants we're trying to maintain here, exactly?  Peter, I think you have blame for all this stuff, and Ehsan is out so you're our best bet here....
Flags: needinfo?(peterv)
Created attachment 8920301 [details]
Another modified testcase that triggers the assert pre-bug-1365092

This is just like trigger.html, but all the code runs onload instead of during parsing, so mParser is null in DeferredContentEditableCountChange and we go ahead and update editing state right when the contenteditable attr is set.

Fundamentally, I expect we're ending up with the selection inside the <input> but it's asserting it's in a contenteditable area or so...
Created attachment 8920309 [details]
Another modified testcase that triggers the assert pre-bug-1365092

Er, forgot to save after uncommenting...
Attachment #8920301 - Attachment is obsolete: true
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #4)
> Kirk and I looked at this for a bit, and we don't really understand the
> state handling in this code.  DeferredContentEditableCountChange checks
> mParser (and relies on EndLoad to turn on editing state), but none of the
> other callers of EditingStateChanged check mParser.  What are the invariants
> we're trying to maintain here, exactly?  Peter, I think you have blame for
> all this stuff, and Ehsan is out so you're our best bet here....

Note that it's been 10 years since I last worked on this, and there's been quite a bit of changes to this code since then that I don't understand yet either.

IIRC there were two main callers to EditingStateChanged, for contentEditable and for designMode.

For contentEditable we delayed turning on editing during page load, IIRC because of concerns for performance. That's the check for mParser that's in EndLoad and used to be in ChangeContentEditableCount (looks like that's now in DeferredContentEditableCountChange). We should probably have added it to the code in EndUpdate (now in MaybeEditingStateChanged) for bug 395340.

The other callers were for designMode, when the property was set and document.open on a document that was already in designMode. Those just didn't have the mParser checks, because it seemed unnecessary. I don't know about other callers that were added later (BeginLoad?).

I'm fine with trying to remove the mParser check if that helps, it'll just turn on editing on the first contentEditable element that's added during pageload. I think the main change will be that we'll be running more spellchecking code in pages with multiple contentEditable elments, but that might be fine.
Flags: needinfo?(peterv)
Maybe Masayuki, Makoto, or Henri know something here (I'm just guessing; feel free to just clear the needinfo and tell me I owe you a beverage some time ;)?
Flags: needinfo?(masayuki)
Flags: needinfo?(m_kato)
Flags: needinfo?(hsivonen)
As mentioned above, the patch that I wrote did not cause the underlying problem here. I do not really have any knowledge specific to the area that needs to be fixed here. I am therefore unassigning myself from this bug. If I can be of any further help though, feel free to needinfo me.
Assignee: ksteuber → nobody
OK.  So my second testcase shows that all this stuff about mParser and when editing gets turned on is a red herring, apart from the code being super-confusing and complicated.  I filed bug 1410504 on that part.

The real issue here is that we end up in a state where part of the document is editable, we do a caret move, the caret ends up in the <input>, I would guess, and then selection asserts we're not in anon content, but of course we are.  So this is basically a question about what our caret behavior should be in this situation...
(In reply to Andrew Overholt [:overholt] from comment #8)
> Maybe Masayuki, Makoto, or Henri know something here (I'm just guessing;

This is outside my area of expertise.
Flags: needinfo?(hsivonen)
I still don't understand this bug. We should hit the assertion only when contenteditable element is in anonymous subtree. If the assertion's condition is wrong (not enough), let me know.
> We should hit the assertion only when contenteditable element is in anonymous subtree.

We're hitting the assertion when newEditingHost is the anonymous <div> inside an <input>.  It's definitely in an anonymous subtree, and it is very much _NOT_ contenteditable in the sense of the HTML attribute.

It does have NODE_IS_EDITABLE, because it's the editing host for the input's editor.

Now another node in the document _is_ contenteditable, which is why Selection::NotifySelectionListener is entering the relevant block at all (document is not editable as a whole, but there is an HTML editor, for the contenteditable node that's there).   And Selection::GetCommonEditingHostForAllRanges just returns the <div> there, because the selection is inside the <input>.

focusedElement is null in this case, by the way.
(Assignee)

Comment 15

4 months ago
Ah, GetCommonEditingHostForAllRanges is only called with contenteditable, but it shouldn't return anonymous node.  I make sense it.
Assignee: nobody → m_kato
Flags: needinfo?(m_kato)
(Assignee)

Updated

2 months ago
Duplicate of this bug: 1463618
You need to log in before you can comment on or make changes to this bug.