1403698 - Address delete-non-virtual-dtor warnings

Status: RESOLVED FIXED

(Core :: General, enhancement)

Description

[Tom Ritter [:tjr]]

https://www.securecoding.cert.org/confluence/display/cplusplus/OOP52-CPP.+Do+not+delete+a+polymorphic+object+without+a+virtual+destructor

Comment 1

[Tom Ritter [:tjr]]

Attachment: Bug 1403698 Address delete-non-virtual-dtor warnings

Review commit: https://reviewboard.mozilla.org/r/184202/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184202/

Comment 2

[Tom Ritter [:tjr]]

MinGW run that should show no warnings of this type (but break because of an unrelated bug): https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf4cc39db88971c37c140d99dd5466cc6aa467ef

Try run of all the other platforms: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d8a21475ceed437e48d0eb5f537a6c7db5f1668

Comment 3

[Tom Ritter [:tjr]]

Nick what do you think of this? Is adding 'virtual' to a destructor an always-safe choice?

Comment 4

[Nicholas Nethercote [inactive]]

I confess to being hazy on this matter, so I will defer to froydnj!

Comment 5

[Nathan Froyd [: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?

Comment 6

[Tom Ritter [:tjr]]

(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

Comment 7

[Nathan Froyd [:froydnj]]

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...

Comment 8

[Tom Ritter [:tjr]]

Comment on attachment 8912872 [details]
Bug 1403698 Address delete-non-virtual-dtor warnings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184202/diff/1-2/

Comment 9

[Tom Ritter [:tjr]]

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.

Comment 10

[Tom Ritter [:tjr]]

Vanilla Try Run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d72a1136d3abd9eb88cbe918fb9375143da96c8

Comment 11

[Nathan Froyd [:froydnj]]

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.

Comment 12

[Tom Ritter [:tjr]]

Comment on attachment 8912872 [details]
Bug 1403698 Address delete-non-virtual-dtor warnings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184202/diff/2-3/

Comment 13

[Tom Ritter [:tjr]]

(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

Comment 14

[Tom Ritter [:tjr]]

Comment on attachment 8912872 [details]
Bug 1403698 Address delete-non-virtual-dtor warnings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184202/diff/3-4/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7cc3f9fd01ac
Address delete-non-virtual-dtor warnings r=froydnj

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7cc3f9fd01ac