1167403 - Mark gfxFont::GlyphChangeObserver::mFont as MOZ_NON_OWNING_REF

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Nika Layzell [:nika] (ni? for response)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=31c30f18b6f6

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Make gfcFont::GlyphChangeObserver::mFont a smart pointer

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8609020 [details] [diff] [review]
Make gfcFont::GlyphChangeObserver::mFont a smart pointer

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

::: gfx/thebes/gfxFont.h
@@ +1769,5 @@
>          explicit GlyphChangeObserver(gfxFont *aFont) : mFont(aFont)
>          {
>              mFont->AddGlyphChangeObserver(this);
>          }
> +        nsRefPtr<gfxFont> mFont;

It looks like mFont was a weak reference. Will making it a strong reference cause it to leak?

Comment 3

[Nika Layzell [:nika] (ni? for response)]

First time I looked through I noticed that gfxFont keeps a reference to the GlyphChangeObserver through the member mGlyphChangeObservers. This member is a nsAutoPtr<nsTHashtable<nsPtrHashKey<GlyphChangeObserver> > >, and the nsPtrHashKey entry type doesn't own it's inner pointer - it only holds a weak reference to it. So I thought it made sense to make this change.

You may be right, however, that this change would cause a memory leak. I've looked a bit closer and noticed that gfxFont's destructor enumerates through the hashtable entries, deleting the mFont property with ForgetFont.

I'll update the patch to instead mark this member as MOZ_NON_OWNING_REF, and add a comment explaining this.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: Mark gfxFont::GlyphChangeObserver::mFont as MOZ_NON_OWNING_REF

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8609393 [details] [diff] [review]
Mark gfxFont::GlyphChangeObserver::mFont as MOZ_NON_OWNING_REF

John is a more appropriate reviewer

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0cc054180475

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0cc054180475