187013 - nsVoidArray::ElementAt and ::SafeElementAt shouldn't check mImpl

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Is there a reason that nsVoidArray::ElementAt and ::SafeElementAt does

return mImpl ? mImpl->mArray[aIndex] : nsnull;

instead of

return mImpl->mArray[aIndex];

*after* checking that aIndex is within bounds. For SafeElementAt it should be
compleatly safe to do the latter.

For ElementAt there one case where we would crash where we didn't used to crash;
if an empty array is accessed with a negative index. We have however asserted on
that for quite some time.

::ElementAt and ::SafeElementAt are inlined and rather heavily used so getting
rid of the extra check should at least save us some codesize.


Doug T added those extra checks as part of the checkin for bug 115853. However
there is no mention in the bug of why.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Doug: I'm going to propose undoing the changes you made to nsVoidArray.h in bug
115853.  The only case where something would happen differently is if the index
was negative, in which case it should be Asserting already.

Taking bug.  Patch to follow.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch to undo changes to nsVoidArray.h

This patch undoes the changes and adds a comment.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Though it may be a separate bug I should open, I should note that we can
probably remove the safety-valve from ElementAt() now (as per the comments in
nsVoidArray.h):

    // This will go away once all assertions are handled and we feel
    // comfortable that there aren't any more out there.  Negative values
    // are all handled I think.
    if (aIndex >= Count())
    {
      return nsnull;
    }

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

See bug 96108 for discussion of the bounds-checking removal

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Adding brendan since he was involved in the whole bug 96108 stuff

Comment 6

[Doug Turner (:dougt)]

why do you want to roll back this change?  As I remember, mImpl can be null.  

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

dougt: looking at the code for ::SafeElementAt i can't understand how mImpl can
be null.

In ::ElementAt it could in theory, but by then someone would have made a bad
call and we would have asserted. If that happens then we should fix the caller,
not work around it in ::ElementAt.

Comment 8

[Doug Turner (:dougt)]

i am not against rolling the change back.  In fact, I am not sure why i checked
the code in -- the bug numer I cited is for something all together different.  

if you and randell have reviewed the code and agree that mImpl can not be null,
then feel free to check in the patch.

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 111088 [details] [diff] [review]
Patch to undo changes to nsVoidArray.h

sure.  also see 196217. if you want to land both, feel free. (probaby want to
do it before beta.)

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This has been fixed