Bug 172635 – Stop assertion failure & crash when editing selection in contenteditable

• Status: NEW
• Component: New Bugs

Stop assertion failure & crash when editing in contenteditable

Created attachment 311342 [details]
Patch

[Build Bot]

Attachment 311342 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebCore/ChangeLog:20:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 311451 [details]
Patch

[Build Bot]

Comment on attachment 311451 [details]
Patch

Attachment 311451 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3835262

New failing tests:
editing/selection/172635.html

[Build Bot]

Created attachment 311457 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Ryosuke Niwa]

Comment on attachment 311451 [details]
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.

[Ryosuke Niwa]

Comment on attachment 311451 [details]
Patch

There are enough issues with this patch that I don't think it should be landed as is. r-.