Assertion failure: aChild && outOffset

RESOLVED FIXED in Firefox 53

Status

()

defect
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: truber, Assigned: m_kato)

Tracking

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

36 Branch
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox52 wontfix, firefox-esr52 wontfix, firefox53 fixed, firefox54 fixed, firefox55 fixed)

Details

Attachments

(3 attachments)

Posted 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?
Assignee

Updated

2 years ago
Priority: -- → P2
Assignee

Updated

2 years ago
Assignee: nobody → m_kato
Assignee

Updated

2 years ago
Blocks: 1088054
Keywords: regression
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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+
Assignee

Comment 5

2 years ago
mozreview-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.
Assignee

Comment 6

2 years ago
mozreview-review-reply
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.
Assignee

Comment 7

2 years ago
mozreview-review-reply
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

2 years ago
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

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/eb8946f3cc00
https://hg.mozilla.org/mozilla-central/rev/b5c06f8247e2
Status: NEW → RESOLVED
Closed: 2 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
Assignee

Comment 11

2 years ago
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?
Assignee

Comment 12

2 years ago
[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.