RESOLVED FIXED 186555
REGRESSION(r228724): Occasional crash when executing ReplaceSelectionCommand at the end of the document 
https://bugs.webkit.org/show_bug.cgi?id=186555
Summary REGRESSION(r228724): Occasional crash when executing ReplaceSelectionCommand ...
Wenson Hsieh
Reported 2018-06-11 20:29:55 PDT
<rdar://problem/39703004>
Attachments
Patch (5.79 KB, patch)
2018-06-11 20:45 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Minor tweak (5.75 KB, patch)
2018-06-11 20:49 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Fix another typo. (5.74 KB, patch)
2018-06-11 21:05 PDT, Wenson Hsieh 		rniwa: review+
DetailsFormatted DiffDiff
Patch for landing (5.89 KB, patch)
2018-06-12 09:46 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Wenson Hsieh
Comment 1 2018-06-11 20:45:11 PDT
Created attachment 342510 [details] Patch
Wenson Hsieh
Comment 2 2018-06-11 20:49:56 PDT
Created attachment 342511 [details] Minor tweak
Wenson Hsieh
Comment 3 2018-06-11 20:54:18 PDT
Example test case: <script> onload = () => { getSelection().setPosition(document.body, 1); document.execCommand("InsertHTML", false, `ptr<span class="Apple-style-span"></span>`); }; </script> <body contenteditable autofocus><span>null</span></body>
Wenson Hsieh
Comment 4 2018-06-11 21:05:50 PDT
Created attachment 342514 [details] Fix another typo.
Fujii Hironori
Comment 5 2018-06-11 22:18:23 PDT
How do you think my patch in Bug 182784?
Wenson Hsieh
Comment 6 2018-06-12 07:23:19 PDT
(In reply to Fujii Hironori from comment #5) > How do you think my patch in Bug 182784? Your patch in 182784 looks reasonable, but I suspect there's a corner case where that change might end up setting m_lastNodeInserted to a node that's before m_firstNodeInserted in the document. Changing the logic to prefer the previous node rather than the next node also seems somewhat risky... The patch in this bug (186555) is just for addressing the nullptr deref uncovered after removing the null check in InsertedNodes::pastLastLeaf().
Michael Catanzaro
Comment 7 2018-06-12 08:37:15 PDT
Wenson, is bug #184884 a duplicate of this issue?
Wenson Hsieh
Comment 8 2018-06-12 08:46:25 PDT
(In reply to Michael Catanzaro from comment #7) > Wenson, is bug #184884 a duplicate of this issue? Ah, that seems like the same issue! In both bugs, the first node inserted is non-null, but the last node inserted is null, which causes a nullptr deref when executing ReplaceSelectionCommand. Technically, in this case, the null deref is happening under pastLastLeaf() while in the other bug it's under lastLeafInserted(), but this might just be due to differences in UB when attempting to dereference null. Do you think I should close this as a dupe and post my patch/test case in Bug #184884?
Michael Catanzaro
Comment 9 2018-06-12 08:52:10 PDT
Nah, let's do it the other way around. All the action is in this bug.
Michael Catanzaro
Comment 10 2018-06-12 08:52:21 PDT
*** Bug 184884 has been marked as a duplicate of this bug. ***
Michael Catanzaro
Comment 11 2018-06-12 08:52:51 PDT
(In reply to Michael Catanzaro from comment #10) > *** Bug 184884 has been marked as a duplicate of this bug. *** Note: this bug has good backtraces.
Ryosuke Niwa
Comment 12 2018-06-12 09:11:16 PDT
Comment on attachment 342514 [details] Fix another typo. View in context: https://bugs.webkit.org/attachment.cgi?id=342514&action=review > LayoutTests/editing/execCommand/insert-apple-style-span-at-document-end.html:9 > + if (window.testRunner) > + testRunner.dumpAsText(); Can we dump the markup using dump-as-markup here?
Wenson Hsieh
Comment 13 2018-06-12 09:46:07 PDT
Created attachment 342553 [details] Patch for landing
Wenson Hsieh
Comment 14 2018-06-12 09:46:47 PDT
Comment on attachment 342514 [details] Fix another typo. View in context: https://bugs.webkit.org/attachment.cgi?id=342514&action=review >> LayoutTests/editing/execCommand/insert-apple-style-span-at-document-end.html:9 >> + testRunner.dumpAsText(); > > Can we dump the markup using dump-as-markup here? 👍
WebKit Commit Bot
Comment 15 2018-06-12 10:25:40 PDT
Comment on attachment 342553 [details] Patch for landing Clearing flags on attachment: 342553 Committed r232757: <https://trac.webkit.org/changeset/232757>
Dawei Fenton (:realdawei)
Comment 16 2018-06-12 13:49:22 PDT
Hi Wenson, WebKit is failing to build on Windows after r232757 Here's a sampler error: https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/9946/steps/compile-webkit/logs/stdio C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\editing/ReplaceSelectionCommand.cpp(349): error C2059: syntax error: ':' [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj] ===== BUILD FAILED ======
Wenson Hsieh
Comment 17 2018-06-12 14:11:06 PDT
(In reply to David Fenton from comment #16) > Hi Wenson, WebKit is failing to build on Windows after r232757 > > Here's a sampler error: > > https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/ > 9946/steps/compile-webkit/logs/stdio > > C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\editing/ > ReplaceSelectionCommand.cpp(349): error C2059: syntax error: ':' > [C:\cygwin\home\buildbot\slave\win- > release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj] > > > ===== BUILD FAILED ====== Thanks for spotting this! <https://trac.webkit.org/changeset/232773> should fix it.
Dawei Fenton (:realdawei)
Comment 18 2018-06-12 14:13:23 PDT
(In reply to Wenson Hsieh from comment #17) > (In reply to David Fenton from comment #16) > > Hi Wenson, WebKit is failing to build on Windows after r232757 > > > > Here's a sampler error: > > > > https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/ > > 9946/steps/compile-webkit/logs/stdio > > > > C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\editing/ > > ReplaceSelectionCommand.cpp(349): error C2059: syntax error: ':' > > [C:\cygwin\home\buildbot\slave\win- > > release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj] > > > > > > ===== BUILD FAILED ====== > > Thanks for spotting this! <https://trac.webkit.org/changeset/232773> should > fix it. Great thanks!
Note You need to log in before you can comment on or make changes to this bug.