Bug 19221

Summary:

ASSERT during Range creation (due to editing commands)

Product:

WebKit

Reporter:

Eric Seidel (no email) <eric>

Component:

HTML Editing

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED DUPLICATE

Severity:

Normal

CC:

ap, justin.garcia

Priority:

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

OS X 10.5

Bug Depends on:

25056

Bug Blocks:

18858

Attachments:

Description	Flags
hits ASSERT in debug mode	none
Fix and test case	ap: review-
Further cleanup to rangeCompliantEquivalent callers	none
Complete fix, including ap's suggested cleanup of rangeCompliantEquivalent callers	none
First pass renaming	none
Clean up DOMSelection some	none

Eric Seidel (no email)

Reported 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.

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

Eric Seidel (no email)

Comment 1 2008-05-23 12:19:39 PDT

Created attachment 21320 [details] hits ASSERT in debug mode

Eric Seidel (no email)

Comment 2 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.

Eric Seidel (no email)

Comment 3 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()).

Justin Garcia

Comment 4 2009-02-19 12:55:30 PST

> I'm sure we have some functions for converting these. rangeCompliantEquivalent

Eric Seidel (no email)

Comment 5 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(-)

Justin Garcia

Comment 6 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

Alexey Proskuryakov

Comment 7 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.

Eric Seidel (no email)

Comment 8 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(-)

Eric Seidel (no email)

Comment 9 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(-)

Eric Seidel (no email)

Comment 10 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

Eric Seidel (no email)

Comment 11 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.

Eric Seidel (no email)

Comment 12 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(-)

Eric Seidel (no email)

Comment 13 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(-)

Eric Seidel (no email)

Comment 14 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.

Justin Garcia

Comment 15 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()."

Justin Garcia

Comment 16 2009-03-11 00:14:15 PDT

Comment on attachment 28057 [details] Clean up DOMSelection some seems ok. r=me

Eric Seidel (no email)

Comment 17 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.

Eric Seidel (no email)

Comment 18 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.

Eric Seidel (no email)

Comment 19 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

Eric Seidel (no email)

Comment 20 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 ***

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