[Webkit-unassigned] [Bug 172635] Stop assertion failure & crash when editing selection in contenteditable

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=172635

--- Comment #6 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 311451
  --> https://bugs.webkit.org/attachment.cgi?id=311451
Patch

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

It's not clear to me that this patch is either applicable or correct for WebKit.

Do you actually understand the code change? Or are you simply copying the code change from Chromium?
WebKit and Chromium have sufficiently diverged in this area that we shouldn't be merging patches from one another without fully understanding what's happening in our codebase.

> LayoutTests/ChangeLog:13
> +        certain circumstances. This is because Squire traps the delete key and
> +        manually manipulates selections. This means that if you select text in
> +        the middle of the contenteditable content and delete it via the delete
> +        key, the assertion in the updatePositionAfterAdoptingTextReplacement
> +        method is fired.
> +
> +        A similar issue was reported in Chromium ( https://bugs.chromium.org/p/chromium/issues/detail?id=383777 ).
> +        Their solution is to simply clamp the length of the selection so that is always in a valid range. This is the same approach, but using the equivalent WebKit classes as Chromium have heavily modified this area.

There's no point in repeating the same description for the layout test change log.
You should simply mention that you're adding a regression test imported from Chromium.

> LayoutTests/ChangeLog:18
> +        Stop assertion failure & crash when editing selection in contenteditable
> +        https://bugs.webkit.org/show_bug.cgi?id=172635
> +
> +        Reviewed by NOBODY (OOPS!).

These lines should appear before the long description.

> Source/WebCore/ChangeLog:10
> +        When using a content editor based on Squire, WebKit can crash under
> +        certain circumstances. This is because Squire traps the delete key and
> +        manually manipulates selections. This means that if you select text in
> +        the middle of the contenteditable content and delete it via the delete
> +        key, the assertion in the updatePositionAfterAdoptingTextReplacement
> +        method is fired.

The first assertion I hit is:
ASSERTION FAILED: m_offset < m_text2->length()
/Volumes/Data/webkit/Source/WebCore/editing/SplitTextNodeCommand.cpp(47) : WebCore::SplitTextNodeCommand::SplitTextNodeCommand(Ref<WebCore::Text> &&, int)
1   0x11faae8fd WTFCrash
2   0x114d5fee7 WebCore::SplitTextNodeCommand::SplitTextNodeCommand(WTF::Ref<WebCore::Text>&&, int)
3   0x114d5ff23 WebCore::SplitTextNodeCommand::SplitTextNodeCommand(WTF::Ref<WebCore::Text>&&, int)
4   0x11287dfb6 WebCore::SplitTextNodeCommand::create(WTF::Ref<WebCore::Text>&&, int)
5   0x11287d4fb WebCore::CompositeEditCommand::splitTextNode(WebCore::Text&, unsigned int)
6   0x112606ef2 WebCore::ApplyStyleCommand::splitTextAtEnd(WebCore::Position const&, WebCore::Position const&)
7   0x112605a84 WebCore::ApplyStyleCommand::applyInlineStyle(WebCore::EditingStyle&)
8   0x112603efa WebCore::ApplyStyleCommand::doApply()
9   0x11287c20f WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::EditCommand>&&)
10  0x11471e9b6 WebCore::RemoveFormatCommand::doApply()
11  0x11287ba8a WebCore::CompositeEditCommand::apply()
12  0x112dcf392 WebCore::Editor::removeFormattingAndStyle()
13  0x112dec698 WebCore::executeRemoveFormat(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&)

> Source/WebCore/ChangeLog:13
> +        A similar issue was reported in Chromium ( https://bugs.chromium.org/p/chromium/issues/detail?id=383777 ).
> +        Their solution is to simply clamp the length of the selection so that is always in a valid range. This is the same approach, but using the equivalent WebKit classes as Chromium have heavily modified this area.

Why are these two lines much longer than the previous paragraph?
Please wrap lines earlier to match the previous paragraph.

> Source/WebCore/ChangeLog:18
> +        Stop assertion failure & crash when editing selection in contenteditable
> +        https://bugs.webkit.org/show_bug.cgi?id=172635
> +
> +        Reviewed by NOBODY (OOPS!).

Ditto about wrong ordering.

> Source/WebCore/ChangeLog:21
> +        Test: editing/selection/172635.html
> +        Copied from the chrome reproduction bug.

Wrong format. It should be "Tests:"
The comment about this being copied from Chromium should be mentioned in the change log entry for the layout test.

> Source/WebCore/editing/FrameSelection.cpp:523
> +    // Basically copy the reasoning from Chrome, although this is a slightly different
> +    // cause, see: https://bugs.chromium.org/p/chromium/issues/detail?id=383777

This comment doesn't belong in the code.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170529/e844622b/attachment-0001.html>