1181323 - Eliminate duplicate mRefCnt members in nsSVGRenderingObserver subclasses

Status: RESOLVED FIXED

(Core :: SVG, defect)

Comment 1

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

Attachment: Eliminate duplicate mRefCnt members in nsSVGRenderingObserver subclasses

Comment 2

[Andrew McCreight [:mccr8]]

I think the NS_DECL_ISUPPORTS should all be public.

Comment 3

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

(In reply to Andrew McCreight [:mccr8] from comment #2)
> I think the NS_DECL_ISUPPORTS should all be public.

NS_DECL_ISUPPORTS contains a `public:` in the macro, in fact it expands to:

public:                                                                       
  NS_IMETHOD QueryInterface(REFNSIID aIID,                                    
                            void** aInstancePtr) override;                    
  NS_IMETHOD_(MozExternalRefCountType) AddRef(void) override;                 
  NS_IMETHOD_(MozExternalRefCountType) Release(void) override;                
protected:                                                                    
  nsAutoRefCnt mRefCnt;                                                       
  NS_DECL_OWNINGTHREAD                                                        
public:

Comment 4

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

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b033b7d9b2c

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Michael Layzell [:mystor] from comment #3)
> (In reply to Andrew McCreight [:mccr8] from comment #2)
> > I think the NS_DECL_ISUPPORTS should all be public.
> 
> NS_DECL_ISUPPORTS contains a `public:` in the macro, in fact it expands to:

Best not to depend on that, or else it can be a footgun for people who come along later & don't realize that.

e.g. someone coming along later and seeing this...
>  class SVGRootRenderingObserver final : public nsSVGRenderingObserver {
>   NS_DECL_ISUPPORTS
>  public:
>    SVGRootRenderingObserver(SVGDocumentWrapper* aDocWrapper,
...might incorrectly think they're OK putting MyNewPrivateMethod() before the "public" declaration.

They might even add an explicit "private" keyword, like so, without realizing it'd have no effect:

>  class SVGRootRenderingObserver final : public nsSVGRenderingObserver {
>  private:
>   NS_DECL_ISUPPORTS
>   void MyNewPrivateMethod();
>  public:
>    SVGRootRenderingObserver(SVGDocumentWrapper* aDocWrapper,

This sort of footgun becomes less likely if the NS_DECL_ISUPPORTS is just inside of "public" and hence doesn't have weird private/public-changing side effects.

Comment 6

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

Attachment: Eliminate duplicate mRefCnt members in nsSVGRenderingObserver subclasses

Moved the NS_DECL_ISUPPORTS into the public: block.
Also moved from jwatt to you, because it appears as though he's going to be away for a while.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8631577 [details] [diff] [review]
Eliminate duplicate mRefCnt members in nsSVGRenderingObserver subclasses

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

A few nits:
> Bug 1181323 - Eliminate duplicate mRefCnt members in nsSVGRenderingObserver subclasses

I think I'm missing something -- where are the duplicate mRefCnt members coming from? It looks like you're just moving NS_DECL_ISUPPORTS (and the refcounting guts that it entails) from superclass to subclass, so you're changing *which class* declares mRefCnt. But where/how did we have duplicate mRefCnt before this?

::: image/VectorImage.cpp
@@ +110,5 @@
>    VectorImage* const mVectorImage;   // Raw pointer because it owns me.
>    bool mHonoringInvalidations;
> +
> +  virtual ~SVGRootRenderingObserver()
> +  {

Please move this destructor up, so it's just underneath the "protected:" decl.

(Because: (1) generally, we put constructors/destructors near the top of the class, not at the very end; and (2) best not to have member-variable declarations in between methods -- member-variables belong at the bottom.)

::: layout/svg/nsSVGEffects.h
@@ +179,5 @@
>      , mFrameReference(aFrame)
>    {}
>  
>  protected:
> +  ~nsSVGRenderingObserverProperty() {}

Label this as "virtual". (It'll be virtual regardless, because it's virtual in the parent class, but good to be on the safe side.  We have child classes whose instances could be referenced & destroyed via nsRefPtr<nsSVGRenderingObserverProperty> variables, and we better make sure we actually invoke the child classes' destructors when that happens - hence, destructor definitely needs to be virtual.)

Comment 8

[Andrew McCreight [:mccr8]]

From the static analysis results in the dependent bug:

1:59.89 /Volumes/Devel/Code/mozilla/bug602122/layout/generic/../svg/nsSVGEffects.h:201:1: warning: Refcounted record 'nsSVGFilterReference' has multiple mRefCnt members
1:59.89 class nsSVGFilterReference final :
1:59.89 ^
1:59.89 /Volumes/Devel/Code/mozilla/bug602122/layout/generic/../svg/nsSVGEffects.h:46:1: note: Superclass 'nsSVGRenderingObserver' also has an mRefCnt member. Consider using the _INHERITED macros for AddRef and Release

So, there's some other subclass of nsSVGRenderingObserver that declares a ref count that is being fixed by the change.
https://bug602122.bugzilla.mozilla.org/attachment.cgi?id=8624942

Comment 9

[Daniel Holbert [:dholbert]]

Ah, thanks -- so nsSVGFilterReference has this line:
>  222   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
...which declares mRefCnt / AddRef / Release, on top of the ones provided by the nsSVGRenderingObserver superclass.

This line was added in this change:
 http://hg.mozilla.org/mozilla-central/diff/6c0b0d329a51/layout/svg/nsSVGEffects.h#l1.29
replacing  NS_DECL_ISUPPORTS_INHERITED, which [per "inherited" in the name] does not redeclare mRefCnt.

So, this is a regression from that bug (bug 1062832).  And unfortunately we do not have NS_DECL_CYCLE_COLLECTING_ISUPPORTS_INHERITED.

Comment 10

[Daniel Holbert [:dholbert]]

So the commit message here should really be something that better-explains what's happening & why, like: 

"Bug 1181323 - Move nsSVGRenderingObserver's isupports/refcounting decl to subclasses, since one subclass (nsSVGFilterReference) already has its own redundant copy of the decl."

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8631577 [details] [diff] [review]
Eliminate duplicate mRefCnt members in nsSVGRenderingObserver subclasses

r=me with comment 7 and comment 10 addressed.

Comment 12

[Andrew McCreight [:mccr8]]

(In reply to Daniel Holbert [:dholbert] from comment #9)
> So, this is a regression from that bug (bug 1062832).  And unfortunately we
> do not have NS_DECL_CYCLE_COLLECTING_ISUPPORTS_INHERITED.

I think we don't actually need NS_DECL_CYCLE_COLLECTING_ISUPPORTS_INHERITED. Instead, you can use NS_DECL_ISUPPORTS_INHERITED, because of virtual dtors. You need to use NS_IMPL_RELEASE_INHERITED instead of NS_IMPL_CYCLE_COLLECTING_RELEASE. Disclaimer: I did not realize most of this off hand.

Comment 13

[Andrew McCreight [:mccr8]]

Oh, looking at the code, the problem is that a cycle collecting class can't implement from a non-cycle-collecting class, due to having different refcounts. Instead, you have to just not implement refcounting in the parent class. Which is what the patch does. Kind of clumsy.

Comment 14

[Daniel Holbert [:dholbert]]

(Just to confirm: so the patch seems good to you then, yes?)

Comment 15

[Andrew McCreight [:mccr8]]

I didn't look at it in detail, but it sounds like it just moves the ISupports implementations to the subclasses, which sounds fine to me.

Comment 16

[Daniel Holbert [:dholbert]]

Cool, then my "r=me with comment 7 and comment 10 addressed" still stands.

(wanted to be sure I wasn't r+'ing despite some fundamental issue you'd uncovered)

Comment 17

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

Attachment: Move nsSVGRenderingObserver's isupports/refcounting decl to subclasses, since one subclass (nsSVGFilterReference) already has its own redundant copy of the decl

Fixing nits from Comment 7 and Comment 10

Comment 18

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

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5311b4831bf3

Comment 19

[Pulsebot]

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

Comment 20

[Carsten Book [:Tomcat]]

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