Make destructors of ref-counted objects private in code only compiled on Windows

RESOLVED FIXED in Thunderbird 51.0

Status

MailNews Core
Address Book
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: Paenglab, Assigned: Frank-Rainer Grahl)

Tracking

({dogfood})

Thunderbird 51.0
All
Windows
dogfood

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

7 months ago
Regressed from bug 1303302.

Backout of this bug let me fully build.

Comment 1

7 months ago
Or let's try to fix what the new check found:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fdf6027b97ae999cdb4a1d2c9589408238705b75
OS: Unspecified → Windows
Hardware: Unspecified → All
Version: unspecified → 51
(Assignee)

Comment 2

7 months ago
These files need to be fixed. Not sure if just setting the destructors to private is ok and all which is needed.

mailnews\addrbook\src\nsAbOutlookDirectory.h
mailnews\addrbook\src\nsAbOutlookDirFactory.h
mailnews\mapi\mapihook\src\msgMapiFactory.h
mailnews\mapi\mapihook\src\msgMapiHook.cpp
mailnews\mapi\mapihook\src\msgMapiMain.h
mailnews\mapi\mapihook\src\msgMapiSupport.h
I wouldn't consider revealing new defects to be a regression. And yeah, I think we should fix whatever this check finds.
Keywords: regression
(Assignee)

Comment 4

7 months ago
Created attachment 8792300 [details] [diff] [review]
1303602-wip.patch

Well SeaMonkey compiles with the patch and doesn't go up in flames when running so if this one is ok feel free to use it as a base for a patch.

Comment 5

7 months ago
Let's get this landed today before branch day tomorrow :-)
(Reporter)

Comment 6

7 months ago
Comment on attachment 8792300 [details] [diff] [review]
1303602-wip.patch

Started https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f7b686e0d596 to check if this patch fixes it.
(Assignee)

Comment 7

7 months ago
If that really is all what is needed I can also formalize it. Just holler.

Comment 8

7 months ago
Oops, I was doing the same, I'll cancel mine.
(Assignee)

Comment 9

7 months ago
No need. Working on something else anyway.

Comment 10

7 months ago
(In reply to Frank-Rainer Grahl from comment #9)
> No need. Working on something else anyway.
I don't understand.
I was referring to Richard's comment about the try push. I did one push too and had to cancel it.
You're patch is ready for approval if everything compiles on try, right?
(Assignee)

Comment 11

7 months ago
Ok, sorry I misunderstood.

Comment 12

7 months ago
Let's also change the commit message of the patch ;-)
Summary: mailnews/addrbook/src/nsAbOutlookDirFactory.cpp(25): error C2338: Reference-counted class nsAbOutlookDirFactory should not have a public destructor. Make this class's destructor non-public → Make destructors of ref-counted objects private in code only compiled on Windows (mailnews/mapi and mailnews/addrbook/src/nsAbOutlook*.cpp)
(Assignee)

Comment 13

7 months ago
Created attachment 8792305 [details] [diff] [review]
1303602-destructors.patch

Proper patch.

I found an additional destructor in msgMapiMain.h and put it in the patch. Could you check this one too:

>>  -    ~nsMAPISession();
Attachment #8792300 - Attachment is obsolete: true

Comment 14

7 months ago
Needed some more treatment :-(
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=454a45e74f9e
(Assignee)

Comment 15

7 months ago
>>  -    ~nsMAPISession();

My local compile just finished. Moving this one fails.

Comment 16

7 months ago
Well, I fixed:

c:/builds/moz2_slave/tb-try-c-cen-w32-0000000000000/build/mail/components/shell/nsMailWinIntegration.cpp(49): error C2338: Reference-counted class nsWindowsShellService should not have a public destructor. Make this class's destructor non-public

c:/builds/moz2_slave/tb-try-c-cen-w32-0000000000000/build/mail/components/migration/src/nsOEProfileMigrator.cpp(17): error C2338: Reference-counted class nsOEProfileMigrator should not have a public destructor. Make this class's destructor non-public

Did you get a local compile error on nsMAPISession?

Comment 17

7 months ago
(In reply to Frank-Rainer Grahl from comment #15)
> My local compile just finished. Moving this one fails.
Meaning that patch is no good?

Updated

7 months ago
Summary: Make destructors of ref-counted objects private in code only compiled on Windows (mailnews/mapi and mailnews/addrbook/src/nsAbOutlook*.cpp) → Make destructors of ref-counted objects private in code only compiled on Windows
(Assignee)

Comment 18

7 months ago
Created attachment 8792309 [details] [diff] [review]
1303602-destructors-V2.patch

This one and the wip is ok for suite. Just my latest tweak was bad. Not a C++ person :)
Attachment #8792305 - Attachment is obsolete: true

Comment 19

7 months ago
Created attachment 8792310 [details] [diff] [review]
FRG's patch + two issues from comment #16.
Attachment #8792309 - Attachment is obsolete: true

Comment 20

7 months ago
Created attachment 8792311 [details] [diff] [review]
FRG's patch + two issues from comment #16.

Made white-space match FRG's patch exactly (I hope).
Attachment #8792310 - Attachment is obsolete: true

Comment 21

7 months ago
Thanks Frank-Rainer, I had to add to fixes in mail/
(Reporter)

Comment 22

7 months ago
Created attachment 8792314 [details] [diff] [review]
1303602-destructors-V3.patch

I had additionally to fix nsOutlookProfileMigrator.h.

This patch built to the end.
Attachment #8792311 - Attachment is obsolete: true
(Reporter)

Comment 23

7 months ago
Let's see what try says: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3c9b9a10cd1a
(Assignee)

Comment 24

7 months ago
>> Thanks Frank-Rainer, I had to add to fixes in mail/

No problem. Not here for taking the glory just to get it working :)

Comment 25

7 months ago
I think we let have FRG have the laurel here ;-)
Richard, thanks for adding one more. I don't have an up-to-date local build and it would cost me an hour (which I don't have) to produce one.

Thanks for saving resources on try, no need to compile on Linux and Mac if the code only compiles on Windows ;-) Also, it if compiles, it will work, no need to run the entire test suite on it.

Please land your patch if it compiles with this commit message:
Bug 1303602 - Declare destructors of ref-counted classes private in code only compiled on Windows. r=jorgk
Assignee: nobody → frgrahl

Comment 26

7 months ago
Comment on attachment 8792314 [details] [diff] [review]
1303602-destructors-V3.patch

Commit as:

Bug 1303602 - Declare destructors of ref-counted classes private in code only compiled on Windows. r=jorgk
Attachment #8792314 - Flags: review+
(Reporter)

Comment 27

7 months ago
Try was successful.

https://hg.mozilla.org/comm-central/rev/23e9f9db0672
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 51.0

Comment 28

7 months ago
16 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* comm-central: 16

Platform breakdown:
* windowsxp: 16

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1303602&startday=2016-09-12&endday=2016-09-18&tree=all
You need to log in before you can comment on or make changes to this bug.