1411345 - stack-overflow in [@ mozilla::HTMLEditRules::WillDeleteSelection]

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.html

The stack is this frame repeated... mozilla::HTMLEditRules::WillDeleteSelection(mozilla::dom::Selection*, short, short, bool*, bool*) /builds/worker/workspace/build/src/editor/libeditor/HTMLEditRules.cpp:2321:16

Comment 1

[Ryan VanderMeulen [:RyanVM]]

INFO: Last good revision: f5c43a9f8510ad50cf45248fe306707aa059b991
INFO: First bad revision: e05f84ea2a338e172a55a0898e3551fff61abb0a
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=f5c43a9f8510ad50cf45248fe306707aa059b991&tochange=e05f84ea2a338e172a55a0898e3551fff61abb0a

Comment 2

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

Stack trace is here:
https://crash-stats.mozilla.com/report/index/cf0039f8-c5c4-4359-95f9-6e1bb0171027

Comment 3

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

TryToJoinBlocks() doesn't return the result of MoveBlock():
https://searchfox.org/mozilla-central/rev/40e8eb46609dcb8780764774ec550afff1eed3a5/editor/libeditor/HTMLEditRules.cpp#2841-2846,2851

Then, WillDeleteSelection() believes that it does nothing. Then, it retries to delete selection again with recursive call and this causes the infinite loop.

Comment 4

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

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

Comment 5

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

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

Comment 6

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

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

Comment 7

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

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

Comment 8

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

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

Comment 9

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

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

Comment 10

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

Attachment: Bug 1411345 - HTMLEditRules::GetHighestInlineParent() shouldn't return editing host even when it's the highest inline parent of aNode

HTMLEditRules::BustUpInlinesAtRangeEndpoints() tries to split all inline nodes
at range start and range end.  It uses EditorBase::SplitNodeDeep() to split
the nodes and HTMLEditRules::GetHighestInlineParent() to retrieve the highest
inline parent of them.

Currently, HTMLEditRules::GetHighestInlineParent() may return editing host or
ancestor of it if active editing host is not block.  Then, it may cause
splitting editing host or its parents and following methods of HTMLEditRules
will fail to modify the nodes created outside the editing host.

So, HTMLEditRules::GetHighestInlineParent() should return only one of the
descendants of active editing host.

Unfortunately, even if just adding the test case as a crash test, I cannot
reproduce the crash with automated tests.  Therefore, this patch doesn't
include any automated tests.

And this patch changes a crash test, 1402196.html, which expects that an inline
editing host is split by execCommand("insertOrderedList").  However, this patch
fixes this wrong behavior.  Therefore, this patch changes the event target of
event listener from <p> inside the editing host to the editing host itself.

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

Comment 11

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

Although, the patch does NOT fix direct cause of the crash. I.e., not fixing the recursive call. However, such unexpected recursive calls are a bug of other methods which WillDeleteSelection() doesn't check the result strictly. So, this fix must be okay for now.

(The root cause of this bug is, SplitNodeDeep() splits editing host whose style is inline unexpectedly.  Then, another editing host cannot be joined to expected node since they are in different editing host.)

Comment 12

[Makoto Kato [:m_kato]]

Comment on attachment 8923875 [details]
Bug 1411345 - HTMLEditRules::GetHighestInlineParent() shouldn't return editing host even when it's the highest inline parent of aNode

https://reviewboard.mozilla.org/r/195008/#review200350

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dd95e6f8c0b6
HTMLEditRules::GetHighestInlineParent() shouldn't return editing host even when it's the highest inline parent of aNode r=m_kato

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/dd95e6f8c0b6