Bug 16279 – VisiblePosition::canonicalPosition can wrongly return a Position object with a null m_node

• Component: HTML Editing
• Status: UNCONFIRMED

[Andrew Bonventre]

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.

[Andrew Bonventre]

Created attachment 17681 [details]
Proposed fix where you return the original Position object passed instead of an uninitialized one.

[Darin Adler]

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.

[Alexey Proskuryakov]

This code is still present in ToT.

[James Robinson]

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.

[James Robinson]

I of course meant "not crashing is better than crashing".

[Enrica Casucci]

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?

[Ojan Vafai]

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.