Closed Bug 1035650 Opened 5 years ago Closed 5 years ago

Remove public destructors of NS_*_INLINE_* refcounted classes, Remove NS_HIDDEN, Fix dangerous public destructors. in mail/ and mailnews/

Categories

(Thunderbird :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 33.0

People

(Reporter: ewong, Assigned: jcranmer)

References

Details

Attachments

(5 files)

+++ This bug was initially created as a clone of Bug #1032800 +++

Port relevant bits from:

Bug 1015664 - Remove NS_HIDDEN usage in Gecko
Bug 1027251 - Remove public destructors of NS_*_INLINE_* refcounted classes, outside of a finite explicit whitelist
Bug 1028588 - Fix dangerous public destructors in miscellaneous places
Bug 758992 - Make the classes which use the XPCOM nsISupports implementation macros final, to avoid the warning about deleting using a pointer to a base class with virtual functions and no virtual dtor (browser parts)
I split this part out because it has some changes that aren't:

A. Moving a destructor to a private/protected block
B. Adding MOZ_FINAL
C. Adding a new, empty destructor
Assignee: nobody → Pidgeot18
Status: NEW → ASSIGNED
Attachment #8452826 - Flags: review?(neil)
Yay, more nontrivial stuff.

/me watches all his patches bitrot horribly.

The nsMsg* in the public are used in mimedrft.cpp. I looked at the failures for a few moments and said "nope, not touching these" So whitelist it is for them.
Attachment #8452840 - Flags: review?(neil)
Attached patch Part 3: CalendarSplinter Review
Attachment #8452865 - Flags: review?(philipp)
I can't promise this is everything, since my frame of mind was "build until next failure, fix" repeated ad nauseum. Probably need more in the import code and the mail migrators, but this should have two of four platforms building at least.
Attachment #8452868 - Flags: review?(neil)
Attached patch Part 5: The restSplinter Review
I hope.
Attachment #8452883 - Flags: review?(neil)
Attachment #8452865 - Flags: review?(philipp) → review+
A couple of general nits:
* Indentation on moved lines could have been fixed, or at least detabbed.
* There's a strange mix of protected or private and virtual or nonvirtual destructors with classes that are or are not marked final. The private virtual destructors just look wrong, and the final protected and/or virtual destructors feel wrong, although I suppose they would be OK on derived classes. The apparently erratic sprinkling of MOZ_FINAL just further confuses matters.

Now if only there was an easy way to mark r+ on four attachments at once...
Attachment #8452826 - Flags: review?(neil) → review+
Attachment #8452840 - Flags: review?(neil) → review+
Attachment #8452868 - Flags: review?(neil) → review+
Attachment #8452883 - Flags: review?(neil) → review+
https://hg.mozilla.org/comm-central/rev/42c4a83ef04c
https://hg.mozilla.org/comm-central/rev/bbefe71a3aea
https://hg.mozilla.org/comm-central/rev/8374fc73b34e
(I folded part 4 and part 5 when pushing, no logical sense for them to remain separate. Parts 1-2 are separate because they have some measure of non-triviality.)
https://hg.mozilla.org/comm-central/rev/21e13ac2fe1f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 33.0
(In reply to neil@parkwaycc.co.uk from comment #6)
> A couple of general nits:
> * Indentation on moved lines could have been fixed, or at least detabbed.
> * There's a strange mix of protected or private and virtual or nonvirtual
> destructors with classes that are or are not marked final. The private
> virtual destructors just look wrong, and the final protected and/or virtual
> destructors feel wrong, although I suppose they would be OK on derived
> classes. The apparently erratic sprinkling of MOZ_FINAL just further
> confuses matters.

I filed bug 1036411 to fix the private virtual destructor issue.
Missing a change here:

../../../mozilla/dist/include/nsError.h:205:3: note: expanded from macro 'NS_ERROR_GENERATE'
  (nsresult)(((uint32_t)(sev) << 31) | \
  ^
../../../../../mailnews/compose/src/nsSmtpProtocol.cpp:211:1: error: static_assert failed "Reference-counted class nsSmtpProtocol should not have a public destructor. Try to make this class's destructor non-public. If that is really not possible, you can whitelist this class by providing a HasDangerousPublicDestructor specialization for it."
NS_IMPL_ADDREF_INHERITED(nsSmtpProtocol, nsMsgAsyncWriteProtocol)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

(unless I'm mistaken?)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I'm mistaken.  that's bug 1036619.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.