Assertion failure: aChild && outOffset

RESOLVED FIXED in Firefox 53

Status

()

Core
Editor
P2
normal
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: truber, Assigned: m_kato)

Tracking

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

36 Branch
mozilla55
assertion, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

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

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

7 months ago
Created attachment 8849117 [details]
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

7 months ago
Priority: -- → P2
(Assignee)

Updated

7 months ago
Assignee: nobody → m_kato
(Assignee)

Updated

7 months 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

7 months 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

7 months 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

7 months 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

7 months 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

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/eb8946f3cc00
https://hg.mozilla.org/mozilla-central/rev/b5c06f8247e2
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Should we consider backporting this or let it ride the trains?
status-firefox52: --- → wontfix
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox-esr52: --- → affected
Flags: needinfo?(m_kato)
Flags: in-testsuite?
Flags: in-testsuite+
Version: Trunk → 36 Branch
(Assignee)

Comment 11

7 months 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

7 months 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+

Comment 14

7 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/c1bdfc489807
https://hg.mozilla.org/releases/mozilla-aurora/rev/a2362db99b02
status-firefox54: affected → fixed

Comment 15

7 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/d7f1e9e1d578
https://hg.mozilla.org/releases/mozilla-beta/rev/a2e8b78114bc
status-firefox53: affected → fixed
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.
status-firefox-esr52: affected → wontfix
You need to log in before you can comment on or make changes to this bug.