819940 - remove EnumerateForwards/Backwards on nsISupportsArray

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Trevor Saunders (:tbsaunde)]

you can usually write the loop yourself, but why are you using nsISupportsArray in the first place.

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: remove nsISupportsArray::EnumerateBackwards()

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: remove nsISupportsArray::EnumerateForwards()

Comment 3

[(no longer active)]

Comment on attachment 690372 [details] [diff] [review]
remove nsISupportsArray::EnumerateBackwards()

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

::: xpcom/ds/nsISupportsArray.idl
@@ -74,5 @@
>        boolean EnumerateForwards(in nsISupportsArrayEnumFunc aFunc,
>                                  in voidPtr aData);
> -  [notxpcom, noscript]
> -      boolean EnumerateBackwards(in nsISupportsArrayEnumFunc aFunc,
> -                                 in voidPtr aData);

Rev the uuid please.

Comment 4

[(no longer active)]

Comment on attachment 690374 [details] [diff] [review]
remove nsISupportsArray::EnumerateForwards()

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

::: content/xul/templates/src/nsXULTreeBuilder.cpp
@@ +285,5 @@
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(nsXULTreeBuilder, nsXULTemplateBuilder)
>      NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mBoxObject)
>      NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mSelection)
>      NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mPersistStateStore)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mObservers)

Nit: please fix the indentation.

