bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Eliminate duplicate mRefCnt members in nsSVGRenderingObserver subclasses

RESOLVED FIXED in Firefox 42

Status

()

Core
SVG
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Nika, Assigned: Nika)

Tracking

Trunk
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8630705 [details] [diff] [review]
Eliminate duplicate mRefCnt members in nsSVGRenderingObserver subclasses
(Assignee)

Updated

3 years ago
Assignee: nobody → michael
Blocks: 602122
(Assignee)

Updated

3 years ago
Attachment #8630705 - Flags: review?(jwatt)
I think the NS_DECL_ISUPPORTS should all be public.
(Assignee)

Comment 3

3 years ago
(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:
(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.
(Assignee)

Comment 6

3 years ago
Created attachment 8631577 [details] [diff] [review]
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.
Attachment #8630705 - Attachment is obsolete: true
Attachment #8630705 - Flags: review?(jwatt)
Attachment #8631577 - Flags: review?(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.)
Flags: needinfo?(michael)
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
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.
Blocks: 1062832
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."
Version: unspecified → Trunk
Comment on attachment 8631577 [details] [diff] [review]
Eliminate duplicate mRefCnt members in nsSVGRenderingObserver subclasses

r=me with comment 7 and comment 10 addressed.
Attachment #8631577 - Flags: review?(dholbert) → review+
Flags: needinfo?(michael)
(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.
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.
(Just to confirm: so the patch seems good to you then, yes?)
Flags: needinfo?(continuation)
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.
Flags: needinfo?(continuation)
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)
(Assignee)

Comment 17

3 years ago
Created attachment 8631965 [details] [diff] [review]
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
Attachment #8631577 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/d4fd9365fdc3
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.