Bug 111228 – InsertUnorderedList can lead to lost content and assertions in moveParagraphs

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Levi Weintraub]

Created attachment 191055 [details]
Test case from crbug.com

ASSERTION FAILED: destination.deepEquivalent().anchorNode()->inDocument()
../../third_party/WebKit/Source/WebCore/editing/CompositeEditCommand.cpp(1220) : void WebCore::CompositeEditCommand::moveParagraphs(const WebCore::VisiblePosition&, const WebCore::V
isiblePosition&, const WebCore::VisiblePosition&, bool, bool)

#0  0x00000000025d490c in WebCore::CompositeEditCommand::moveParagraphs (this=0x25bab8595b60, startOfParagraphToMove=..., endOfParagraphToMove=..., destination=..., preserveSelectio
n=true, preserveStyle=true) at ../../third_party/WebKit/Source/WebCore/editing/CompositeEditCommand.cpp:1220
#1  0x000000000219bb04 in WebCore::InsertListCommand::unlistifyParagraph (this=0x25bab8595b60, originalStart=..., listNode=0x25bab8c0c980, listChildNode=0x25bab852b840) at ../../thi
rd_party/WebKit/Source/WebCore/editing/InsertListCommand.cpp:306
#2  0x000000000219b245 in WebCore::InsertListCommand::doApplyForSingleParagraph (this=0x25bab8595b60, forceCreateList=false, listTag=..., currentSelection=0x25bab8429ec0) at ../../t
hird_party/WebKit/Source/WebCore/editing/InsertListCommand.cpp:246
#3  0x000000000219a46f in WebCore::InsertListCommand::doApply (this=0x25bab8595b60) at ../../third_party/WebKit/Source/WebCore/editing/InsertListCommand.cpp:158
#4  0x00000000025ce45f in WebCore::CompositeEditCommand::apply (this=0x25bab8595b60) at ../../third_party/WebKit/Source/WebCore/editing/CompositeEditCommand.cpp:214
#5  0x00000000025ce18a in WebCore::applyCommand (command=...) at ../../third_party/WebKit/Source/WebCore/editing/CompositeEditCommand.cpp:170
#6  0x0000000002185f2f in WebCore::executeInsertUnorderedList (frame=0x25bab8a4dc20) at ../../third_party/WebKit/Source/WebCore/editing/EditorCommand.cpp:565
#7  0x0000000002188f83 in WebCore::Editor::Command::execute (this=0x7fffde005690, parameter=..., triggeringEvent=0x0) at ../../third_party/WebKit/Source/WebCore/editing/EditorComman
d.cpp:1700
#8  0x0000000002bcc848 in WebCore::Document::execCommand (this=0x25bab8a1b020, commandName=..., userInterface=false, value=...) at ../../third_party/WebKit/Source/WebCore/dom/Docume
nt.cpp:4185
#9  0x00000000032fff04 in WebCore::DocumentV8Internal::execCommandMethod (args=...) at gen/webcore/bindings/V8Document.cpp:2357
#10 0x00000000032fff90 in WebCore::DocumentV8Internal::execCommandMethodCallback (args=...) at gen/webcore/bindings/V8Document.cpp:2362
#11 0x000024e9ceb44d15 in ?? ()
#12 0x00007fffde005a00 in ?? ()
#13 0x00007fffde005a08 in ?? ()
#14 0x0000000000000001 in ?? ()
#15 0x0000000000000000 in ?? ()


This also shows off a separate bug where the second InsertUnorderdList command that should unlistify breaks each list item into its own list and sets the blocks to inlines.

[Levi Weintraub]

I've got a fix for this.

[Ryosuke Niwa]

