Bug 32131

Summary:

WebCore::canHaveChildrenForEditing ReadAV@NULL (cd05b3b20e0f4c6b3afe5d165a1798aa)

Product:

WebKit

Reporter:

Berend-Jan Wever <skylined>

Component:

HTML Editing

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

adele, darin, enrica, eric, justin.garcia, tony

Priority:

Keywords:

HasReduction, InRadar

Version:

528+ (Nightly build)

Hardware:

OS:

Windows Vista

URL:

http://skypher.com/SkyLined/Repro/WebKit/Bug%2032131%20-%20WebCore..canHaveChildrenForEditing%20ReadAV@NULL%20(cd05b3b20e0f4c6b3afe5d165a1798aa)/repro.html

Attachments:

Description	Flags
Repro	none
Patch	none
Patch	none
Patch	abarth: review+

Berend-Jan Wever

Reported 2009-12-03 13:05:24 PST

Created attachment 44266 [details] Repro The following HTML causes a NULL pointer read access violation exception in WebCore::canHaveChildrenForEditing: <BODY></BODY> <SCRIPT> document.execCommand("SelectAll"); document.designMode = "on"; document.execCommand("underline"); document.execCommand("InsertUnorderedList"); document.execCommand("InsertImage"); document.execCommand("InsertLineBreak"); document.execCommand("InsertText", false, 34359738365); document.execCommand("JustifyRight"); document.execCommand("Outdent"); document.execCommand("insertunorderedlist"); document.execCommand("JustifyCenter"); document.execCommand("selectall"); document.execCommand("insertorderedlist"); </SCRIPT> Relevant call stack: WebCore::canHaveChildrenForEditing WebCore::CompositeEditCommand::insertNodeAt WebCore::InsertListCommand::doApply WebCore::InsertListCommand::modifyRange WebCore::InsertListCommand::doApply WebCore::EditCommand::apply WebCore::applyCommand WebCore::executeInsertOrderedList WebCore::Editor::Command::execute WebCore::Document::execCommand WebCore::DocumentInternal::execCommandCallback

Attachments
Repro (583 bytes, text/html) 2009-12-03 13:05 PST, Berend-Jan Wever	no flags	Details
Patch (4.95 KB, patch) 2010-01-14 23:46 PST, Tony Chang	no flags	Details Formatted Diff Diff
Patch (5.47 KB, patch) 2010-01-18 22:02 PST, Tony Chang	no flags	Details Formatted Diff Diff
Patch (5.46 KB, patch) 2010-02-01 17:19 PST, Tony Chang	abarth: review+	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Berend-Jan Wever

Comment 1 2009-12-03 13:08:33 PST

Added online repro.

Mark Rowe (bdash)

Comment 2 2009-12-03 17:03:41 PST

<rdar://problem/7442973>

Tony Chang

Comment 3 2010-01-14 23:46:25 PST

Created attachment 46645 [details] Patch

Tony Chang

Comment 4 2010-01-14 23:59:59 PST

Here's what's happening: insertorderedlist works by taking each paragraph and indenting them one by one. It indents a paragraph by creating the new list item and moving the paragraph from the old location to under the new list item. When moving a paragraph, it puts a placeholder near the old location, moves the paragraph to the new location, then tries to clean up the placeholder and placeholder's parent node. When we delete a paragraph from the old location, we call DeleteSelectionCommand::calculateTypingStyleAfterDelete. This can call applyStyle(), which can move our endingSelection(). Unfortunately, we need the endingSelection() position for removing the placeholder near the old location. This can result in getting the wrong placeholder which causes us to delete the wrong nodes. Specifically, it's the text-align and display style on the div that causes applyStyle to be called which leads to this crash.

Tony Chang

Comment 5 2010-01-15 00:02:32 PST

