Bug 210354

Summary: Infinite loop in InsertListCommand::doApply()
Product: WebKit Reporter: Jack <shihchieh_lee>
Component: HTML EditingAssignee: Jack <shihchieh_lee>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, ews-watchlist, ggaren, mifenton, wenson_hsieh, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch for landing
none
Patch none

Jack
Reported 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.
Attachments
Patch (4.18 KB, patch)
2020-04-11 13:49 PDT, Jack 		no flags
DetailsFormatted DiffDiff
Patch for landing (4.20 KB, patch)
2020-04-11 19:36 PDT, Jack 		no flags
DetailsFormatted DiffDiff
Patch (2.03 KB, patch)
2020-04-12 19:41 PDT, Jack 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Jack
Comment 1 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.
Jack
Comment 2 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"
Jack
Comment 3 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"
Jack
Comment 4 2020-04-11 13:49:20 PDT
Created attachment 396189 [details] Patch
Jack
Comment 5 2020-04-11 19:36:48 PDT
Created attachment 396202 [details] Patch for landing
Jack
Comment 6 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
EWS
Comment 7 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].
zalan
Comment 8 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).
Jack
Comment 9 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).
Jack
Comment 10 2020-04-12 19:41:19 PDT
Reopening to attach new patch.
Jack
Comment 11 2020-04-12 19:41:20 PDT
Created attachment 396249 [details] Patch
Geoffrey Garen
Comment 12 2020-04-15 14:50:30 PDT
Comment on attachment 396249 [details] Patch r=me
EWS
Comment 13 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].
Note You need to log in before you can comment on or make changes to this bug.