Bug 144047 - VisibleSelection should only accept Range by reference
Summary: VisibleSelection should only accept Range by reference
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brent Fulgham
Depends on:
Reported: 2015-04-22 09:32 PDT by Brent Fulgham
Modified: 2015-04-22 19:15 PDT (History)
2 users (show)

See Also:

Patch (20.42 KB, patch)
2015-04-22 16:48 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch v2 (No RELEASE_ASSERT, iOS Fixes) (24.63 KB, patch)
2015-04-22 17:06 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch v3 (More build fixes for different configurations) (29.68 KB, patch)
2015-04-22 17:38 PDT, Brent Fulgham
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2015-04-22 09:32:13 PDT
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.
Comment 1 Brent Fulgham 2015-04-22 09:36:18 PDT
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));
Comment 2 Brent Fulgham 2015-04-22 16:48:00 PDT
Created attachment 251383 [details]
Comment 3 Tim Horton 2015-04-22 16:53:50 PDT
Comment on attachment 251383 [details]

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.
Comment 4 Brent Fulgham 2015-04-22 17:06:06 PDT
Created attachment 251387 [details]
Patch v2 (No RELEASE_ASSERT, iOS Fixes)
Comment 5 Brent Fulgham 2015-04-22 17:38:43 PDT
Created attachment 251390 [details]
Patch v3 (More build fixes for different configurations)
Comment 6 Brent Fulgham 2015-04-22 19:15:51 PDT
Committed r183158: <http://trac.webkit.org/changeset/183158>