Bug 226398 - Crash in HTMLConverter::_addLinkForElement()
Summary: Crash in HTMLConverter::_addLinkForElement()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified macOS 11
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-28 14:44 PDT by Julian Gonzalez
Modified: 2021-06-03 01:42 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.74 KB, patch)
2021-05-28 14:49 PDT, Julian Gonzalez
no flags Details | Formatted Diff | Diff
Patch (4.10 KB, patch)
2021-06-02 15:29 PDT, Julian Gonzalez
no flags Details | Formatted Diff | Diff
Patch (3.58 KB, patch)
2021-06-02 19:15 PDT, Julian Gonzalez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Gonzalez 2021-05-28 14:44:44 PDT
e.g.

0 HTMLConverter::_addLinkForElement(WebCore::Element&, _NSRange)+1430
1 HTMLConverter::_exitElement(WebCore::Element&, long, unsigned long)+391
2 HTMLConverter::_traverseNode(WebCore::Node&, unsigned int, bool)+850
3 HTMLConverter::_traverseNode(WebCore::Node&, unsigned int, bool)+1166
4 HTMLConverter::_traverseNode(WebCore::Node&, unsigned int, bool)+1166
5 HTMLConverter::convert()+362
6 WebCore::attributedString(WebCore::SimpleRange const&)+275
7 WebCore::selectionAsAttributedString(WebCore::Document const&)+286
8 WebCore::Editor::writeSelectionToPasteboard(WebCore::Pasteboard&)+334
9 WebCore::Editor::performCutOrCopy(WebCore::Editor::EditorActionSpecifier)+1402

<rdar://problem/78216316>
Comment 1 Julian Gonzalez 2021-05-28 14:49:35 PDT
Created attachment 430058 [details]
Patch
Comment 2 Ryosuke Niwa 2021-05-29 11:28:13 PDT
Comment on attachment 430058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430058&action=review

> Source/WebCore/editing/cocoa/HTMLConverter.mm:2219
> -                _exitElement(element, depth, startIndex);
> +                if (startIndex <= [_attrStr length])

It seems that we don't even want to call _processElement when this condition isn't met?
Comment 3 Julian Gonzalez 2021-06-01 11:16:42 PDT
(In reply to Ryosuke Niwa from comment #2)
> Comment on attachment 430058 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430058&action=review
> 
> > Source/WebCore/editing/cocoa/HTMLConverter.mm:2219
> > -                _exitElement(element, depth, startIndex);
> > +                if (startIndex <= [_attrStr length])
> 
> It seems that we don't even want to call _processElement when this condition
> isn't met?

When the call to _processElement happens, startIndex is within the bounds of [_attrStr length]. The operations inside the else block right above here cause this condition to be violated (this code assuming that [_attrStr length] does not change is the bug).
Comment 4 Ryosuke Niwa 2021-06-02 02:04:06 PDT
Comment on attachment 430058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430058&action=review

>>> Source/WebCore/editing/cocoa/HTMLConverter.mm:2219
>>> +                if (startIndex <= [_attrStr length])
>> 
>> It seems that we don't even want to call _processElement when this condition isn't met?
> 
> When the call to _processElement happens, startIndex is within the bounds of [_attrStr length]. The operations inside the else block right above here cause this condition to be violated (this code assuming that [_attrStr length] does not change is the bug).

I see. In that case, I think we just need to adjust startIndex instead of avoid calling _exitElement.

> LayoutTests/editing/pasteboard/cut-paste-mouse-event.html:9
> +function styleonload() {

Can we call this runTest or something?

> LayoutTests/editing/pasteboard/cut-paste-mouse-event.html:12
> +    document.createEvent("MouseEvent").initMouseEvent("1", document.execCommand("paste", false), 0, null, 0, document.execCommand("cut", false));

Surely creating MouseEvent is nothing to do with this crash?
As far as I just tested, this should work:
document.execCommand("selectAll", false);
document.execCommand("createLink", false, "test");
document.execCommand("cut", false);
Comment 5 Julian Gonzalez 2021-06-02 15:29:07 PDT
Created attachment 430412 [details]
Patch
Comment 6 Ryosuke Niwa 2021-06-02 16:11:42 PDT
Comment on attachment 430412 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430412&action=review

> Source/WebCore/editing/cocoa/HTMLConverter.mm:-2201
> -            NSUInteger startIndex = [_attrStr length];

Oh neat.
Comment 7 Ryosuke Niwa 2021-06-02 17:41:17 PDT
WK1 failures looks real:
--- /Volumes/Data/worker/macOS-Catalina-Release-WK1-Tests-EWS/build/layout-test-results/editing/mac/attributed-string/anchor-element-expected.txt
+++ /Volumes/Data/worker/macOS-Catalina-Release-WK1-Tests-EWS/build/layout-test-results/editing/mac/attributed-string/anchor-element-actual.txt
@@ -27,7 +27,6 @@
     NSColor: #0000ee (sRGB)
     NSFont: Times-Roman 16.00 pt.
     NSKern: 0pt
-    NSLink: https://webkit.org/
     NSStrokeColor: #0000ee (sRGB)
     NSStrokeWidth: 0
     NSUnderline: true
Comment 8 Ryosuke Niwa 2021-06-02 17:44:49 PDT
Comment on attachment 430412 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430412&action=review

> Source/WebCore/editing/cocoa/HTMLConverter.mm:2218
> -                _exitElement(element, depth, startIndex);
> +                _exitElement(element, depth, [_attrStr length]);

Oh, this isn't quite right. startIndex is named so because we're adding more stuff to _attrStr.
What's wrong with the existing code is that startIndex can be less than [_attrStr length].
So I think we need to use std::min(startIndex, [_attrStr length]) here.
Comment 9 Ryosuke Niwa 2021-06-02 17:45:09 PDT
Comment on attachment 430412 [details]
Patch

r- because of the aforementioned WK1 test failures.
Comment 10 Julian Gonzalez 2021-06-02 19:15:21 PDT
Created attachment 430434 [details]
Patch
Comment 11 Ryosuke Niwa 2021-06-02 20:51:51 PDT
Let's wait for other EWSs to catch up.
Comment 12 EWS 2021-06-03 00:09:36 PDT
Committed r278398 (238423@main): <https://commits.webkit.org/238423@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430434 [details].