1114880 - Annotate some strong and weak references in XPCOM

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Comment 1

[(no longer active)]

Attachment: Annotate some strong and wweak references in XPCOM

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8540536 [details] [diff] [review]
Annotate some strong and wweak references in XPCOM

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

Your commit message needs a little tweaking for |wweak|.

r=me with the comments addressed.

::: xpcom/glue/nsComponentManagerUtils.h
@@ +78,5 @@
>    virtual nsresult NS_FASTCALL operator()(const nsIID&, void**) const;
>  
>  private:
> +  nsCOMPtr<nsIFactory>   mFactory;
> +  nsISupports* MOZ_WEAK_REF mOuter;

Why nsCOMPtr for mFactory, but annotation for mOuter?  I would think that annotation would be sufficient for mFactory, too.

::: xpcom/glue/nsHashKeys.h
@@ +384,5 @@
>    }
>    enum { ALLOW_MEMMOVE = true };
>  
>  protected:
> +  T* MOZ_STRONG_REF mKey;

I don't think this is semantically correct.  In a brief grep of these, it looks like somebody else is keeping mKey alive, not the hash table.  So I would think that this would be MOZ_WEAK_REF, because somebody is keeping this alive for me?

Comment 3

[Andrew McCreight [:mccr8]]

Yeah, nsPtrHashKey is definitely weak.

Comment 4

[Nathan Froyd [:froydnj]]

Maybe we should have called them MOZ_OWNING_REF and MOZ_NON_OWNING_REF.

Comment 5

[(no longer active)]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #2)
> Your commit message needs a little tweaking for |wweak|.

Hah!

> ::: xpcom/glue/nsComponentManagerUtils.h
> @@ +78,5 @@
> >    virtual nsresult NS_FASTCALL operator()(const nsIID&, void**) const;
> >  
> >  private:
> > +  nsCOMPtr<nsIFactory>   mFactory;
> > +  nsISupports* MOZ_WEAK_REF mOuter;
> 
> Why nsCOMPtr for mFactory, but annotation for mOuter?  I would think that
> annotation would be sufficient for mFactory, too.

OK, sure.

> ::: xpcom/glue/nsHashKeys.h
> @@ +384,5 @@
> >    }
> >    enum { ALLOW_MEMMOVE = true };
> >  
> >  protected:
> > +  T* MOZ_STRONG_REF mKey;
> 
> I don't think this is semantically correct.  In a brief grep of these, it
> looks like somebody else is keeping mKey alive, not the hash table.  So I
> would think that this would be MOZ_WEAK_REF, because somebody is keeping
> this alive for me?

Oops, my bad.  I added MOZ_WEAK_REF afterwards and probably forgot to adjust this annotation.  Sorry!

Comment 6

[(no longer active)]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #4)
> Maybe we should have called them MOZ_OWNING_REF and MOZ_NON_OWNING_REF.

They do seem less ambiguous, I suppose.  I can change them later (have tons of local patches applied right now...)

Comment 7

[(no longer active)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #6)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #4)
> > Maybe we should have called them MOZ_OWNING_REF and MOZ_NON_OWNING_REF.
> 
> They do seem less ambiguous, I suppose.  I can change them later (have tons
> of local patches applied right now...)

Done: https://hg.mozilla.org/integration/mozilla-inbound/rev/9e07239decd3

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b9094d876dc5
https://hg.mozilla.org/mozilla-central/rev/9e07239decd3