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

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Richard Marti (:Paenglab)]

Regressed from bug 1303302.

Backout of this bug let me fully build.

Comment 1

[:aceman]

Or let's try to fix what the new check found:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fdf6027b97ae999cdb4a1d2c9589408238705b75

Comment 2

[Frank-Rainer Grahl (:frg)]

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

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

I wouldn't consider revealing new defects to be a regression. And yeah, I think we should fix whatever this check finds.

Comment 4

[Frank-Rainer Grahl (:frg)]

Attachment: 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

[Jorg K (CEST = GMT+2)]

Let's get this landed today before branch day tomorrow :-)

Comment 6

[Richard Marti (:Paenglab)]

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.

Comment 7

[Frank-Rainer Grahl (:frg)]

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

Comment 8

[Jorg K (CEST = GMT+2)]

Oops, I was doing the same, I'll cancel mine.

Comment 9

[Frank-Rainer Grahl (:frg)]

No need. Working on something else anyway.

Comment 10

[Jorg K (CEST = GMT+2)]

(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?

Comment 11

[Frank-Rainer Grahl (:frg)]

Ok, sorry I misunderstood.

Comment 12

[Jorg K (CEST = GMT+2)]

Let's also change the commit message of the patch ;-)

Comment 13

[Frank-Rainer Grahl (:frg)]

Attachment: 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();

Comment 14

[Jorg K (CEST = GMT+2)]

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

Comment 15

[Frank-Rainer Grahl (:frg)]

>>  -    ~nsMAPISession();

My local compile just finished. Moving this one fails.

Comment 16

[Jorg K (CEST = GMT+2)]

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

[Jorg K (CEST = GMT+2)]

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

Comment 18

[Frank-Rainer Grahl (:frg)]

Attachment: 1303602-destructors-V2.patch

This one and the wip is ok for suite. Just my latest tweak was bad. Not a C++ person :)

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: FRG's patch + two issues from comment #16.

Comment 20

[Jorg K (CEST = GMT+2)]

Attachment: FRG's patch + two issues from comment #16.

Made white-space match FRG's patch exactly (I hope).

Comment 21

[Jorg K (CEST = GMT+2)]

Thanks Frank-Rainer, I had to add to fixes in mail/

Comment 22

[Richard Marti (:Paenglab)]

Attachment: 1303602-destructors-V3.patch

I had additionally to fix nsOutlookProfileMigrator.h.

This patch built to the end.

Comment 23

[Richard Marti (:Paenglab)]

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

Comment 24

[Frank-Rainer Grahl (:frg)]

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

[Jorg K (CEST = GMT+2)]

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

Comment 26

[Jorg K (CEST = GMT+2)]

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

Comment 27

[Richard Marti (:Paenglab)]

Try was successful.

https://hg.mozilla.org/comm-central/rev/23e9f9db0672

Comment 28

[Intermittent Failures Robot]

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