Closed
Bug 1348851
Opened 8 years ago
Closed 8 years ago
Assertion failure: aChild && outOffset
Categories
(Core :: DOM: Editor, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: truber, Assigned: m_kato)
References
Details
(Keywords: assertion, regression, testcase)
Attachments
(3 files)
(deleted),
text/html
|
Details | |
(deleted),
text/x-review-board-request
|
masayuki
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details |
(deleted),
text/x-review-board-request
|
masayuki
:
review+
|
Details |
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•8 years ago
|
Priority: -- → P2
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → m_kato
Assignee | ||
Updated•8 years ago
|
Blocks: 1088054
Keywords: regression
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•8 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/#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 4•8 years ago
|
||
mozreview-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•8 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•8 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•8 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.
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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/eb8946f3cc00
https://hg.mozilla.org/mozilla-central/rev/b5c06f8247e2
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 10•8 years ago
|
||
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•8 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•8 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+
Comment 14•8 years ago
|
||
bugherder uplift |
Comment 15•8 years ago
|
||
bugherder uplift |
Comment 16•8 years ago
|
||
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.
Description
•