1348851 - Assertion failure: aChild && outOffset

Status: RESOLVED FIXED

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

Description

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

The attached testcase causes an assertion failure in mozilla-central rev 8d967436d696.

[14923] ###!!! ASSERTION: null node passed to IsTextNode(): 'Not Reached', file /home/worker/workspace/build/src/editor/libeditor/EditorBase.cpp, line 3689
Assertion failure: aChild && outOffset, at /home/worker/workspace/build/src/editor/libeditor/EditorBase.cpp:3067
#01: mozilla::HTMLEditRules::PinSelectionToNewBlock at editor/libeditor/HTMLEditRules.cpp:7340
#02: mozilla::HTMLEditRules::AfterEditInner at editor/libeditor/HTMLEditRules.cpp:519
#03: mozilla::HTMLEditRules::AfterEdit at editor/libeditor/HTMLEditRules.cpp:410
#04: mozilla::HTMLEditor::EndOperation at editor/libeditor/HTMLEditor.cpp:3514
#05: mozilla::AutoRules::~AutoRules at editor/libeditor/EditorUtils.h:253
#06: mozilla::HTMLEditor::MakeOrChangeList at editor/libeditor/HTMLEditor.cpp:1983
#07: nsListCommand::ToggleState at xpcom/string/nsTSubstring.h:395
#08: nsBaseStateUpdatingCommand::DoCommand at editor/composer/nsComposerCommands.cpp:92
#09: nsControllerCommandTable::DoCommand at dom/commandhandler/nsControllerCommandTable.cpp:147
#10: nsBaseCommandController::DoCommand at xpcom/base/nsCOMPtr.h:801
#11: nsCommandManager::DoCommand at dom/commandhandler/nsCommandManager.cpp:210
#12: nsHTMLDocument::ExecCommand at dom/bindings/ErrorResult.h:375

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Bug 1348851 - Part 1. Use new block when better selection isn't found.

This is a kind of regression of bug 1088054 part 6.  If new block doesn't have child node, GetLastEditableChild will return null after landing bug 1088054. So, we should use new block when GetLastEditableChild returns null.

Review commit: https://reviewboard.mozilla.org/r/122176/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122176/

Comment 2

[Makoto Kato [:m_kato]]

Attachment: Bug 1348851 - Part 2. Add crash test.

Review commit: https://reviewboard.mozilla.org/r/122178/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122178/

Comment 3

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

Comment on attachment 8849422 [details]
Bug 1348851 - Part 1. Use new block when better selection isn't found.

https://reviewboard.mozilla.org/r/122176/#review124316

::: editor/libeditor/HTMLEditRules.cpp:7305
(Diff revision 1)
>    nsCOMPtr<nsIDOMNode> selNode, temp;
>    int32_t selOffset;
>    nsresult rv =
>      EditorBase::GetStartNodeAndOffset(aSelection,
>                                        getter_AddRefs(selNode), &selOffset);
>    NS_ENSURE_SUCCESS(rv, rv);
>    temp = selNode;

Wow, looks like that this |temp| won't be used after here. Could you remove this? It's very unclear name and I'm confused with |tmp| declared below.

::: editor/libeditor/HTMLEditRules.cpp:7307
(Diff revision 1)
>    nsresult rv =
>      EditorBase::GetStartNodeAndOffset(aSelection,
>                                        getter_AddRefs(selNode), &selOffset);
>    NS_ENSURE_SUCCESS(rv, rv);
>    temp = selNode;
>  
>    // use ranges and sRangeHelper to compare sel point to new block
>    nsCOMPtr<nsINode> node = do_QueryInterface(selNode);
>    NS_ENSURE_STATE(node);
>    RefPtr<nsRange> range = new nsRange(node);
>    rv = range->SetStart(selNode, selOffset);
>    NS_ENSURE_SUCCESS(rv, rv);
>    rv = range->SetEnd(selNode, selOffset);
>    NS_ENSURE_SUCCESS(rv, rv);

Well, although, out of scope of this bug, I don't understand this block. We should just get first selection range instead of creating new range because new range won't be used except comparing it with mNewBlock. So, it must not be no problem to use the result of getRangeAt(0) directly.

