Bug 186555

Summary: REGRESSION(r228724): Occasional crash when executing ReplaceSelectionCommand at the end of the document
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: HTML EditingAssignee: 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 Flags
Patch
none
Minor tweak
none
Fix another typo.
rniwa: review+
Patch for landing none

Description Wenson Hsieh 2018-06-11 20:29:55 PDT
<rdar://problem/39703004>
Comment 1 Wenson Hsieh 2018-06-11 20:45:11 PDT
Created attachment 342510 [details]
Patch
Comment 2 Wenson Hsieh 2018-06-11 20:49:56 PDT
Created attachment 342511 [details]
Minor tweak
Comment 3 Wenson Hsieh 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>
Comment 4 Wenson Hsieh 2018-06-11 21:05:50 PDT
Created attachment 342514 [details]
Fix another typo.
Comment 5 Fujii Hironori 2018-06-11 22:18:23 PDT
How do you think my patch in Bug 182784?
Comment 6 Wenson Hsieh 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().
Comment 7 Michael Catanzaro 2018-06-12 08:37:15 PDT
Wenson, is bug #184884 a duplicate of this issue?
Comment 8 Wenson Hsieh 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?
Comment 9 Michael Catanzaro 2018-06-12 08:52:10 PDT
Nah, let's do it the other way around. All the action is in this bug.
Comment 10 Michael Catanzaro 2018-06-12 08:52:21 PDT
*** Bug 184884 has been marked as a duplicate of this bug. ***
Comment 11 Michael Catanzaro 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.
Comment 12 Ryosuke Niwa 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?
Comment 13 Wenson Hsieh 2018-06-12 09:46:07 PDT
Created attachment 342553 [details]
Patch for landing
Comment 14 Wenson Hsieh 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?

👍
Comment 15 WebKit Commit Bot 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>
Comment 16 Dawei Fenton (:realdawei) 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 ======
Comment 17 Wenson Hsieh 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.
Comment 18 Dawei Fenton (:realdawei) 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!