Bug 32131 - WebCore::canHaveChildrenForEditing ReadAV@NULL (cd05b3b20e0f4c6b3afe5d165a1798aa)
Summary: WebCore::canHaveChildrenForEditing ReadAV@NULL (cd05b3b20e0f4c6b3afe5d165a179...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows Vista
: P1 Normal
Assignee: Nobody
URL: http://skypher.com/SkyLined/Repro/Web...
Keywords: HasReduction, InRadar
Depends on:
Blocks:
 
Reported: 2009-12-03 13:05 PST by Berend-Jan Wever
Modified: 2010-03-08 20:32 PST (History)
6 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Berend-Jan Wever 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
Comment 1 Berend-Jan Wever 2009-12-03 13:08:33 PST
Added online repro.
Comment 2 Mark Rowe (bdash) 2009-12-03 17:03:41 PST
<rdar://problem/7442973>
Comment 3 Tony Chang 2010-01-14 23:46:25 PST
Created attachment 46645 [details]
Patch
Comment 4 Tony Chang 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.
Comment 5 Tony Chang 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.
Comment 6 Enrica Casucci 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;
>      }
Comment 7 Tony Chang 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.
Comment 8 Enrica Casucci 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.
Comment 9 Tony Chang 2010-01-18 22:02:52 PST
Created attachment 46884 [details]
Patch
Comment 10 Tony Chang 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.
Comment 11 Eric Seidel (no email) 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?
Comment 12 Tony Chang 2010-02-01 17:19:36 PST
Created attachment 47885 [details]
Patch
Comment 13 Tony Chang 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.
Comment 14 Tony Chang 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.
Comment 15 Adam Barth 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.
Comment 16 Tony Chang 2010-03-08 20:32:23 PST
Committed r55705: <http://trac.webkit.org/changeset/55705>