Closed Bug 1403698 Opened 2 years ago Closed 2 years ago

Address delete-non-virtual-dtor warnings

Categories

(Core :: General, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Nick what do you think of this? Is adding 'virtual' to a destructor an always-safe choice?
Attachment #8912872 - Flags: review?(n.nethercote) → review?(nfroyd)
I confess to being hazy on this matter, so I will defer to froydnj!
Comment on attachment 8912872 [details]
Bug 1403698 Address delete-non-virtual-dtor warnings

https://reviewboard.mozilla.org/r/184202/#review189888

I don't have the text of the warning in front of me, but I think all of these affected objects are refcounted objects, and we have plenty of refcounted objects with non-virtual destructors (e.g. everything that inherits from `nsISupports`).  So we should be able to do some combination of:

1. Make the destructors private.  (Prevent random people calling the destructor, which is just good practice.)
2. Making the classes final.

Can you do that, and see if it addresses the problems?
Attachment #8912872 - Flags: review?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #5)
> Comment on attachment 8912872 [details]
> Bug 1403698 Address delete-non-virtual-dtor warnings
> 
> https://reviewboard.mozilla.org/r/184202/#review189888
> 
> I don't have the text of the warning in front of me, but I think all of
> these affected objects are refcounted objects, and we have plenty of
> refcounted objects with non-virtual destructors (e.g. everything that
> inherits from `nsISupports`).  So we should be able to do some combination
> of:
> 
> 1. Make the destructors private.  (Prevent random people calling the
> destructor, which is just good practice.)
> 2. Making the classes final.
> 
> Can you do that, and see if it addresses the problems?

Two of the classes already had private destructors. Marking the classes final (and removing the virtual) kept the error away.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfc6042816ddbfba45a4af14e7aec9569811ba70&selectedJob=133904857 (ignore the linking error)



I tried doing that to the remaining 6; but the build failed as making the destructor private on (at least one) class broke the build.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ea13d58148f9cc1da7d19ac98299dccfd00e62c&selectedJob=133922723



Here is a try run that does not address any of the in-progress warning cleanup and has all occurrences of this warning: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cf168528a59ba7adcfcb760ac326cc76f58894a&selectedJob=133906172
Ugh, OK.

Looking at your try run, PDFViaEMFPrintHelper in widget/windows/PDFViaEMFPrintHelper.h has three virtual functions, two of which don't need to be (OpenDocument variants) and one of which is virtual only so we can override it in a subclass in tests, which seems like poor design.  So we can't mark it final.  The class is used with UniquePtr, so we would need to engage in serious shenanigans to make the destructor private.

Sadly, the build didn't get any farther, so we don't know the results on the remaining classes.

For that class, we have two options:

- Mark the destructor virtual, as you did; or
- Try and remove the virtual function usage from that class, two of which are really easy, the other one being somewhat difficult.

I guess the first choice is the path of least resistance here...
That seems to have done it:

MinGW run (broken, but fixed these warnings): https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd0913969549e346c3e2c0642411336a6b6de0fc&selectedJob=134086034

If this is good, before I land this I'll ensure it passes a non-MinGW run also, but I don't have that queued yet.
Status: NEW → ASSIGNED
Comment on attachment 8912872 [details]
Bug 1403698 Address delete-non-virtual-dtor warnings

https://reviewboard.mozilla.org/r/184202/#review190722

Thanks, this seems much more palatable.  Questions below.

::: gfx/2d/NativeFontResourceDWrite.cpp:142
(Diff revision 2)
>  private:
>    std::vector<uint8_t> mData;
>    Atomic<uint32_t> mRefCnt;
>    uint64_t mFontFileKey;
> +
> +  virtual ~DWriteFontFileStream();

It is not clear to me why this must be virtual.  I guess COM requires virtual destructors?  But then the `AudioNotification` change, above, didn't require a virtual destructor, and AFAICT `AudioNotification` participates in COM too...so that leads me to believe that this can just be a normal destructor, no virtualness required.  Or did testing prove that this needed to be virtual?

::: gfx/layers/d3d11/TextureD3D11.cpp:76
(Diff revision 2)
>    }
>  private:
>    int mRefCnt;
>    int mMemoryUsed;
> +
> +  virtual ~TextureMemoryMeasurer() = default;

Same comment here about virtual dtors.

::: widget/windows/InkCollector.h:50
(Diff revision 2)
>    bool IsHardProximityTablet(IInkTablet* aTablet) const;
>  
>  private:
>    uint32_t  mRefCount = 0;
> +
> +  virtual ~InkCollectorEvent() = default;

And here.
Attachment #8912872 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #11)
> Comment on attachment 8912872 [details]
> Bug 1403698 Address delete-non-virtual-dtor warnings
> 
> https://reviewboard.mozilla.org/r/184202/#review190722
> 
> Thanks, this seems much more palatable.  Questions below.
> 
> ::: gfx/2d/NativeFontResourceDWrite.cpp:142
> (Diff revision 2)
> >  private:
> >    std::vector<uint8_t> mData;
> >    Atomic<uint32_t> mRefCnt;
> >    uint64_t mFontFileKey;
> > +
> > +  virtual ~DWriteFontFileStream();
> 
> It is not clear to me why this must be virtual.  I guess COM requires
> virtual destructors?  But then the `AudioNotification` change, above, didn't
> require a virtual destructor, and AFAICT `AudioNotification` participates in
> COM too...so that leads me to believe that this can just be a normal
> destructor, no virtualness required.  Or did testing prove that this needed
> to be virtual?
> 
> ::: gfx/layers/d3d11/TextureD3D11.cpp:76
> (Diff revision 2)
> >    }
> >  private:
> >    int mRefCnt;
> >    int mMemoryUsed;
> > +
> > +  virtual ~TextureMemoryMeasurer() = default;
> 
> Same comment here about virtual dtors.
> 
> ::: widget/windows/InkCollector.h:50
> (Diff revision 2)
> >    bool IsHardProximityTablet(IInkTablet* aTablet) const;
> >  
> >  private:
> >    uint32_t  mRefCount = 0;
> > +
> > +  virtual ~InkCollectorEvent() = default;
> 
> And here.



Testing indicated that of these three, it is needed on InkCollectorEvent, but not the other two.

Removed from all three: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bcd87530c46b091ce45f096c10b13b7b19921bb&selectedJob=134733734

Put it back on InkCollectorEvent: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdff9dba49bdfda735a745a10d18cc1187c8db0f&selectedJob=134742343
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7cc3f9fd01ac
Address delete-non-virtual-dtor warnings r=froydnj
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7cc3f9fd01ac
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.