Bug 41284 - ApplyStyleCommand::splitTextElementAtEndIfNeeded/splitTextAtEndIfNeeded ASSERTS should be returns.
Summary: ApplyStyleCommand::splitTextElementAtEndIfNeeded/splitTextAtEndIfNeeded ASSER...
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-28 10:33 PDT by Mike Fenton
Modified: 2010-09-17 14:58 PDT (History)
2 users (show)

See Also:


Attachments
Patch (2.81 KB, patch)
2010-06-28 10:33 PDT, Mike Fenton
ojan: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Fenton 2010-06-28 10:33:17 PDT
Created attachment 59909 [details]
Patch

Both splitTextElementAtStartIfNeeded and splitTextAtEndIfNeeded call splitTextNodeContainingElement and splitTextNode respectively assuming that the split will succeed and verify the resulting using ASSERTS.

In both functions there are a number of early returns possibly.  The state after the split occurs should be validated with an if statement so that a proper return can happen.
Comment 1 Ojan Vafai 2010-07-22 14:37:56 PDT
Comment on attachment 59909 [details]
Patch

Why is asserting wrong? When do we hit this? Does this cause a change in behavior? If so, can you add a layout test?
Comment 2 Ryosuke Niwa 2010-07-24 00:08:21 PDT
(In reply to comment #0)
> In both functions there are a number of early returns possibly.  The state after the split occurs should be validated with an if statement so that a proper return can happen.

Let me explain to you why this doesn't happen.  For splitTextAtEndIfNeeded, splitTextNode will create a text node before the text.  So, text->previousSibling() should ALWAYS return a valid node.  Since splitTextNode does not delete or move any nodes, we know that ALL start.node(), end.node(), and prevNode are valid pointers.

For splitTextElementAtEndIfNeeded, splitTextNodeContainingElement splits the text node along with its container.  So if we have <elem><text></elem>, we get <elem#1><text#1></elem#1><elem#2><text#2></elem#2> where <text#1><text#2>=<text>.  Now, text is pointing at <text#2> at this point because splitTextNodeContainingElement adds <text#1> and strips a part of text from <text> to make <text#2> (i.e. <text> and <text#2> share the same pointer value).  Now, text->parent()->previousSibling() is pointing at <elem#1>.  The last child of <elem#1> is <text#1>, and this node should exist as long as splitTextNodeContainingElement didn't blow up.  I omit the argument for startNode in this case because it's identical to that of splitTextAtEndIfNeeded.