(In reply to comment #1)
> I've got a fix for this.

Nice!

[Levi Weintraub]

Created attachment 191630 [details]
Patch

[WebKit Review Bot]

Comment on attachment 191630 [details]
Patch

Attachment 191630 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17044102

New failing tests:
fast/parser/nested-fragment-parser-crash.html

[Levi Weintraub]

Comment on attachment 191630 [details]
Patch

I don't believe that's a related crash.

[Ryosuke Niwa]

Comment on attachment 191630 [details]
Patch

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

Please fix the test before you land it.

> LayoutTests/editing/execCommand/insert-remove-block-list-inside-presentational-inline.html:10
> +function styleList() {

styleList is a vague name.

> LayoutTests/editing/execCommand/insert-remove-block-list-inside-presentational-inline.html:11
> +    var newElem = document.createElement('div');

Please don't abbreviate element as elem.

> LayoutTests/editing/execCommand/insert-remove-block-list-inside-presentational-inline.html:14
> +    var textNode = document.createTextNode('x');
> +    newElem.appendChild(textNode);

We could do this in one line.

> LayoutTests/editing/execCommand/insert-remove-block-list-inside-presentational-inline.html:15
> +    root.insertBefore(newElem, root.firstChild);

Where is root coming from?

> LayoutTests/editing/execCommand/insert-remove-block-list-inside-presentational-inline.html:22
> +		testRunner.dumpAsText();

Nit: tab. cq-.

> LayoutTests/editing/execCommand/insert-remove-block-list-inside-presentational-inline.html:26
> +	root.offsetHeight;

Why do we need to force layout here? SelectAll is going to do that anyways

> LayoutTests/editing/execCommand/insert-remove-block-list-inside-presentational-inline.html:34
> +	finalMarkup = root.innerHTML;
> +	listIsRemoved = finalMarkup.indexOf("ul") == -1;

Why don't you just check document.getElementsByTagName('ul').length == 0 instead?

> LayoutTests/editing/execCommand/insert-remove-block-list-inside-presentational-inline.html:38
> +		root.parentNode.removeChild(root);

Nit: tab.

[Levi Weintraub]

Thanks for the review!

[Build Bot]

Comment on attachment 191630 [details]
Patch

Attachment 191630 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17011018

New failing tests:
fast/parser/nested-fragment-parser-crash.html
plugins/plugin-clip-subframe.html

[Levi Weintraub]

Created attachment 191784 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 191784 [details]
Patch for landing

Rejecting attachment 191784 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-04', 'apply-attachment', '--no-update', '--non-interactive', 191784, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
  return self.open(self.click(*args, **kwds))
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_mechanize.py", line 203, in open
    return self._mech_open(url, data, timeout=timeout)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_mechanize.py", line 255, in _mech_open
    raise response
webkitpy.thirdparty.autoinstalled.mechanize._response.httperror_seek_wrapper: HTTP Error 500: Internal Server Error

Full output: http://webkit-commit-queue.appspot.com/results/17086088

[WebKit Review Bot]

Comment on attachment 191784 [details]
Patch for landing

Attachment 191784 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17009341

New failing tests:
fast/parser/nested-fragment-parser-crash.html

[Build Bot]

Comment on attachment 191784 [details]
Patch for landing

Attachment 191784 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17050246

New failing tests:
fast/parser/nested-fragment-parser-crash.html

[Levi Weintraub]

Created attachment 191822 [details]
Patch

[Levi Weintraub]

That failure was actually legitimate and needed another 1-line change. Ryosuke, can you take another look?

[Ryosuke Niwa]

Comment on attachment 191822 [details]
Patch

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

> Source/WebCore/editing/EditingStyle.cpp:196
> -    return style->getPropertyCSSValue(CSSPropertyWebkitTextDecorationsInEffect) || style->getPropertyCSSValue(CSSPropertyTextDecoration);
> +    return style && (style->getPropertyCSSValue(CSSPropertyWebkitTextDecorationsInEffect) || style->getPropertyCSSValue(CSSPropertyTextDecoration));

This doesn't look right. We need to check the nullity in its caller instead.

[Levi Weintraub]

Comment on attachment 191822 [details]
Patch

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

>> Source/WebCore/editing/EditingStyle.cpp:196
>> +    return style && (style->getPropertyCSSValue(CSSPropertyWebkitTextDecorationsInEffect) || style->getPropertyCSSValue(CSSPropertyTextDecoration));
> 
> This doesn't look right. We need to check the nullity in its caller instead.

It matches the other version. See: HTMLElementEquivalent::propertyExistsInStyle. There are only 5 callers, all in EditingStyle. If you want I can fix all of them to do this null check.

[Ryosuke Niwa]

Comment on attachment 191822 [details]
Patch

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

>>> Source/WebCore/editing/EditingStyle.cpp:196
>>> +    return style && (style->getPropertyCSSValue(CSSPropertyWebkitTextDecorationsInEffect) || style->getPropertyCSSValue(CSSPropertyTextDecoration));
>> 
>> This doesn't look right. We need to check the nullity in its caller instead.
> 
> It matches the other version. See: HTMLElementEquivalent::propertyExistsInStyle. There are only 5 callers, all in EditingStyle. If you want I can fix all of them to do this null check.

That's wrong :( We need to remove that null check and move it elsewhere.
conflictsWithImplicitStyleOfAttributes and extractConflictingImplicitStyleOfAttributes already ensures style is not null.
So we just need to update elementMatchesAndPropertyIsNotInInlineStyleDecl.

[Levi Weintraub]

Created attachment 191843 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 191843 [details]
Patch

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

> Source/WebCore/editing/EditingStyle.cpp:131
> -    virtual bool propertyExistsInStyle(const StylePropertySet* style) const { return style && style->getPropertyCSSValue(m_propertyID); }
> +    virtual bool propertyExistsInStyle(const StylePropertySet* style) const { return style->getPropertyCSSValue(m_propertyID); }

You're missing a change log entry for this function.

[Levi Weintraub]

Comment on attachment 191843 [details]
Patch

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

>> Source/WebCore/editing/EditingStyle.cpp:131
>> +    virtual bool propertyExistsInStyle(const StylePropertySet* style) const { return style->getPropertyCSSValue(m_propertyID); }
> 
> You're missing a change log entry for this function.

Will do.

[Levi Weintraub]

Created attachment 191850 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 191850 [details]
Patch for landing

Clearing flags on attachment: 191850

Committed r144995: <http://trac.webkit.org/changeset/144995>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Antoine Quint]

This caused https://bugs.webkit.org/show_bug.cgi?id=118847.