Bug 19221 - ASSERT during Range creation (due to editing commands)
: ASSERT during Range creation (due to editing commands)
Status: RESOLVED DUPLICATE of bug 25056
: WebKit
HTML Editing
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
: 25056
: 18858
  Show dependency treegraph
 
Reported: 2008-05-23 12:19 PST by
Modified: 2009-04-06 02:26 PST (History)


Attachments
hits ASSERT in debug mode (220 bytes, text/html)
2008-05-23 12:19 PST, Eric Seidel
no flags Details
Fix and test case (4.03 KB, patch)
2009-02-19 13:32 PST, Eric Seidel
ap: review-
Review Patch | Details | Formatted Diff | Diff
Further cleanup to rangeCompliantEquivalent callers (15.75 KB, patch)
2009-02-19 15:58 PST, Eric Seidel
no flags Review Patch | Details | Formatted Diff | Diff
Complete fix, including ap's suggested cleanup of rangeCompliantEquivalent callers (21.00 KB, patch)
2009-02-19 16:03 PST, Eric Seidel
no flags Review Patch | Details | Formatted Diff | Diff
First pass renaming (54.92 KB, patch)
2009-02-26 15:11 PST, Eric Seidel
no flags Review Patch | Details | Formatted Diff | Diff
Clean up DOMSelection some (9.15 KB, patch)
2009-02-26 17:52 PST, Eric Seidel
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2008-05-23 12:19:08 PST
ASSERT during Range creation (due to editing commands)

ASSERTION FAILED: !ec
(/Users/eseidel/Projects/WebKit/WebCore/dom/Range.cpp:96 WebCore::Range::Range(WTF::PassRefPtr<WebCore::Document>, WTF::PassRefPtr<WebCore::Node>, int, WTF::PassRefPtr<WebCore::Node>, int))

<BODY><SCRIPT>
document.execCommand("selectall")
document.designMode = "on"
document.execCommand("insertimage", false, "red")
document.execCommand("createLink", false, true)
document.execCommand("removeformat")
</SCRIPT>

Yet another from the editing fuzzer.
------- Comment #1 From 2008-05-23 12:19:39 PST -------
Created an attachment (id=21320) [details]
hits ASSERT in debug mode
------- Comment #2 From 2009-02-19 12:53:11 PST -------
The ASSERT is tripping because we're getting back an INDEX_SIZE_ERR because the "end" is being set to (img, 1).

Tree for the end node:
BODY    0x1bf90d60
*    IMG    0x1bffa830
    SCRIPT    0x1a40b440
        #text    0x1bf91420 "\ndocument.execCommand("selectall")\ndocument.designMode = "on"\ndocument.execCommand("insertimage", false, "red")\ndocument.execCommand("createLink", false, true)\ndocument.execCommand("removeformat")\n"

I expect this is just an "internal" range being passed off to a DOM Range object.  I'm sure we have some functions for converting these.
------- Comment #3 From 2009-02-19 12:54:28 PST -------
To explain for anyone following along at home.  WebKit sometimes uses (img, 1) style ranges (where 1, being off the end of the node, signifies the position directly after the node), instead of (img.parentNode, image->childIndex()).
------- Comment #4 From 2009-02-19 12:55:30 PST -------
> I'm sure we have some functions for converting these.

rangeCompliantEquivalent
------- Comment #5 From 2009-02-19 13:32:17 PST -------
Created an attachment (id=27811) [details]
Fix and test case

 LayoutTests/ChangeLog                              |   15 +++++++++++++++
 .../assert-on-range-creation-expected.txt          |    1 +
 .../execCommand/assert-on-range-creation.html      |   12 ++++++++++++
 WebCore/ChangeLog                                  |   17 +++++++++++++++++
 WebCore/dom/Range.cpp                              |    7 ++++++-
 5 files changed, 51 insertions(+), 1 deletions(-)
------- Comment #6 From 2009-02-19 13:42:12 PST -------
(From update of attachment 27811 [details])
> +        This fixes an ASSERT hit when trying to wrap an image in an link using
> +        editing commands (which probably resulted in incorrect behavior before too).

Doesn't the ASSERT happen during RemoveFormat?  (I haven't looked at the backtrace, just the testcase that you've included).

