1417492 - Crash in mozilla::HTMLEditRules::TryToJoinBlocks

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is
report bp-0e9d9fe9-893b-42a7-a3a1-f0c1e0171114.
=============================================================

Crash seen in nightly, but also 58: http://bit.ly/2ms0OCa. Crashes in 58 started using 20171108110838.

Bug 1414713 landed in that timeframe - perhaps :masayuki can offer some insight. 

Top 10 frames of crashing thread:

0 xul.dll mozilla::HTMLEditRules::TryToJoinBlocks editor/libeditor/HTMLEditRules.cpp:2980
1 xul.dll mozilla::HTMLEditRules::WillDeleteSelection editor/libeditor/HTMLEditRules.cpp:2448
2 xul.dll mozilla::HTMLEditRules::WillDoAction editor/libeditor/HTMLEditRules.cpp:653
3 xul.dll mozilla::TextEditor::DeleteSelection editor/libeditor/TextEditor.cpp:663
4 xul.dll mozilla::EditorBase::HandleKeyPressEvent editor/libeditor/EditorBase.cpp:4912
5 xul.dll mozilla::HTMLEditor::HandleKeyPressEvent editor/libeditor/HTMLEditor.cpp:595
6 xul.dll mozilla::EditorEventListener::KeyPress editor/libeditor/EditorEventListener.cpp:608
7 xul.dll mozilla::EditorEventListener::HandleEvent editor/libeditor/EditorEventListener.cpp:418
8 xul.dll mozilla::EventListenerManager::HandleEventSubType dom/events/EventListenerManager.cpp:1118
9 xul.dll mozilla::EventListenerManager::HandleEventInternal dom/events/EventListenerManager.cpp:1293

=============================================================

Comment 1

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

Yeah, must be a regression of my work.

Comment 2

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

The assertion tells us an existing bug.

The bug is a regression of bug 1414713.

Comment 3

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0530f64079fb4c06c3e3d785b840130cd3fb3227

Comment 4

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

Hmm, that's not a regression of bug 1392867. The comment in source code said that all list items in right list node should be moved to the left list node.  However, that has never done that actually at least since 2002...
https://searchfox.org/mozilla-central/rev/c187e71ad56f59a17262fe1007145fa1082d5967/editor/libeditor/html/nsHTMLEditRules.cpp#2578,2590-2591,2595,2624-2625,2630

Comment 5

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=abd20774f00222d23967b985261d098e8a60c67d

Comment 6

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

Attachment: Bug 1417492 - HTMLEditRules::TryToJoinBlocks() should no do nothing when left list node and right list node are not descendant of each other

We hit assertion which were added by bug 1414713.  That tells us an ancient bug.

There is comment which claims that we should move all list items in the right
list node to left list node if the list nodes should be merged.  However, this
has never been done actually since 2002.  The code tried to move *some* list
items in the right list node to the list list node.  However, retrieving first
list item at an offset almost always failed because the offset variable has
never been initialized.

If we believe the comment, we should move all children of the right list node
to the left list node.  However, until we get a testcase to reach this case,
we should keep current behavior, i.e., do nothing, for unexpected regression.

Review commit: https://reviewboard.mozilla.org/r/201004/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201004/

Comment 7

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

If we know how to use this path, we can fix this bug correctly, though. Reaching the block is really complicated with current code...

Comment 8

[Makoto Kato [:m_kato]]

Comment on attachment 8929801 [details]
Bug 1417492 - HTMLEditRules::TryToJoinBlocks() should no do nothing when left list node and right list node are not descendant of each other

https://reviewboard.mozilla.org/r/201004/#review206626

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6faac9d0daff
HTMLEditRules::TryToJoinBlocks() should no do nothing when left list node and right list node are not descendant of each other r=m_kato

Comment 10

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/6faac9d0daff

Comment 11

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

Comment on attachment 8929801 [details]
Bug 1417492 - HTMLEditRules::TryToJoinBlocks() should no do nothing when left list node and right list node are not descendant of each other

Approval Request Comment
[Feature/Bug causing the regression]:
Regression of bug 1414713.

[User impact if declined]:
Nightly and Developer edition users may hit this MOZ_DIAGNOSTIC_ASSERT which was added for confirming we have dead code (if it's not dead code, nobody hits this assert).

[Is this code covered by automated tests?]:
No, we're still not sure how to reproduce this.

[Has the fix been verified in Nightly?]:
Not yet because we don't know how to reproduce this. However, this patch removes the dead code and MOZ_DIAGNOSTIC_ASSERT, so, this crash won't occur.

[Needs manual test from QE? If yes, steps to reproduce]:
No.

[List of other uplifts needed for the feature/fix]:
No.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
Because this removes a for loop which is added in 2002 but haven't run due to almost impossible to make the continuation condition true due to using uninitialized variable. And just remove the MOZ_DIAGNOSTIC_ASSERT too.

[String changes made/needed]:
No.

Comment 12

[Gerry Chang [:gchang]]

Comment on attachment 8929801 [details]
Bug 1417492 - HTMLEditRules::TryToJoinBlocks() should no do nothing when left list node and right list node are not descendant of each other

Fix a regression of bug 1414713. Beta58+.

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/3412b2ad3c3d