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.
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));
Created attachment 251383 [details] Patch
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.
Created attachment 251387 [details] Patch v2 (No RELEASE_ASSERT, iOS Fixes)
Created attachment 251390 [details] Patch v3 (More build fixes for different configurations)
Committed r183158: <http://trac.webkit.org/changeset/183158>