r=me
------- Comment #7 From 2009-02-19 13:44:48 PST -------
(From update of attachment 27811 [details])
A lot of callers use rangeCompliantEquivalent() already. I don't know if that's important for performance, but if you add normalization in Range::create, it needs to be removed at call sites.
------- Comment #8 From 2009-02-19 15:58:48 PST -------
Created an attachment (id=27812) [details]
Further cleanup to rangeCompliantEquivalent callers

 WebCore/dom/Range.h                        |    2 +
 WebCore/editing/ApplyStyleCommand.cpp      |    4 +-
 WebCore/editing/CompositeEditCommand.cpp   |   20 +++++++---------
 WebCore/editing/DeleteSelectionCommand.cpp |    4 +-
 WebCore/editing/Editor.cpp                 |    2 +-
 WebCore/editing/VisiblePosition.cpp        |    2 +-
 WebCore/editing/VisibleSelection.cpp       |    8 +-----
 WebCore/editing/htmlediting.cpp            |    6 ++++-
 WebCore/editing/visible_units.cpp          |   34 +++++++++++++--------------
 WebCore/page/DOMSelection.cpp              |   12 +++------
 10 files changed, 44 insertions(+), 50 deletions(-)
------- Comment #9 From 2009-02-19 16:03:09 PST -------
Created an attachment (id=27814) [details]
Complete fix, including ap's suggested cleanup of rangeCompliantEquivalent callers

 LayoutTests/ChangeLog                              |   15 ++++++
 .../assert-on-range-creation-expected.txt          |    1 +
 .../execCommand/assert-on-range-creation.html      |   12 +++++
 WebCore/ChangeLog                                  |   46 ++++++++++++++++++++
 WebCore/dom/Range.cpp                              |    7 +++-
 WebCore/dom/Range.h                                |    2 +
 WebCore/editing/ApplyStyleCommand.cpp              |    4 +-
 WebCore/editing/CompositeEditCommand.cpp           |   20 ++++-----
 WebCore/editing/DeleteSelectionCommand.cpp         |    4 +-
 WebCore/editing/Editor.cpp                         |    2 +-
 WebCore/editing/VisiblePosition.cpp                |    2 +-
 WebCore/editing/VisibleSelection.cpp               |    8 +---
 WebCore/editing/htmlediting.cpp                    |    6 ++-
 WebCore/editing/visible_units.cpp                  |   34 +++++++--------
 WebCore/page/DOMSelection.cpp                      |   12 ++---
 15 files changed, 124 insertions(+), 51 deletions(-)
------- Comment #10 From 2009-02-19 16:49:01 PST -------
(From update of attachment 27814 [details])
Sigh.  I've convinced myself that I really should split out the concept of positionAvoidingIgnoredContent out from rangeCompliantEquivalent before doing this massive replace. :(  Painful, but better for the codebase in the longrun I think.

https://bugs.webkit.org/show_bug.cgi?id=24045
------- Comment #11 From 2009-02-19 23:39:23 PST -------
(From update of attachment 27814 [details])
Sigh.  I'll fix the other bug first as I mentioned in the previous comment.
------- Comment #12 From 2009-02-26 15:11:59 PST -------
Created an attachment (id=28043) [details]
First pass renaming

 LayoutTests/ChangeLog                              |   24 +++++
 .../editing/assert-on-range-creation-expected.txt  |    1 +
 LayoutTests/editing/assert-on-range-creation.html  |   12 +++
 WebCore/ChangeLog                                  |  105 ++++++++++++++++++++
 WebCore/WebCore.base.exp                           |    5 +-
 WebCore/WebCore.xcodeproj/project.pbxproj          |    2 +-
 WebCore/dom/Position.cpp                           |   54 +++++++++-
 WebCore/dom/Position.h                             |   52 ++++++----
 WebCore/dom/Range.cpp                              |    8 +-
 WebCore/dom/RangeBoundaryPoint.h                   |   48 +++++-----
 WebCore/editing/ApplyStyleCommand.cpp              |    4 +-
 WebCore/editing/CompositeEditCommand.cpp           |   12 +-
 WebCore/editing/DeleteSelectionCommand.cpp         |   24 +++---
 WebCore/editing/Editor.cpp                         |    4 +-
 WebCore/editing/InsertLineBreakCommand.cpp         |    2 +-
 .../editing/InsertParagraphSeparatorCommand.cpp    |    6 +-
 WebCore/editing/ReplaceSelectionCommand.cpp        |    2 +-
 WebCore/editing/VisiblePosition.cpp                |   57 +++++------
 WebCore/editing/VisiblePosition.h                  |    4 +-
 WebCore/editing/VisibleSelection.cpp               |   12 +-
 WebCore/editing/htmlediting.cpp                    |   64 +++++-------
 WebCore/editing/htmlediting.h                      |    3 +-
 WebCore/editing/visible_units.cpp                  |   68 ++++++-------
 WebCore/page/DOMSelection.cpp                      |   16 ++--
 WebKit/mac/ChangeLog                               |   24 +++++
 WebKit/mac/WebView/WebFrame.mm                     |    9 +-
 26 files changed, 410 insertions(+), 212 deletions(-)