@@ +838,5 @@
>      }
>  
> +    uint32_t count = mObservers.Count();
> +    for (uint32_t i = 0; i < count; ++i) {
> +        nsCOMPtr<nsIXULTreeBuilderObserver> observer = mObservers.SafeObjectAt(i);

Again, no need for AddRef/Release here, just use a plain nsIXULTreeBuilderObserver* please.  You should do this elsewhere in this patch as well.

::: xpcom/ds/nsISupportsArray.idl
@@ -71,5 @@
>    void Compact();
>    
> -  [notxpcom, noscript]
> -      boolean EnumerateForwards(in nsISupportsArrayEnumFunc aFunc,
> -                                in voidPtr aData);

Rev the uuid here too.

::: xpcom/ds/nsSupportsArray.cpp
@@ +589,2 @@
>    nsresult rv;
> +  rv = NS_NewISupportsArray(getter_AddRefs(newArray));

Nit: declare and initialize rv on the same line, please.

@@ +592,5 @@
> +
> +  uint32_t count = 0;
> +  Count(&count);
> +  for (uint32_t i = 0; i < count; i++) {
> +    nsCOMPtr<nsISupports> element = ElementAt(i);

No need to AddRef and Release here, you can just say:

nsISupport* element = mArray[i];

::: xpfe/appshell/src/nsWindowMediator.cpp
@@ +91,5 @@
>    if (!windowInfo)
>      return NS_ERROR_OUT_OF_MEMORY;
>  
> +  WindowTitleData winData = { inWindow, nullptr };
> +  mListeners.EnumerateForwards(notifyOpenWindow, (void*)&winData);

Please remove these explicit void* casts.  They're not needed.

::: xpfe/appshell/src/nsWindowMediator.h
@@ +69,5 @@
>    bool          mSortingZOrder;
>    bool          mReady;
>    mozilla::Mutex mListLock;
>  
> +  nsCOMArray<nsIWindowMediatorListener> mListeners;

You can probably remove the nsISupportsArray.h #include from this file.

Comment 5

[Trevor Saunders (:tbsaunde)]

> @@ +838,5 @@
> >      }
> >  
> > +    uint32_t count = mObservers.Count();
> > +    for (uint32_t i = 0; i < count; ++i) {
> > +        nsCOMPtr<nsIXULTreeBuilderObserver> observer = mObservers.SafeObjectAt(i);
> 
> Again, no need for AddRef/Release here, just use a plain
> nsIXULTreeBuilderObserver* please.  You should do this elsewhere in this
> patch as well.

so, I'm not absolutely sure that's actually safe.  Consider what happens ifthe the current observer removes itself.  If we aren't holding a ref then it could be deleted while the observer's Notify() is running.

> ::: xpcom/ds/nsISupportsArray.idl
> @@ -71,5 @@
> >    void Compact();
> >    
> > -  [notxpcom, noscript]
> > -      boolean EnumerateForwards(in nsISupportsArrayEnumFunc aFunc,
> > -                                in voidPtr aData);
> 
> Rev the uuid here too.

yeah, what was I thinking????

> @@ +592,5 @@
> > +
> > +  uint32_t count = 0;
> > +  Count(&count);
> > +  for (uint32_t i = 0; i < count; i++) {
> > +    nsCOMPtr<nsISupports> element = ElementAt(i);
> 
> No need to AddRef and Release here, you can just say:
> 
> nsISupport* element = mArray[i];

actually this is worse than silly that leaks, because ElementAt() is crazy and returns a nsISupports* that its AddRefed.

Comment 6

[(no longer active)]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > @@ +838,5 @@
> > >      }
> > >  
> > > +    uint32_t count = mObservers.Count();
> > > +    for (uint32_t i = 0; i < count; ++i) {
> > > +        nsCOMPtr<nsIXULTreeBuilderObserver> observer = mObservers.SafeObjectAt(i);
> > 
> > Again, no need for AddRef/Release here, just use a plain
> > nsIXULTreeBuilderObserver* please.  You should do this elsewhere in this
> > patch as well.
> 
> so, I'm not absolutely sure that's actually safe.  Consider what happens
> ifthe the current observer removes itself.  If we aren't holding a ref then
> it could be deleted while the observer's Notify() is running.

Yeah, but only if it actually gets a chance to remove itself.  Most of these call sites just call a single method on the observer pointer, and even if that method deletes the observer, the pointer is not used again (except for the CanDrop/DoDrop call site near the end of this file.)  But yeah this is error prone in case somebody changes these loops in the future.  So, keep these AddRef's I guess.

> > @@ +592,5 @@
> > > +
> > > +  uint32_t count = 0;
> > > +  Count(&count);
> > > +  for (uint32_t i = 0; i < count; i++) {
> > > +    nsCOMPtr<nsISupports> element = ElementAt(i);
> > 
> > No need to AddRef and Release here, you can just say:
> > 
> > nsISupport* element = mArray[i];
> 
> actually this is worse than silly that leaks, because ElementAt() is crazy
> and returns a nsISupports* that its AddRefed.

Please file a follow-up to make that return an already_AddRefed then.

Comment 7

[(no longer active)]

Comment on attachment 690374 [details] [diff] [review]
remove nsISupportsArray::EnumerateForwards()

r+ with my comments except for the one about the extra AddRef/Releases.

Comment 8

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d00a8bf1508

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to Ehsan Akhgari [:ehsan] from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > > @@ +838,5 @@
> > > >      }
> > > >  
> > > > +    uint32_t count = mObservers.Count();
> > > > +    for (uint32_t i = 0; i < count; ++i) {
> > > > +        nsCOMPtr<nsIXULTreeBuilderObserver> observer = mObservers.SafeObjectAt(i);
> > > 
> > > Again, no need for AddRef/Release here, just use a plain
> > > nsIXULTreeBuilderObserver* please.  You should do this elsewhere in this
> > > patch as well.
> > 
> > so, I'm not absolutely sure that's actually safe.  Consider what happens
> > ifthe the current observer removes itself.  If we aren't holding a ref then
> > it could be deleted while the observer's Notify() is running.
> 
> Yeah, but only if it actually gets a chance to remove itself.  Most of these
> call sites just call a single method on the observer pointer, and even if
> that method deletes the observer, the pointer is not used again (except for
> the CanDrop/DoDrop call site near the end of this file.)  But yeah this is
> error prone in case somebody changes these loops in the future.  So, keep
> these AddRef's I guess.
> 
> > > @@ +592,5 @@
> > > > +
> > > > +  uint32_t count = 0;
> > > > +  Count(&count);
> > > > +  for (uint32_t i = 0; i < count; i++) {
> > > > +    nsCOMPtr<nsISupports> element = ElementAt(i);
> > > 
> > > No need to AddRef and Release here, you can just say:
> > > 
> > > nsISupport* element = mArray[i];
> > 
> > actually this is worse than silly that leaks, because ElementAt() is crazy
> > and returns a nsISupports* that its AddRefed.
> 
> Please file a follow-up to make that return an already_AddRefed then.

820182 but as I said there it may be safer to just remove it all togehter.  Anyway I'll try and look at it tomorrow.

Comment 10

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/db40186d9d39

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8d00a8bf1508

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/db40186d9d39