Bug 16279 - VisiblePosition::canonicalPosition can wrongly return a Position object with a null m_node
Summary: VisiblePosition::canonicalPosition can wrongly return a Position object with ...
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords: GoogleBug
Depends on:
Blocks:
 
Reported: 2007-12-03 11:28 PST by Andrew Bonventre
Modified: 2017-07-18 08:27 PDT (History)
6 users (show)

See Also:


Attachments
Proposed fix where you return the original Position object passed instead of an uninitialized one. (1.44 KB, patch)
2007-12-03 11:31 PST, Andrew Bonventre
darin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Bonventre 2007-12-03 11:28:47 PST
The only case in which canonicalPosition should ever return a Position object with a null m_node is when the passed-in Position object has a null m_node itself.  In the case where there is no suitable upstream or downstream candidate and (!nextIsInSameEditableElement && !prevIsInSameEditableElement) evaluates to true, it will return Position(), which itself has a null m_node.  This will crash the browser due to a null reference.

This needs a whittled down test case, and I am working on it, but any advice on how to reach this codepath before I find one would be greatly appreciated.
Comment 1 Andrew Bonventre 2007-12-03 11:31:56 PST
Created attachment 17681 [details]
Proposed fix where you return the original Position object passed instead of an uninitialized one.
Comment 2 Darin Adler 2007-12-03 13:20:15 PST
Comment on attachment 17681 [details]
Proposed fix where you return the original Position object passed instead of an uninitialized one.

Looks fine, but needs a regression test.

review- because of the lack of a test.
Comment 3 Alexey Proskuryakov 2010-06-11 16:40:12 PDT
This code is still present in ToT.
Comment 4 James Robinson 2010-06-11 19:21:36 PDT
Tests are better than no tests, but crashing is also better than not crashing and this change looks pretty obviously correct by inspection.  This code is from http://trac.webkit.org/changeset/14952.
Comment 5 James Robinson 2010-06-11 19:43:57 PDT
I of course meant "not crashing is better than crashing".
Comment 6 Enrica Casucci 2010-06-14 13:58:58 PDT
I don't think this is the right thing to do. I agree that it avoids potential crashes, but it also changes completely the semantics of canonicalizePosition. From my understanding, it should always return a candidate or a null Position. If we return the original position we don't know whether is was found to be a candidate or not.
Do all the layout tests still pass with this change?
Comment 7 Ojan Vafai 2010-06-16 15:11:02 PDT
Bug 30116 has a couple cases where returning null causes crashes. In that case, we added null-checks higher up. But those aren't really a complete fix. They were just a "not crashing is better than crashing" solution.

A quick scan of the deepEquivalent calls in WebCore shows a ton of places where we don't null-check and probably should. For example, http://trac.webkit.org/browser/trunk/WebCore/editing/CompositeEditCommand.cpp#L661. We either need to change the calling code throughout the codebase to null-check or we need to assert that canonicalPosition only returns a null Position if the passed in Position was null.

Maybe as an interim step, we can add ASSERTs to canonicalPosition anywhere we might return null. That way, we can find some cases where we hit this and that will help decide whether we should be returning non-null, or null-checking higher up. If we do this, the if-statement modified in this patch would probably want an ASSERT_NOT_REACHED and the final return value of canconicalPosition would need to assert "next" is not null.