Summary: | REGRESSION(r228724): Occasional crash when executing ReplaceSelectionCommand at the end of the document | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||||||
Component: | HTML Editing | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bdakin, commit-queue, Hironori.Fujii, mcatanzaro, realdawei, rniwa, thorton, webkit-bug-importer, wenson_hsieh | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=184884 https://bugs.webkit.org/show_bug.cgi?id=182784 |
||||||||||||
Attachments: |
|
Description
Wenson Hsieh
2018-06-11 20:29:55 PDT
Created attachment 342510 [details]
Patch
Created attachment 342511 [details]
Minor tweak
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> Created attachment 342514 [details]
Fix another typo.
How do you think my patch in Bug 182784? (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(). Wenson, is bug #184884 a duplicate of this issue? (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? Nah, let's do it the other way around. All the action is in this bug. *** Bug 184884 has been marked as a duplicate of this bug. *** (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. 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? Created attachment 342553 [details]
Patch for landing
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? 👍 Comment on attachment 342553 [details] Patch for landing Clearing flags on attachment: 342553 Committed r232757: <https://trac.webkit.org/changeset/232757> 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 ====== (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. (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! |