Bug 210354

Summary:

Infinite loop in InsertListCommand::doApply()

Product:

WebKit

Reporter:

Jack <shihchieh_lee>

Component:

HTML Editing

Assignee:

Jack <shihchieh_lee>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

darin, ews-watchlist, ggaren, mifenton, wenson_hsieh, zalan

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch for landing	none
Patch	none

Description Jack 2020-04-10 13:46:35 PDT

<rdar://61427778> Infinite loop in InsertListCommand::doApply()

The condition check, !inSameParagraph(), in the while loop is always satisfied.

<script>
    window.onload = () => {
        window.getSelection().setBaseAndExtent(TH,1,SPAN,0);
        document.execCommand("insertUnorderedList", false);
    }
</script>
<body contenteditable="true"><table><select></select><th id=TH>a</th><sapn id=SPAN></span>

1. The code is inserting an UL and going through each paragraph in the selected range.
2. After <select> is putting into a LI element, we try to find the next paragraph by calling startOfNextParagraph(endingSelection().visibleStart()), which indirectly calls nextVisuallyDistinctCandidate.
3. However, because table is now in <body>, function nextVisuallyDistinctCandidate cannot find next candidate and return null position.
4. As a result, the next paragraph becomes empty, and is assinged to variable startOfCurrentParagraph
5. Function inSameParagraph() always returns false with input of empty startOfCurrentParagraph.
6. The empty startOfNextParagraph is assigned to endingSelection.
7. The empty ending selection is again used to find the next paragraph by calling  startOfNextParagraph again as in step #1.
8. startOfNextParagraph returns empty position and assign it to startOfCurrentParagraph and start looping to step #5.

Comment 1 Jack 2020-04-10 14:13:02 PDT

There are two ways to patch this issue. One is to skip the while loop only and finishes the rest of the code. The other is to exit this function immediately.

The decision depends on whether an error occurs, such as some contents being deleted. Two immediate returns are found in the while loop.

However, in this particular test case, startOfNextParagraph returns empty location for a valid reason (no visually distinct candidate), so it is treated as no new paragraph is found, and we can just break out of the while loop.

Comment 2 Jack 2020-04-10 17:38:54 PDT

Node tree if we allow code to finish:

*#document	0x61f000031080 (renderer 0x61700003bc00) 
	HTML	0x60c000082f00 (renderer 0x612000081940) 
		HEAD	0x60c000082fc0 (renderer 0x0) 
			SCRIPT	0x61000002d440 (renderer 0x0) 
				#text	0x60800004f6a0 "\n    window.onload = () => {\n        window.getSelection().setBaseAndExtent(TH,1,SPAN,0);\n        document.execCommand("insertUnorderedList", false);\n    }\n"
			#text	0x60800004f7a0 "\n"
		BODY	0x60c000083b00 (renderer 0x612000081ac0) 
			UL	0x60c0000955c0 (renderer 0x6120000841c0) 
				LI	0x60c0000952c0 (renderer 0x612000084340) 
					SELECT	0x61300006d9c0 (renderer 0x615000089600) 
			TABLE	0x60e0000597c0 (renderer 0x61400003aa40) 
				TBODY	0x60c000083bc0 (renderer 0x61300006a000) 
					TR	0x60c000083c80 (renderer 0x6110000d1500) 
						TH	0x60c000083d40 (renderer 0x612000081f40) 
							UL	0x60c000027640 (renderer 0x612000085840) 
								LI	0x60c0000274c0 (renderer 0x612000085b40) 
									#text	0x608000019920 "a"

Comment 3 Jack 2020-04-10 17:44:06 PDT

If we return prematurely when startOfNextParagraph returns empty position, the text node is not inserted in an UL.

BODY	0x60c000083500 (renderer 0x612000081ac0) 
	UL	0x60c000094fc0 (renderer 0x6120000841c0) 
		LI	0x60c000094cc0 (renderer 0x612000084340) 
			SELECT	0x61300006d9c0 (renderer 0x615000089380) 
	TABLE	0x60e0000596e0 (renderer 0x61400003aa40) 
		TBODY	0x60c0000835c0 (renderer 0x61300006a000) 
			TR	0x60c000083680 (renderer 0x6110000d1500) 
				TH	0x60c000083740 (renderer 0x612000081f40) 
*					#text	0x608000057820 "a"

Comment 4 Jack 2020-04-11 13:49:20 PDT

Created attachment 396189 [details]
Patch

Comment 5 Jack 2020-04-11 19:36:48 PDT

Created attachment 396202 [details]
Patch for landing

Comment 6 Jack 2020-04-11 19:42:46 PDT

Darin, thanks for reviewing the patch. I found that the minimized test case in the first patch doesn't reproduce on ToT so I uploaded the more complicated one as described in analysis.

(In reply to Jack from comment #5)
> Created attachment 396202 [details]
> Patch for landing

Comment 7 EWS 2020-04-11 20:13:23 PDT

Committed r259939: <https://trac.webkit.org/changeset/259939>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396202 [details].

Comment 8 zalan 2020-04-12 12:57:35 PDT

When constructing a test reduction, I usually check the render tree state right before when the bug occurs and reconstruct the markup accordingly. The normalization process (invalid content inside <table> etc) produces a tree that matches the following markup (notice how different the markup is).
 
<body contenteditable="true"><input id=input><table><td id=td>content</td></table></body>
<script>
document.body.offsetHeight;
window.getSelection().setBaseAndExtent(td, 1, input, 0);
document.execCommand("insertUnorderedList", false);
document.body.offsetHeight;
document.body.innerText = "Tests inserting list at the end of a table. The test passes if WebKit doesn't crash or hit an assertion.";
if (window.testRunner)
  testRunner.dumpAsText();
</script>

Also, it's a good practice (slightly faster and less error-prone) to force layout in an inline script right after the body instead of the onload event (and the rAF in this test case was just a way to jump a run loop and have the content laid out which can also be replaced with a forced layout).

Comment 9 Jack 2020-04-12 19:10:48 PDT

Alan, thanks for the tips. I tried to simplify the tests as you mentioned by looking at the node tree, but didn't know enough to really make a good one.

It's great to have this as a template, but in the future I might still need your help on creating reduced test case.

I will submit a patch shortly. Thanks!

(In reply to zalan from comment #8)
> When constructing a test reduction, I usually check the render tree state
> right before when the bug occurs and reconstruct the markup accordingly. The
> normalization process (invalid content inside <table> etc) produces a tree
> that matches the following markup (notice how different the markup is).

Comment 10 Jack 2020-04-12 19:41:19 PDT

Reopening to attach new patch.

Comment 11 Jack 2020-04-12 19:41:20 PDT

Created attachment 396249 [details]
Patch

Comment 12 Geoffrey Garen 2020-04-15 14:50:30 PDT

Comment on attachment 396249 [details]
Patch

r=me

Comment 13 EWS 2020-04-15 15:01:57 PDT

Committed r260154: <https://trac.webkit.org/changeset/260154>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396249 [details].