Closed
Bug 1035650
Opened 10 years ago
Closed 10 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)
Thunderbird
General
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 33.0
People
(Reporter: ewong, Assigned: jcranmer)
References
Details
Attachments
(5 files)
14.16 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
29.56 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
4.39 KB,
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
181.52 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
19.43 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
+++ 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)
Assignee | ||
Comment 1•10 years ago
|
||
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 | ||
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8452865 -
Flags: review?(philipp)
Assignee | ||
Comment 4•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8452865 -
Flags: review?(philipp) → review+
Comment 6•10 years ago
|
||
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...
Updated•10 years ago
|
Attachment #8452826 -
Flags: review?(neil) → review+
Updated•10 years ago
|
Attachment #8452840 -
Flags: review?(neil) → review+
Updated•10 years ago
|
Attachment #8452868 -
Flags: review?(neil) → review+
Updated•10 years ago
|
Attachment #8452883 -
Flags: review?(neil) → review+
Assignee | ||
Comment 7•10 years ago
|
||
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: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 33.0
Assignee | ||
Comment 8•10 years ago
|
||
(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.
Reporter | ||
Comment 9•10 years ago
|
||
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 → ---
Reporter | ||
Comment 10•10 years ago
|
||
I'm mistaken. that's bug 1036619.
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•9 years ago
|
Flags: in-testsuite-
You need to log in
before you can comment on or make changes to this bug.
Description
•