Closed Bug 1348851 Opened 5 years ago Closed 5 years ago

Assertion failure: aChild && outOffset

Categories

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

36 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: truber, Assigned: m_kato)

References

Details

(Keywords: assertion, regression, testcase)

Attachments

(3 files)

Attached file 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
Flags: in-testsuite?
Priority: -- → P2
Assignee: nobody → m_kato
Blocks: 1088054
Keywords: regression
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?)
Attachment #8849422 - Flags: review?(masayuki) → review+
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.
Attachment #8849423 - Flags: review?(masayuki) → review+
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 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 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.
Pushed by m_kato@ga2.so-net.ne.jp:
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
https://hg.mozilla.org/mozilla-central/rev/eb8946f3cc00
https://hg.mozilla.org/mozilla-central/rev/b5c06f8247e2
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Should we consider backporting this or let it ride the trains?
Flags: needinfo?(m_kato)
Flags: in-testsuite?
Flags: in-testsuite+
Version: Trunk → 36 Branch
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]:
Flags: needinfo?(m_kato)
Attachment #8849422 - Flags: approval-mozilla-beta?
Attachment #8849422 - Flags: approval-mozilla-aurora?
[String changes made/needed]:
None
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.
Attachment #8849422 - Flags: approval-mozilla-beta?
Attachment #8849422 - Flags: approval-mozilla-beta+
Attachment #8849422 - Flags: approval-mozilla-aurora?
Attachment #8849422 - Flags: approval-mozilla-aurora+
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.
You need to log in before you can comment on or make changes to this bug.