820182 - nsISupportsArray::ElementAt() is particularly evil

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Trevor Saunders (:tbsaunde)]

it returns an addrefed raw nsISupports* QED.

We can either just make it return a already_AddRefed, or remove it all together.  the later may be safer if dont_AddRef() doesn't gaurd against being passed a already_AddRefed, since some of the callers expect the crazy behavior and use dont_AddRef().

Comment 1

[Trevor Saunders (:tbsaunde)]

So, I started looking into what had to be changed for ElementAt() to go away, and found some lovely code in rdf/base/src/nsInMemoryDatasource.cpp which I think maybe uses if there is an array to decide what to do lter on, but I don't trust my understanding of it enough to change it much less think people will be interested in reviewing it, so I think getting rid of ElementAt() is probably out for now.

However it looks like dont_AddRef() does handle being passed an already_AddReed so that seems to be a reasonable option.

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: bug 820182 - nsISupportsArray::ElementAt() should return already_AddRefed<nsISupports>

Comment 3

[(no longer active)]

Comment on attachment 690894 [details] [diff] [review]
bug 820182 - nsISupportsArray::ElementAt() should return already_AddRefed<nsISupports>

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

r=me, assuming that you've verified that this covers every caller of ElementAt.  I'm a bit surprised that there are no callers that use dont_AddRef, for example.  I have not looked through all of the call sites, but I'm assuming that you have.

Comment 4

[Boris Zbarsky [:bzbarsky]]

For what it's worth, the last time I tried making this change (many years ago, granted) the issue described in comment 3 is precisely what I ran into: lots of code that assumed the current model, not limited to using dont_AddRef even...

