Bug 144047 – VisibleSelection should only accept Range by reference

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brent Fulgham]

One of the available VisibleSelection constructors accepts a Range pointer. However, it immediately dereferences the pointer to access members, resulting in runtime crashes if an unwitting developer passes an unchecked Range pointer.

Since a nullptr is not valid input for this constructor, we should change the interface to require a reference. The code already must do null checks before passing Range pointers to this constructor, so simply changing this code to dereference the pointer is a trivial mechanical change.

This will make it harder to accidentally code an unsafe use of VisibleSelection.

[Brent Fulgham]

It's interesting (alarming?) that there are cases in the same file (e.g., WebPage.cpp) where null checks are performed, and others where it is missed:

Example 1: (WebPage.cpp Line 4207)
void WebPage::insertTextAsync(const String& text, const EditingRange& replacementEditingRange, bool registerUndoGroup)

        RefPtr<Range> replacementRange = rangeFromEditingRange(frame, replacementEditingRange);
        if (replacementRange)
            frame.selection().setSelection(VisibleSelection(replacementRange.get(), SEL_DEFAULT_AFFINITY));


Example 2: (WebPage.cpp Line 4296)
void WebPage::setCompositionAsync(const String& text, Vector<CompositionUnderline> underlines, const EditingRange& selection, const EditingRange& replacementEditingRange)

        replacementRange = rangeFromEditingRange(frame, replacementEditingRange);
        frame.selection().setSelection(VisibleSelection(replacementRange.get(), SEL_DEFAULT_AFFINITY));

[Brent Fulgham]

Created attachment 251383 [details]
Patch

[Tim Horton]

Comment on attachment 251383 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251383&action=review

> Source/WebCore/editing/Editor.cpp:2003
> +        RELEASE_ASSERT(badGrammarRange.get());

Please don't RELEASE_ASSERT in all these places. The next line will do a null deref anyway.

[Brent Fulgham]

Created attachment 251387 [details]
Patch v2 (No RELEASE_ASSERT, iOS Fixes)

[Brent Fulgham]

Created attachment 251390 [details]
Patch v3 (More build fixes for different configurations)

[Brent Fulgham]

Committed r183158: <http://trac.webkit.org/changeset/183158>