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.
I've got a fix for this.
(In reply to comment #1) > I've got a fix for this. Nice!
Created attachment 191630 [details] Patch
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
Comment on attachment 191630 [details] Patch I don't believe that's a related crash.
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.
Thanks for the review!
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
Created attachment 191784 [details] Patch for landing
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
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
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
Created attachment 191822 [details] Patch
That failure was actually legitimate and needed another 1-line change. Ryosuke, can you take another look?
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.
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.
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.
Created attachment 191843 [details] Patch
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.
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.
Created attachment 191850 [details] Patch for landing
Comment on attachment 191850 [details] Patch for landing Clearing flags on attachment: 191850 Committed r144995: <http://trac.webkit.org/changeset/144995>
All reviewed patches have been landed. Closing bug.
This caused https://bugs.webkit.org/show_bug.cgi?id=118847.