Oh, one more thing: This caused an extra node to be indented in a different layout test: editing/execCommand/19087.html (added here: https://bugs.webkit.org/show_bug.cgi?id=19087 ). I'm not entirely sure why that's happening.

Enrica Casucci

Comment 6 2010-01-15 12:40:32 PST

Comment on attachment 46645 [details] Patch > diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog > index 52b75bc..5ebd638 100644 > --- a/LayoutTests/ChangeLog > +++ b/LayoutTests/ChangeLog > @@ -1,3 +1,14 @@ > +2010-01-14 Tony Chang <tony@chromium.org> > + > + Reviewed by NOBODY (OOPS!). > + > + Editing crash when inserting a list. > + https://bugs.webkit.org/show_bug.cgi?id=32131 > + > + * editing/execCommand/19087-expected.txt: > + * editing/execCommand/insert-ordered-list-expected.txt: Added. > + * editing/execCommand/insert-ordered-list.html: Added. > + > 2010-01-13 Kent Hansen <kent.hansen@nokia.com> > > Reviewed by Oliver Hunt. > diff --git a/LayoutTests/editing/execCommand/19087-expected.txt b/LayoutTests/editing/execCommand/19087-expected.txt > index 3211e80..391e821 100644 > --- a/LayoutTests/editing/execCommand/19087-expected.txt > +++ b/LayoutTests/editing/execCommand/19087-expected.txt > @@ -5,3 +5,4 @@ This tests for a crash when indenting a particular selection that contains a blo > > > > + > diff --git a/LayoutTests/editing/execCommand/insert-ordered-list-expected.txt b/LayoutTests/editing/execCommand/insert-ordered-list-expected.txt > new file mode 100644 > index 0000000..7997d03 > --- /dev/null > +++ b/LayoutTests/editing/execCommand/insert-ordered-list-expected.txt > @@ -0,0 +1,3 @@ > +SUCCESS > +text > + > diff --git a/LayoutTests/editing/execCommand/insert-ordered-list.html b/LayoutTests/editing/execCommand/insert-ordered-list.html > new file mode 100644 > index 0000000..f2d2df0 > --- /dev/null > +++ b/LayoutTests/editing/execCommand/insert-ordered-list.html > @@ -0,0 +1,24 @@ > +<BODY> > +<u> > + <img><br> > + <div> > + <ul> > + <li> > + <span><u> > + <div style="text-align:right; display: inline;"> > + <span><u>text</u></span> > + </div> > + </u></span> > + </li> > + </ul> > + </div> > +</u> > +</BODY> > +<SCRIPT> > + if (window.layoutTestController) > + layoutTestController.dumpAsText(); > + document.designMode = "on"; > + document.execCommand("selectall"); > + document.execCommand("insertorderedlist"); > + document.getElementsByTagName('u')[0].innerHTML = 'SUCCESS'; > +</SCRIPT> > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog > index 8d01b57..86086ab 100644 > --- a/WebCore/ChangeLog > +++ b/WebCore/ChangeLog > @@ -1,3 +1,20 @@ > +2010-01-14 Tony Chang <tony@chromium.org> > + > + Reviewed by NOBODY (OOPS!). > + > + https://bugs.webkit.org/show_bug.cgi?id=32131 > + Editing crash when trying to insert an ordered list. > + While moving paragraphs that have been added to the new list, > + we lose the position of the placeholder <br> causing us to delete > + a bunch of nodes we don't mean to delete. > + > + Test: editing/execCommand/insert-ordered-list.html > + > + * editing/CompositeEditCommand.cpp: > + (WebCore::CompositeEditCommand::moveParagraphs): > + * editing/DeleteSelectionCommand.cpp: > + (WebCore::DeleteSelectionCommand::calculateTypingStyleAfterDelete): > + > 2010-01-13 Gavin Barraclough <barraclough@apple.com> > > Reviewed by Oliver Hunt. > diff --git a/WebCore/editing/CompositeEditCommand.cpp b/WebCore/editing/CompositeEditCommand.cpp > index e9b6971..c69e84a 100644 > --- a/WebCore/editing/CompositeEditCommand.cpp > +++ b/WebCore/editing/CompositeEditCommand.cpp > @@ -946,10 +946,10 @@ void CompositeEditCommand::moveParagraphs(const VisiblePosition& startOfParagrap > > setEndingSelection(VisibleSelection(start, end, DOWNSTREAM)); > deleteSelection(false, false, false, false); > - > ASSERT(destination.deepEquivalent().node()->inDocument()); > > cleanupAfterDeletion(); > + ASSERT(destination.deepEquivalent().node()->inDocument()); > I dont' understand why you moved the ASSERT here. In moveparagraph destination should always be valid after deleteSelection, because destination represents the new location to move to and deleteSelection only affects the old stuff. > // Add a br if pruning an empty block level element caused a collapse. For example: > // foo^ > @@ -971,6 +971,7 @@ void CompositeEditCommand::moveParagraphs(const VisiblePosition& startOfParagrap > destinationIndex = TextIterator::rangeLength(startToDestinationRange.get(), true); > > setEndingSelection(destination); > + ASSERT(endingSelection().isCaretOrRange()); > applyCommandToComposite(ReplaceSelectionCommand::create(document(), fragment, true, false, !preserveStyle, false, true)); > > // If the selection is in an empty paragraph, restore styles from the old empty paragraph to the new empty paragraph. > diff --git a/WebCore/editing/DeleteSelectionCommand.cpp b/WebCore/editing/DeleteSelectionCommand.cpp > index f07f038..3dd1490 100644 > --- a/WebCore/editing/DeleteSelectionCommand.cpp > +++ b/WebCore/editing/DeleteSelectionCommand.cpp > @@ -688,6 +688,8 @@ void DeleteSelectionCommand::calculateTypingStyleAfterDelete() > applyStyle(m_typingStyle.get(), EditActionUnspecified); > // applyStyle can destroy the placeholder that was at m_endingPosition if it needs to > // move it, but it will set an endingSelection() at [movedPlaceholder, 0] if it does so. > + // We don't want to move endingSelection(), because another command might be relying on it. > + setEndingSelection(visibleEnd); I believe the 19087.html is failing because you are moving the end selection in a case where it is not necessary. You should try to find out why this test fails, I don't think you should just re-baseline the test without an explanation. > m_endingPosition = endingSelection().start(); > m_typingStyle = 0; > }

Tony Chang

Comment 7 2010-01-17 20:44:47 PST

(In reply to comment #6) > > --- a/WebCore/editing/CompositeEditCommand.cpp > > +++ b/WebCore/editing/CompositeEditCommand.cpp > > @@ -946,10 +946,10 @@ void CompositeEditCommand::moveParagraphs(const VisiblePosition& startOfParagrap > > > > setEndingSelection(VisibleSelection(start, end, DOWNSTREAM)); > > deleteSelection(false, false, false, false); > > - > > ASSERT(destination.deepEquivalent().node()->inDocument()); > > > > cleanupAfterDeletion(); > > + ASSERT(destination.deepEquivalent().node()->inDocument()); > > > > I dont' understand why you moved the ASSERT here. In moveparagraph destination > should always be valid after deleteSelection, because destination represents > the > new location to move to and deleteSelection only affects the old stuff. I was removing a blank line and adding another ASSERT. The destination needs to be valid after the cleanup so we know where to insert paragraph into. > I believe the 19087.html is failing because you are moving the end selection in > a case where it is not necessary. > You should try to find out why this test fails, I don't think you should just > re-baseline the test without an explanation. You're right, the bug is lower: it's in applyStyle. In ApplyStyleCommand::applyBlockStyle, it tries to save the selection, move the paragraphs, then restore the selection. In this test case, it fails to restore the selection to the right place. It looks like this might be a bug in TextIterator::rangeLength and TextIterator::rangeFromLocationAndLength not agreeing on how to restore a range? I'm not sure what rangeLength is supposed to represent.

Enrica Casucci

Comment 8 2010-01-18 11:04:27 PST

(In reply to comment #7) > (In reply to comment #6) > > > --- a/WebCore/editing/CompositeEditCommand.cpp > > > +++ b/WebCore/editing/CompositeEditCommand.cpp > > > @@ -946,10 +946,10 @@ void CompositeEditCommand::moveParagraphs(const VisiblePosition& startOfParagrap > > > > > > setEndingSelection(VisibleSelection(start, end, DOWNSTREAM)); > > > deleteSelection(false, false, false, false); > > > - > > > ASSERT(destination.deepEquivalent().node()->inDocument()); > > > > > > cleanupAfterDeletion(); > > > + ASSERT(destination.deepEquivalent().node()->inDocument()); > > > > > > > I dont' understand why you moved the ASSERT here. In moveparagraph destination > > should always be valid after deleteSelection, because destination represents > > the > > new location to move to and deleteSelection only affects the old stuff. > > I was removing a blank line and adding another ASSERT. The destination needs > to be valid after the cleanup so we know where to insert paragraph into. > > The point I'm trying to make is that it is not necessary to move the ASSERT. Destination is a position that outside the selection being deleted. The ASSERT should verify that there is no overlap between destination and the selection to be deleted. We should not make unnecessary code changes. > > I believe the 19087.html is failing because you are moving the end selection in > > a case where it is not necessary. > > You should try to find out why this test fails, I don't think you should just > > re-baseline the test without an explanation. > > You're right, the bug is lower: it's in applyStyle. In > ApplyStyleCommand::applyBlockStyle, it tries to save the selection, move the > paragraphs, then restore the selection. In this test case, it fails to restore > the selection to the right place. It looks like this might be a bug in > TextIterator::rangeLength and TextIterator::rangeFromLocationAndLength not > agreeing on how to restore a range? I'm not sure what rangeLength is supposed > to represent. I don't agree. The test is failing because one line is left behind and I believe it has to do with moving the end selection. TextIterator computes correctly the position starting from an offset expressed in terms of number of visible characters. It could be off only if a layout is needed.

Tony Chang

Comment 9 2010-01-18 22:02:52 PST

Created attachment 46884 [details] Patch

Tony Chang

Comment 10 2010-01-18 22:04:24 PST

(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > > --- a/WebCore/editing/CompositeEditCommand.cpp > > > > +++ b/WebCore/editing/CompositeEditCommand.cpp > > > > @@ -946,10 +946,10 @@ void CompositeEditCommand::moveParagraphs(const VisiblePosition& startOfParagrap > > > > > > > > setEndingSelection(VisibleSelection(start, end, DOWNSTREAM)); > > > > deleteSelection(false, false, false, false); > > > > - > > > > ASSERT(destination.deepEquivalent().node()->inDocument()); > > > > > > > > cleanupAfterDeletion(); > > > > + ASSERT(destination.deepEquivalent().node()->inDocument()); > > > > > > > > > > I dont' understand why you moved the ASSERT here. In moveparagraph destination > > > should always be valid after deleteSelection, because destination represents > > > the > > > new location to move to and deleteSelection only affects the old stuff. > > > > I was removing a blank line and adding another ASSERT. The destination needs > > to be valid after the cleanup so we know where to insert paragraph into. > > > > > The point I'm trying to make is that it is not necessary to move the ASSERT. > Destination is a position that outside the selection being deleted. The ASSERT > should verify that there is no overlap between destination and the selection to > be deleted. We should not make unnecessary code changes. Ok, I put back the blank line. > > > I believe the 19087.html is failing because you are moving the end selection in > > > a case where it is not necessary. > > > You should try to find out why this test fails, I don't think you should just > > > re-baseline the test without an explanation. > > > > You're right, the bug is lower: it's in applyStyle. In > > ApplyStyleCommand::applyBlockStyle, it tries to save the selection, move the > > paragraphs, then restore the selection. In this test case, it fails to restore > > the selection to the right place. It looks like this might be a bug in > > TextIterator::rangeLength and TextIterator::rangeFromLocationAndLength not > > agreeing on how to restore a range? I'm not sure what rangeLength is supposed > > to represent. > I don't agree. The test is failing because one line is left behind and I > believe it has to do with moving the end selection. TextIterator computes > correctly the position starting from an offset expressed in terms of number of > visible characters. It could be off only if a layout is needed. I was agreeing with your assessment that moving the end selection was wrong. I've posted a new patch that works around the bug in TextIterator::rangeFromLocationAndLength.

Eric Seidel (no email)

Comment 11 2010-02-01 15:24:39 PST

Comment on attachment 46884 [details] Patch If you're getting a position from a Range, can't you use the non-deprecated offset lookup?

Tony Chang

Comment 12 2010-02-01 17:19:36 PST

Created attachment 47885 [details] Patch

Tony Chang

Comment 13 2010-02-01 17:21:10 PST

(In reply to comment #11) > (From update of attachment 46884 [details]) > If you're getting a position from a Range, can't you use the non-deprecated > offset lookup? Yes, you are correct. New patch uploaded using the range offset directly. For reference, the existing use of deprecated offset is bug 25193.

Tony Chang

Comment 14 2010-02-14 19:41:31 PST

Justin: It looks like you were the original author of this code. Do you have time to review this? Thanks.

Adam Barth

Comment 15 2010-03-08 16:50:26 PST

Comment on attachment 47885 [details] Patch This change is a bit out of my area, but the change looks plausible and Tony has addressed all of Enrica and Eric's comments. Sorry for the delay in reviewing your patch Tony.

Tony Chang

Comment 16 2010-03-08 20:32:23 PST

Committed r55705: <http://trac.webkit.org/changeset/55705>

Note You need to log in before you can comment on or make changes to this bug.