In particular, there's a ton of mailnews code that uses nsISupportsArray; I'm not sure whether you built comm-central or looked into the callsites there.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Then again, I somewhat recall renaming ElementAt() in my tree, going through all the compile failures and fixing all the random leaks that I found, so maybe it's ok now...  It's just hard to tell.  :(

Comment 6

[(no longer active)]

Yeah, Trevor, at the very least you need to build Firefox, Thunderbird and SeaMonkey to make sure none of them break.  Renaming ElementAt temporarily is probably a good idea if you want the compiler to help you catch all of the call sites.

Comment 7

[Trevor Saunders (:tbsaunde)]

So, actually it turns out that its silly to write something like dont_AddRef(myArray->ElementAt(i)) now, but not actually harmful, because we define dont_AddRef() as

template <class T>
inline
const already_AddRefed<T>
dont_AddRef( T* aRawPtr )
  {
    return already_AddRefed<T>(aRawPtr);
  }

template <class T>
inline
const already_AddRefed<T>
dont_AddRef( const already_AddRefed<T> aAlreadyAddRefedPtr )
  {
    return aAlreadyAddRefedPtr;
  }

why we have that second overload I have no idea, it seems like it would be a good idea to remove it, but it means that its somewhat easier to get things to compile and be correct when making a change like this.

Comment 8

[(no longer active)]

(In reply to comment #7)
> So, actually it turns out that its silly to write something like
> dont_AddRef(myArray->ElementAt(i)) now, but not actually harmful, because we
> define dont_AddRef() as

No, it's not silly at all, that's what it's designed to do.

For example, the following code:

nsCOMPtr<nsIFoo> foo = mArray->ElementAt(i);

will address the i'th element two times, once inside ElementAt and once inside nsCOMPtr's constructor.  So effectively you'll leak a reference.  If you rewrite this as:

nsCOMPtr<nsIFoo> foo = dont_AddRef(mArray->ElementAt(i));

then the second AddRef will be avoided since nsCOMPtr's ctor knows to not call AddRef if it receives an already_AddRefed object.

> template <class T>
> inline
> const already_AddRefed<T>
> dont_AddRef( T* aRawPtr )
>   {
>     return already_AddRefed<T>(aRawPtr);
>   }
> 
> template <class T>
> inline
> const already_AddRefed<T>
> dont_AddRef( const already_AddRefed<T> aAlreadyAddRefedPtr )
>   {
>     return aAlreadyAddRefedPtr;
>   }
> 
> why we have that second overload I have no idea, it seems like it would be a
> good idea to remove it, but it means that its somewhat easier to get things to
> compile and be correct when making a change like this.

Hmm, actually with this second overload, it _should_ be ok if callers use dont_AddRef with your patch, since then they would effectively be using the second overload.  But still that's not very hygenic...

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to Ehsan Akhgari [:ehsan] from comment #8)
> (In reply to comment #7)
> > So, actually it turns out that its silly to write something like
> > dont_AddRef(myArray->ElementAt(i)) now, but not actually harmful, because we
> > define dont_AddRef() as
> 
> No, it's not silly at all, that's what it's designed to do.
> 
> For example, the following code:

I should have been more clear, by "now" I meant a world in which ElementAt() returns already_AddRefed, because that is the state of the world in my tree.

> > template <class T>
> > inline
> > const already_AddRefed<T>
> > dont_AddRef( const already_AddRefed<T> aAlreadyAddRefedPtr )
> >   {
> >     return aAlreadyAddRefedPtr;
> >   }
> > 
> > why we have that second overload I have no idea, it seems like it would be a
> > good idea to remove it, but it means that its somewhat easier to get things to
> > compile and be correct when making a change like this.
> 
> Hmm, actually with this second overload, it _should_ be ok if callers use
> dont_AddRef with your patch, since then they would effectively be using the
> second overload.  But still that's not very hygenic...

yes, that's what I meant by it being silly.

Comment 10

[(no longer active)]

OK, we're in violent agreement it seems!

Comment 11

[Trevor Saunders (:tbsaunde)]

standard8, neil  I'm about to attach a patch removing the last few uses of this method in m-c and the method itself.  Any idea how much work there is on the c-c side? or would you like to tell me how to build c-c to write you guys a patch?

Comment 12

[Trevor Saunders (:tbsaunde)]

Attachment: bug 820182 - remove nsISupportsArray::ElementAt()

Comment 13

[[e-mail]]

Comment on attachment 691086 [details] [diff] [review]
bug 820182 - remove nsISupportsArray::ElementAt()

IIRC the last big mailnews use of ElementAt went away as part of bug 379806 a mere 4 years ago. I didn't notice any current consumers.

>-            mCurrent = static_cast<nsIRDFResource *>
>-                                  (mHashArcs->ElementAt(itemCount));
>+            mHashArcs->GetElementAt(itemCount,
>+                                    reinterpret_cast<nsISupports**>(&mCurrent));
Shouldn't this change to do_QueryElementAt too?

>+native alreadyAddRefedNSISupports(already_AddRefed<nsISupports>);
Obsolete?

>+NS_IMETHODIMP
>+nsSupportsArray::GetElementAt(uint32_t aIndex, nsISupports **aOutPtr)
> {
>   if (aIndex < mCount) {
>-    nsISupports*  element = mArray[aIndex];
>-    NS_IF_ADDREF(element);
>-    return element;
>+    NS_IF_ADDREF(*aOutPtr = mArray[aIndex]);
>   }
>-  return 0;
>+  return NS_OK;
Need to set your result before returning.

Comment 14

[(no longer active)]

Comment on attachment 691086 [details] [diff] [review]
bug 820182 - remove nsISupportsArray::ElementAt()

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

::: xpcom/ds/nsISupportsArray.idl
@@ +31,5 @@
>  #define NS_SUPPORTSARRAY_CLASSNAME "Supports Array"
>   
>  %}
>  
> +native alreadyAddRefedNSISupports(already_AddRefed<nsISupports>);

This is no longer needed.

Comment 15

[(no longer active)]

And Neil spotted two things that I missed!  Please fix those too.  Thanks!

Comment 16

[Trevor Saunders (:tbsaunde)]

(In reply to [e-mail] from comment #13)
> Comment on attachment 691086 [details] [diff] [review]
> bug 820182 - remove nsISupportsArray::ElementAt()
> 
> IIRC the last big mailnews use of ElementAt went away as part of bug 379806
> a mere 4 years ago. I didn't notice any current consumers.

great

> >-            mCurrent = static_cast<nsIRDFResource *>
> >-                                  (mHashArcs->ElementAt(itemCount));
> >+            mHashArcs->GetElementAt(itemCount,
> >+                                    reinterpret_cast<nsISupports**>(&mCurrent));
> Shouldn't this change to do_QueryElementAt too?

I don't see a reason why not

> >+native alreadyAddRefedNSISupports(already_AddRefed<nsISupports>);
> Obsolete?

yes

Comment 17

[Trevor Saunders (:tbsaunde)]

Turns out this depends on other ElementAt() removals in bug 819936 so it won't be landing for a little while.

Comment 18

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/163f1b12bdef

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/163f1b12bdef

Comment 20

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/06ae0a5f4d9c

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/06ae0a5f4d9c