Annotate some strong and weak references in XPCOM

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

(Blocks 1 bug)

unspecified
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

No description provided.
Assignee: nobody → ehsan.akhgari
Blocks: 1114683
Summary: Annotate some strong and wweak references in XPCOM → Annotate some strong and weak references in XPCOM
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?
Attachment #8540536 - Flags: review?(nfroyd) → review+
Yeah, nsPtrHashKey is definitely weak.
Maybe we should have called them MOZ_OWNING_REF and MOZ_NON_OWNING_REF.
(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!
(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...)
(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
https://hg.mozilla.org/mozilla-central/rev/b9094d876dc5
https://hg.mozilla.org/mozilla-central/rev/9e07239decd3
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.