Bug 19221 - ASSERT during Range creation (due to editing commands)
: ASSERT during Range creation (due to editing commands)
Status: RESOLVED DUPLICATE of bug 25056
Product: WebKit
Classification: Unclassified
Component: HTML Editing
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To: Nobody
:
Depends on: 25056
Blocks: 18858
  Show dependency treegraph
 
Reported: 2008-05-23 12:19 PDT by Eric Seidel
Modified: 2009-04-06 02:26 PDT (History)
2 users (show)

See Also:


Attachments
hits ASSERT in debug mode (220 bytes, text/html)
2008-05-23 12:19 PDT, Eric Seidel
no flags Details
Fix and test case (4.03 KB, patch)
2009-02-19 13:32 PST, Eric Seidel
ap: review-
Details | Formatted Diff | Diff
Further cleanup to rangeCompliantEquivalent callers (15.75 KB, patch)
2009-02-19 15:58 PST, Eric Seidel
no flags 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 Details | Formatted Diff | Diff
First pass renaming (54.92 KB, patch)
2009-02-26 15:11 PST, Eric Seidel
no flags Details | Formatted Diff | Diff
Clean up DOMSelection some (9.15 KB, patch)
2009-02-26 17:52 PST, Eric Seidel
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel 2008-05-23 12:19:08 PDT
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 Eric Seidel 2008-05-23 12:19:39 PDT
Created attachment 21320 [details]
hits ASSERT in debug mode
Comment 2 Eric Seidel 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 Eric Seidel 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 Justin Garcia 2009-02-19 12:55:30 PST
> I'm sure we have some functions for converting these.

rangeCompliantEquivalent
Comment 5 Eric Seidel 2009-02-19 13:32:17 PST
Created attachment 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 Justin Garcia 2009-02-19 13:42:12 PST
Comment on attachment 27811 [details]
Fix and test case

> +        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 Alexey Proskuryakov 2009-02-19 13:44:48 PST
Comment on attachment 27811 [details]
Fix and test case

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 Eric Seidel 2009-02-19 15:58:48 PST
Created attachment 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 Eric Seidel 2009-02-19 16:03:09 PST
Created attachment 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 Eric Seidel 2009-02-19 16:49:01 PST
Comment on attachment 27814 [details]
Complete fix, including ap's suggested cleanup of rangeCompliantEquivalent callers

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 Eric Seidel 2009-02-19 23:39:23 PST
Comment on attachment 27814 [details]
Complete fix, including ap's suggested cleanup of rangeCompliantEquivalent callers

Sigh.  I'll fix the other bug first as I mentioned in the previous comment.
Comment 12 Eric Seidel 2009-02-26 15:11:59 PST
Created attachment 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 Eric Seidel 2009-02-26 17:52:13 PST
Created attachment 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 Eric Seidel 2009-02-26 17:52:46 PST
Comment on attachment 28057 [details]
Clean up DOMSelection some

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 Justin Garcia 2009-02-26 19:10:17 PST
Comment on attachment 28043 [details]
First pass renaming

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 Justin Garcia 2009-03-11 00:14:15 PDT
Comment on attachment 28057 [details]
Clean up DOMSelection some

seems ok.  r=me
Comment 17 Eric Seidel 2009-03-12 13:57:38 PDT
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 Eric Seidel 2009-03-12 15:58:17 PDT
Comment on attachment 28043 [details]
First pass renaming

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 Eric Seidel 2009-03-12 16:06:46 PDT
Comment on attachment 28057 [details]
Clean up DOMSelection some

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 Eric Seidel 2009-04-06 02:26:53 PDT
This will be fixed by landing bug 25056.

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