1408170 - 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 repeated... mozilla::HTMLEditRules::WillDeleteSelection(mozilla::dom::Selection*, short, short, bool*, bool*) /builds/worker/workspace/build/src/editor/libeditor/HTMLEditRules.cpp:2321:16

Comment 1

[Tyson Smith [:tsmith]]

Attachment: prefs.js

Comment 2

[Ryan VanderMeulen [:RyanVM]]

INFO: Last good revision: a2bc0214fbcaf6104fe2ae8d2f4843997ea283de
INFO: First bad revision: e22de36807c6229b3710382f0094ffe5e31eaa26
INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a2bc0214fbcaf6104fe2ae8d2f4843997ea283de&tochange=e22de36807c6229b3710382f0094ffe5e31eaa26

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Ehsan isn't accepting NI requests, so maybe Masayuki can take a look.

Comment 4

[(no longer active)]

I have a fix.  I need to figure out what set of prefs the test case depends on...

Comment 5

[(no longer active)]

Hmm, the test doesn't depend on a pref, but doesn't reproduce the crash when loaded from https for some reason.  At any rate, when loaded as a crashtest it does reproduce the crash which is good from a regression testing perspective.

Comment 6

[(no longer active)]

Attachment: Set child correctly in HTMLEditRules::GetPromotedPoint()

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Comment on attachment 8918117 [details] [diff] [review]
Set child correctly in HTMLEditRules::GetPromotedPoint()

>@@ -5568,17 +5568,17 @@ HTMLEditRules::GetPromotedPoint(RulesEndpoint aWhere,
>     // from us, and that are enclosed in the same block.
>     nsCOMPtr<nsINode> priorNode =
>       htmlEditor->GetPriorHTMLNode(node, offset, child, true);
> 
>     while (priorNode && priorNode->GetParentNode() &&
>            !htmlEditor->IsVisibleBRElement(priorNode) &&
>            !IsBlockNode(*priorNode)) {
>       offset = priorNode->GetParentNode()->IndexOf(priorNode);
>-      child = node;
>+      child = priorNode;

Oh, it really mismatches with the offset. Sorry for not catching it at review.

>diff --git a/editor/libeditor/crashtests/1408170.html b/editor/libeditor/crashtests/1408170.html
>@@ -0,0 +1,20 @@
>+<script>
>+function jsfuzzer() {
>+  try { document.execCommand("insertUnorderedList", false); } catch(e) { }
>+  try { document.execCommand("delete", false); } catch(e) { }
>+}
>+function eventhandler1() {
>+  try { window.getSelection().collapse(htmlvar00001,1); } catch(e) { }

Oh, don't we need to do |document.getElementById("htmlvar0001")|?

>+}
>+function eventhandler2() {
>+  try { htmlvar00002.appendChild(htmlvar00001); } catch(e) { }

And same here for htmlvar0002 and htmlvar0001?

Comment 8

[(no longer active)]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #7)
> >diff --git a/editor/libeditor/crashtests/1408170.html b/editor/libeditor/crashtests/1408170.html
> >@@ -0,0 +1,20 @@
> >+<script>
> >+function jsfuzzer() {
> >+  try { document.execCommand("insertUnorderedList", false); } catch(e) { }
> >+  try { document.execCommand("delete", false); } catch(e) { }
> >+}
> >+function eventhandler1() {
> >+  try { window.getSelection().collapse(htmlvar00001,1); } catch(e) { }
> 
> Oh, don't we need to do |document.getElementById("htmlvar0001")|?
> 
> >+}
> >+function eventhandler2() {
> >+  try { htmlvar00002.appendChild(htmlvar00001); } catch(e) { }
> 
> And same here for htmlvar0002 and htmlvar0001?

These will just throw.  This is a fuzzer generated test case, so not everything in the test makes sense.  :-)  In this case, the bug was triggered by the execCommand("delete") call...

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4baaea004689
Set child correctly in HTMLEditRules::GetPromotedPoint(); r=masayuki

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/4baaea004689