1633700 - Crash in [@ mozilla::CreateElementTransaction::InsertNewNode]

Status: RESOLVED FIXED

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

Description

[Alex Chronopoulos [:achronop]]

This bug is for crash report bp-1153e9c3-00ef-4c33-81b6-a2ba10200427.

Top 10 frames of crashing thread:

0 XUL mozilla::CreateElementTransaction::InsertNewNode editor/libeditor/CreateElementTransaction.cpp:151
1 XUL mozilla::CreateElementTransaction::RedoTransaction editor/libeditor/CreateElementTransaction.cpp:193
2 XUL mozilla::EditAggregateTransaction::RedoTransaction editor/libeditor/EditAggregateTransaction.cpp:55
3 XUL mozilla::PlaceholderTransaction::RedoTransaction editor/libeditor/PlaceholderTransaction.cpp:84
4 XUL mozilla::TransactionItem::RedoTransaction editor/txmgr/TransactionItem.cpp:170
5 XUL mozilla::TransactionManager::Redo editor/txmgr/TransactionManager.cpp:171
6 XUL mozilla::TextEditor::RedoAsAction editor/libeditor/TextEditor.cpp:1637
7 XUL mozilla::EditorCommand::DoCommand editor/libeditor/EditorCommands.cpp:64
8 XUL nsControllerCommandTable::DoCommand dom/commandhandler/nsControllerCommandTable.cpp:138
9 XUL nsBaseCommandController::DoCommand dom/commandhandler/nsBaseCommandController.cpp:114

mPointToInsert.GetChild() can return nullptr but the check was removed.
Low traffic crash.

Comment 1

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

Interesting. And we should have a patch just prevent the crash at least for now.

Comment 2

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

Attachment: Bug 1633700 - Make `CreateElementTransaction::InsertNewNode()` check the relation between container and child only when the point has child node r=m_kato!

I guess that this crash may occur in these conditions:

1. It pointed end of a node, but the node has fewer children at redo.
2. The storing point has already been reset by cycle collector.

So, anyway, the check should be tested only when the point stores a
child node.

This adds WPT tests for related situation. Although "insertLineBreak" of
Blink and WebKit behave odd when selection is collapsed between 2 elements.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1619914

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06bcc4f14cfa
Make `CreateElementTransaction::InsertNewNode()` check the relation between container and child only when the point has child node r=m_kato

Comment 5

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/23451 for changes under testing/web-platform/tests

Comment 6

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 7

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/06bcc4f14cfa

Comment 8

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 9

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

Comment on attachment 9144958 [details]
Bug 1633700 - Make CreateElementTransaction::InsertNewNode() check the relation between container and child only when the point has child node r=m_kato!

Beta/Release Uplift Approval Request

• User impact if declined: New regression bug since 76. And as the new WPT, this can occur even with normal web apps (meaning even not malicious web apps).
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Although we have odd regression, bug 1636232, but this fix works correctly on usual environment/build. So, I think that it's enough safe to uplift.
• String changes made/needed: no

Comment 10

[Pascal Chevrel:pascalc]

Comment on attachment 9144958 [details]
Bug 1633700 - Make CreateElementTransaction::InsertNewNode() check the relation between container and child only when the point has child node r=m_kato!

It's an extremely low volume of crashes (2 on 76 release, 1 in 77 beta) and there is some logic change with a new test failure in bug 1636232. I think it should ride the trains, thanks.

Comment 11

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

there is some logic change with a new test failure in bug 1636232

This is wrong, it's added by the patch which I requested to uplift.