Bug 186555 - REGRESSION(r228724): Occasional crash when executing ReplaceSelectionCommand at the end of the document
Summary: REGRESSION(r228724): Occasional crash when executing ReplaceSelectionCommand ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
: 184884 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-06-11 20:29 PDT by Wenson Hsieh
Modified: 2018-06-12 14:13 PDT (History)
9 users (show)

See Also:


Attachments
Patch (5.79 KB, patch)
2018-06-11 20:45 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Minor tweak (5.75 KB, patch)
2018-06-11 20:49 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Fix another typo. (5.74 KB, patch)
2018-06-11 21:05 PDT, Wenson Hsieh
rniwa: review+
Details | Formatted Diff | Diff
Patch for landing (5.89 KB, patch)
2018-06-12 09:46 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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!