ASSERT(destination.deepEquivalent().anchorNode()->inDocument()); fails. <div contenteditable="true" id="div"><hkern><span contenteditable="false"><dl>000A0<script> var sel = window.getSelection(); sel.setPosition(div, 2000000000); document.execCommand("Delete"); </script>
I'm going to look into this today. A quick glance in the debugger shows that mergeParagraphs() gets called with the following selections (S for startParagraph, E for endParagraph, and D for destination): BODY 0x131d05cb0 SE DIV 0x131d05ff0 D HKERN 0x131d064b0 SPAN 0x131d066a0 DL 0x131d06980 #text 0x131d07230 "000A0" SCRIPT 0x131d07310 #text 0x131d07550 "\nvar sel = window.getSelection();\nsel.setPosition(div, 2000000000);\ndocument.execCommand("Delete");\n" afterChildren, offset:0 It seems weird to merge paragraphs that aren't adjacent. What ends up happening is that cleanupAfterDeletion removes the DIV from the document entirely and then the ASSERT fires. The comments indicate that cleanupAfterDeletion is meant to help with list/table problems, so it seems like the codepath is getting called for something it wasn't intended to handle. I am also going to try and reduce this test case; I think it was generated by a fuzzer, and hopefully we don't need an svg element, a contenteditable=false span, a block tag within a span, and an invalid position to reproduce the problem.
Sigh... mergeParagraphs again. I personally won't spend too much time on crashes caused by mergeParagraphs because they're insanely hard to fix correctly. I'd focus on other high priority bugs if I were you.
Created attachment 107082 [details] Patch It looks like changing the mergeParagraphs algorithm for an edge case like this introduces a lot of risk. So this patch changes the ASSERT to an early bail, which fixes the crash.
Comment on attachment 107082 [details] Patch This is wrong. We need to fix cleanupAfterDeletion not to remove the destination.
(In reply to comment #4) > (From update of attachment 107082 [details]) > This is wrong. We need to fix cleanupAfterDeletion not to remove the destination. I definitely agree it's not an ideal fix. It does fix a potential crash without adding a lot of code, though. Do you think it's worth spending the time to fix this correctly? I wasn't able to reduce the test case further than what's in the layout test: <div contenteditable><hkern><span contenteditable="false">text
(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 107082 [details] [details]) > > This is wrong. We need to fix cleanupAfterDeletion not to remove the destination. > > I definitely agree it's not an ideal fix. It does fix a potential crash without adding a lot of code, though. Do you think it's worth spending the time to fix this correctly? I wasn't able to reduce the test case further than what's in the layout test: <div contenteditable><hkern><span contenteditable="false">text Yes. That assertion has caught us many bugs in the past, and I'm very reluctant to remove it. I'm okay with adding an early exit there in addition to the assertion but we shouldn't be removing the assertion.
Created attachment 116615 [details] Proposed patch Added a check for the root editability of the parent node. If parent is not root editable, we should not remove its child and hence not prune it
Comment on attachment 116615 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=116615&action=review > Source/WebCore/ChangeLog:8 > + Added a check for root editability of node's parent. You should explain why this fixes the crash and why your fix is correct. > Source/WebCore/editing/CompositeEditCommand.cpp:232 > + if (parent && !parent->rootEditableElement()) I think what you're trying to check is that parent->isContentEditable(). > LayoutTests/ChangeLog:8 > + Added a test case for the crash-67764. crash-67764 doesn't tell me anything about the kind of crash you're fixing. Please explain rather than giving the bug number. Also, the bug number is redundant because it's already given at line 4. > LayoutTests/editing/deleting/delete-block-merge-contents-025.html:6 > +var sel = window.getSelection(); > +sel.setPosition(div, 2000000000); > +document.execCommand("Delete"); This should be converted to a dumpAsText test. Just do: if (window.layoutTestController) layoutTestController.dumpAsText();
Created attachment 129024 [details] Updated Patch This patch is to make an early exit without removing the ASSERT.
Comment on attachment 129024 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129024&action=review r- due to the bad test description and comment. > Source/WebCore/editing/CompositeEditCommand.cpp:1037 > + // for VisiblePositions V1 and V2, it's possible V1 != V2 but still editing code treat them as same. > + // Ref : FIXME @ Position.h :inline bool operator==(const Position& a, const Position& b) I don't understand why the fact two VisiblePositions being treated same is resulting in this code since that's not true. I think what you meant is Position. But the fact multiple Positions can be treated as the same position is a well-known fact and doesn't deserve a comment. Instead, you should explain why you're calling rendersInDifferentPosition in this particular situation. > LayoutTests/editing/deleting/delete-block-merge-contents-025-expected.txt:1 > +This is for bug-67764,to Pass this testcase it should not crash. Please explain what the test is doing. Saying that it's for the bug 67764 doesn't explain anything about the nature of the test.
Created attachment 129241 [details] Updated Patch Updated patch with review comments implemented.
Comment on attachment 129241 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129241&action=review > Source/WebCore/ChangeLog:8 > + If caret position after deletion and destination position where to move paragraph Nit: "where to move paragraph" is making this sentence more confusing. "destination position" should be clear enough. > Source/WebCore/ChangeLog:10 > + Hence ASSERT. The bug summary says crash though?
Created attachment 129416 [details] Updated Patch Updated patch with all review comments implemented.
Comment on attachment 129416 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129416&action=review Please address nits below. > Source/WebCore/ChangeLog:8 > + If caret position after deletion and destination position coinsides than Nit: s/ than/, then/ > LayoutTests/editing/deleting/delete-block-merge-contents-025-expected.txt:1 > +This is to test a usecase in which caret position after deletion and the destination position where to move the paragraph, coinsides. To pass this testcase it should not crash. Same comment about "where to move the paragraph, ". > LayoutTests/editing/deleting/delete-block-merge-contents-025.html:1 > +<html> No DOCTYPE?
Created attachment 129425 [details] Updated Patch Updated patch with all review comments implemented.
Comment on attachment 129425 [details] Updated Patch Great. Thanks for the patch! r=me.
Comment on attachment 129425 [details] Updated Patch Clearing flags on attachment: 129425 Committed r109218: <http://trac.webkit.org/changeset/109218>
All reviewed patches have been landed. Closing bug.