------- Comment #13 From 2009-02-26 17:52:13 PST -------
Created an attachment (id=28057) [details]
Clean up DOMSelection some

 LayoutTests/ChangeLog                              |    9 +++
 .../selection/click-before-and-after-table.html    |   13 ++--
 WebCore/ChangeLog                                  |   27 ++++++++
 WebCore/page/DOMSelection.cpp                      |   70 ++++++++++++--------
 WebCore/page/DOMSelection.h                        |    6 ++-
 5 files changed, 89 insertions(+), 36 deletions(-)
------- Comment #14 From 2009-02-26 17:52:46 PST -------
(From update of attachment 28057 [details])
I've continued work on my git branch, to try and explore what sections of the code I can remove these silly positionAvoidingIgnoredContent hacks from.
------- Comment #15 From 2009-02-26 19:10:17 PST -------
(From update of attachment 28043 [details])
Don't really need to talk about WebCore code changes in the LayoutTest ChangeLog.

+// Create a dom-compliant position which is outside of any ignored content

"DOM"

+// FIXME: This combines both avoiding tables and avoiding ignored nodes,
+// those should probably be separate functions.
+// FIXME: Split out part of this into positionAvoidingFirstPositionInTable?

These sound like the same FIXME.

+Position positionAvoidingIgnoredContent(const Position& originalPosition)
+{
+    Position position = originalPosition.toRangeCompliantEquivalent();

Should also mention that this function includes making positions DOM compliant, which I believe is something you want to split out, no?

+        I also cleaned up Position a little bit to make it a real class.

Please include your rationale for doing this, I remember Darin felt quite strongly about Position being more like a struct.

+    // Returns node() cast to an element() or the closest element ancestor
+    Element* element() const;

"or the cloest element ancestor of node()."
------- Comment #16 From 2009-03-11 00:14:15 PST -------
(From update of attachment 28057 [details])
seems ok.  r=me
------- Comment #17 From 2009-03-12 13:57:38 PST -------
I'm having second thoughts about this patch.  This doesn't solve the "Position" problem in Editing.  We still need to split out the two ways in which Positions are handled inside Editing.

I think I will make a new patch which does only enough to fix this ASSERT and try re-writing Positions in a different way.
------- Comment #18 From 2009-03-12 15:58:17 PST -------
(From update of attachment 28043 [details])
I've decided I don't like this patch after all.  :(  Sorry for asking you to review this.  I think we need a different attack for fixing the Position mess.
------- Comment #19 From 2009-03-12 16:06:46 PST -------
(From update of attachment 28057 [details])
I've committed this cleanup patch.  However, this doesn't actually have to do anything with what this bug is about.  So I'm clearing the review flags and marking this obsolete.

Committing to http://svn.webkit.org/repository/webkit/trunk ...
    M    LayoutTests/ChangeLog
    M    LayoutTests/editing/selection/click-before-and-after-table.html
    M    WebCore/ChangeLog
    M    WebCore/page/DOMSelection.cpp
    M    WebCore/page/DOMSelection.h
Committed r41657
------- Comment #20 From 2009-04-06 02:26:53 PST -------
This will be fixed by landing bug 25056.

*** This bug has been marked as a duplicate of 25056 ***