::: editor/libeditor/HTMLEditRules.cpp:7322
(Diff revision 1)
> -  rv = nsRange::CompareNodeToRange(block, range, &nodeBefore, &nodeAfter);
> +  rv = nsRange::CompareNodeToRange(mNewBlock, range, &nodeBefore, &nodeAfter);
>    NS_ENSURE_SUCCESS(rv, rv);

If mNewBlock is nullptr, nsRange::CompareNodeToRange() returns an error. So, I think that we should check it immediately after checking if the selection is collapsed. (Perhaps, NS_ERROR_NULL_POINTER?)

Comment 4

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

Comment on attachment 8849423 [details]
Bug 1348851 - Part 2. Add crash test.

https://reviewboard.mozilla.org/r/122178/#review124320

::: editor/libeditor/crashtests/1348851.html:16
(Diff revision 1)
> +  document.execCommand("insertunorderedlist");
> +}
> +addEventListener("DOMContentLoaded", boom);
> +</script>
> +</head>
> +<body style="display:flex;">

Is |style="display:flex;"| really necessary? If yes, it's okay. But otherwise, please remove this style attribute.

::: editor/libeditor/crashtests/1348851.html:17
(Diff revision 1)
> +}
> +addEventListener("DOMContentLoaded", boom);
> +</script>
> +</head>
> +<body style="display:flex;">
> +<!--comment-->

I guess that this comment node is necessary.

Comment 5

[Makoto Kato [:m_kato]]

Comment on attachment 8849422 [details]
Bug 1348851 - Part 1. Use new block when better selection isn't found.

https://reviewboard.mozilla.org/r/122176/#review125362

::: editor/libeditor/HTMLEditRules.cpp:7322
(Diff revision 1)
> -  rv = nsRange::CompareNodeToRange(block, range, &nodeBefore, &nodeAfter);
> +  rv = nsRange::CompareNodeToRange(mNewBlock, range, &nodeBefore, &nodeAfter);
>    NS_ENSURE_SUCCESS(rv, rv);

OK, Actually, mNewBlock already checks before this is called.  But we should do it that this is called from anothers at feature.

Comment 6

[Makoto Kato [:m_kato]]

Comment on attachment 8849422 [details]
Bug 1348851 - Part 1. Use new block when better selection isn't found.

https://reviewboard.mozilla.org/r/122176/#review124316

> Wow, looks like that this |temp| won't be used after here. Could you remove this? It's very unclear name and I'm confused with |tmp| declared below.

Ah, I will remove it.

Comment 7

[Makoto Kato [:m_kato]]

Comment on attachment 8849423 [details]
Bug 1348851 - Part 2. Add crash test.

https://reviewboard.mozilla.org/r/122178/#review124320

> Is |style="display:flex;"| really necessary? If yes, it's okay. But otherwise, please remove this style attribute.

If removing this, this crash doesn't occur.

> I guess that this comment node is necessary.

If removing this, this crash doesn't occur.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb8946f3cc00
Part 1. Use new block when better selection isn't found. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5c06f8247e2
Part 2. Add crash test. r=masayuki

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eb8946f3cc00
https://hg.mozilla.org/mozilla-central/rev/b5c06f8247e2

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Should we consider backporting this or let it ride the trains?

Comment 11

[Makoto Kato [:m_kato]]

Comment on attachment 8849422 [details]
Bug 1348851 - Part 1. Use new block when better selection isn't found.

Approval Request Comment
[Feature/Bug causing the regression]:
bug 1088054

[User impact if declined]:
when using debug build, document.execCommand("insertunorderedlist") hits debug assertion with design mode.

[Is this code covered by automated tests?]:
Yes.

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]: 
No

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
Low

[Why is the change risky/not risky?]:
removing assertion case

[String changes made/needed]:

Comment 12

[Makoto Kato [:m_kato]]

[String changes made/needed]:
None

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8849422 [details]
Bug 1348851 - Part 1. Use new block when better selection isn't found.

Removes an unnecessary assertion, let's bring it to beta.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c1bdfc489807
https://hg.mozilla.org/releases/mozilla-aurora/rev/a2362db99b02

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/d7f1e9e1d578
https://hg.mozilla.org/releases/mozilla-beta/rev/a2e8b78114bc

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Doesn't seem like there's enough user impact here to justify taking this on ESR52 as well. Feel free to set it back to affected and request approval if you feel otherwise.