Last Comment Bug 766181 - Need an extra null check for aOutIndex in Selection::AddItem
: Need an extra null check for aOutIndex in Selection::AddItem
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Selection (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla18
Assigned To: Graeme McCutcheon [:graememcc]
:
:
Mentors:
Depends on:
Blocks: 493111
  Show dependency treegraph
 
Reported: 2012-06-19 10:09 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2012-09-17 12:23 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Trivial fix (671 bytes, patch)
2012-06-22 11:23 PDT, Graeme McCutcheon [:graememcc]
no flags Details | Diff | Splinter Review
v2 (2.53 KB, patch)
2012-07-05 08:14 PDT, Graeme McCutcheon [:graememcc]
no flags Details | Diff | Splinter Review
v3 (2.47 KB, patch)
2012-09-14 12:38 PDT, Graeme McCutcheon [:graememcc]
roc: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2012-06-19 10:09:13 PDT
Need a null-check at <http://hg.mozilla.org/mozilla-central/annotate/d29af708ec3c/layout/generic/nsSelection.cpp#l3559>.
Comment 1 Graeme McCutcheon [:graememcc] 2012-06-22 11:23:43 PDT
Created attachment 635827 [details] [diff] [review]
Trivial fix

Trivial fix.

I fear this wasn't the cause of the crashes Boris was hitting. I tried to work backwards with the aim of creating a crashtest. To hit this line requires the existing selection to be non-empty, and the given range to overlap it in some way. However, the only caller of AddItem without an initialised aOutIndex is Selection::Collapse, which explicitly empties the selection before calling AddItem.
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-22 16:42:57 PDT
How about just making all callers pass something for aOutIndex? I think that's the way to go here.
Comment 3 Graeme McCutcheon [:graememcc] 2012-07-05 08:14:37 PDT
Created attachment 639349 [details] [diff] [review]
v2
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-05 17:44:48 PDT
Comment on attachment 639349 [details] [diff] [review]
v2

Review of attachment 639349 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsSelection.cpp
@@ +3459,1 @@
>      return NS_ERROR_NULL_POINTER;

Don't check aOutIndex here. Just add NS_ASSERTION(aOutIndex, "aOutIndex can't be null").
Comment 5 Graeme McCutcheon [:graememcc] 2012-09-14 12:38:58 PDT
Created attachment 661320 [details] [diff] [review]
v3

I forgot I still owed you a patch here.
Comment 6 Graeme McCutcheon [:graememcc] 2012-09-17 07:22:15 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bf7fa20ea33
Comment 7 Ed Morley [:emorley] 2012-09-17 12:23:53 PDT
https://hg.mozilla.org/mozilla-central/rev/8bf7fa20ea33

Note You need to log in before you can comment on or make changes to this bug.