Port Bug 1315812 | Mark nsISupportsArray, nsICollection, nsIEnumerator as deprecated| to Mailnews: error C2220: warning treated as error - warning C4996: 'nsICollection': was declared deprecated

RESOLVED FIXED in Thunderbird 52.0

Status

Thunderbird
General
--
blocker
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: Jorg K (GMT+2), Assigned: Jorg K (GMT+2))

Tracking

Trunk
Thunderbird 52.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 months ago
c:\builds\moz2_slave\tb-c-cen-w32-00000000000000000\build\objdir-tb\dist\include\nsICollection.h(58): error C2220: warning treated as error - no 'object' file generated
c:\builds\moz2_slave\tb-c-cen-w32-00000000000000000\build\objdir-tb\dist\include\nsICollection.h(58): warning C4996: 'nsICollection': was declared deprecated

...

c:/builds/moz2_slave/tb-c-cen-w32-00000000000000000/build/mozilla/config/rules.mk:951: recipe for target 'ImportOutFile.obj' failed
mozmake.exe[5]: Leaving directory 'c:/builds/moz2_slave/tb-c-cen-w32-00000000000000000/build/objdir-tb/mailnews/import/src'
c:/builds/moz2_slave/tb-c-cen-w32-00000000000000000/build/mozilla/config/recurse.mk:71: recipe for target 'mailnews/import/src/target' failed
mozmake.exe[5]: *** [ImportOutFile.obj] Error 2
mozmake.exe[4]: *** [mailnews/import/src/target] Error 2
mozmake.exe[4]: *** Waiting for unfinished jobs....

Comment 1

6 months ago
I've seen they mark this deprecated. I didn't expect it to break the build. OK, so we must fix it quickly.

Anyway, in bug 1315812 that did this, there is sample code for disabling the warning in specific files.

But we get more hosed if the same effect happens for nsISupportArray (which was marked deprecated too). That one is sprinkled all over the code due to filters using it.
Blocks: 1315812
(Assignee)

Comment 2

6 months ago
Anything that will keep us going with get an r+ from me especially that the other platforms are building and not stopping on the warning. The current situation makes no sense.

I thought nsISupportArray was mostly gone?

Comment 3

6 months ago
nsISupportsArray is mostly gone (but not totally, even m-c had to disable the warnings for some files), but there is a one big offender remaining, that is nsIMsgFilter::searchTerms . Due to that there are about hundred places that still reference nsISupportsArray. There is bug 857230 for it. It is a large patch/project but mostly mechanical conversion once we decide what to convert it to.
(Assignee)

Comment 4

6 months ago
M-C suppresses warnings, see here:
https://hg.mozilla.org/mozilla-central/rev/f424e29dc77e#l3.12
we should do the same.

> There is bug 857230 for it. It
> is a large patch/project but mostly mechanical conversion once we decide
> what to convert it to.
There is no leeway for a decision here. We *must* convert before M-C remove it completely.
Summary: Windows build bustate 2016-11-12: error C2220: warning treated as error - warning C4996: 'nsICollection': was declared deprecated → Port Bug 1315812 | Mark nsISupportsArray, nsICollection, nsIEnumerator as deprecated| to Mailnews: error C2220: warning treated as error - warning C4996: 'nsICollection': was declared deprecated

Comment 5

6 months ago
The question is whether that is "before m-c remove it completely" OR "now, because it is already marked deprecated and causes build failure".

I have cleaned out nsIEnumerator in the past so that is done.
I do not see us referencing nsICollection anywhere either.

We use nsISupportsArray which may include nsICollection internally. I do not yet see why ImportOutFile.cpp fails, as it does not reference either one directly. May be due to the inclusion of nsMsgUtils.cpp which does include nsISupportsArray.

c:\builds\moz2_slave\tb-c-cen-w32-00000000000000000\build\objdir-tb\dist\include\nsISupportsArray.h(38): warning C4996: 'nsICollection': was declared deprecated
c:\builds\moz2_slave\tb-c-cen-w32-00000000000000000\build\objdir-tb\dist\include\nsICollection.h(27): note: see declaration of 'nsICollection'
c:\builds\moz2_slave\tb-c-cen-w32-00000000000000000\build\objdir-tb\dist\include\nsISupportsArray.h(66): warning C4996: 'nsISupportsArray': was declared deprecated
c:\builds\moz2_slave\tb-c-cen-w32-00000000000000000\build\objdir-tb\dist\include\nsISupportsArray.h(38): note: see declaration of 'nsISupportsArray'
mozmake.exe[5]: *** [ImportOutFile.obj] Error 2
mozmake.exe[4]: *** [mailnews/import/src/target] Error 2
Severity: normal → blocker
OS: Unspecified → All
Hardware: Unspecified → All
(Assignee)

Comment 6

6 months ago
Is there more to it than wrapping all
https://dxr.mozilla.org/comm-central/search?q=nsISupportsArray.h&redirect=false
into this:

https://hg.mozilla.org/mozilla-central/rev/f424e29dc77e#l3.12

+// Disable deprecation warnings generated by nsISupportsArray and associated
+// classes.
+#if defined(__GNUC__)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
+#elif defined(_MSC_VER)
+#pragma warning (push)
+#pragma warning (disable : 4996)
+#endif
+
 #include "nsSupportsArray.h"
+
+#if defined(__GNUC__)
+#pragma GCC diagnostic pop
+#elif defined(_MSC_VER)
+#pragma warning (pop)
+#endif
+
(Assignee)

Comment 7

6 months ago
I won't have time this afternoon. The person trying this approach, please register here so we don't waste time with two people addressing this at the same time.
Duplicate of this bug: 1317052
(Assignee)

Comment 9

6 months ago
OK, I'll do it I have 30 minutes to edit 11 files ;-)
(Assignee)

Comment 10

6 months ago
Created attachment 8810079 [details] [diff] [review]
1317040.patch

Try run here:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=147da4f6414c3850d57e0e1ce9fd1a43db1f2c8b

I suggest that anyone with Level 3 rights can land this if the try turns out green. It's "pre-approved" with rs=bustage-fix ;-)

My 30 minutes are up ;-)
Assignee: nobody → jorgk
Status: NEW → ASSIGNED

Comment 11

6 months ago
I cannot work on this until tomorrow evening.
But I can promise to give review here if anybody makes a patch.

I'm fine if you choose to just hide the warnings, if it is only 11 files.

We will still need to get traction in bug 857230, but this temp solution will win us time to do it after 52. I'm willing to work on that one, after there is agreement on the API changes.
Blocks: 857230
(Assignee)

Comment 12

6 months ago
Comment on attachment 8810079 [details] [diff] [review]
1317040.patch

;-)
Attachment #8810079 - Flags: review?(acelists)
(Assignee)

Comment 13

6 months ago
Comment on attachment 8810079 [details] [diff] [review]
1317040.patch

Didn't work :-(
Attachment #8810079 - Flags: review?(acelists)
(Assignee)

Comment 14

6 months ago
Maybe wrap this as well:
mailnews/base/util/nsMsgUtils.h
38 class nsISupportsArray;
(I'm not at my desk.)
(In reply to Jorg K (GMT+1) from comment #14)
> Maybe wrap this as well:
> mailnews/base/util/nsMsgUtils.h
> 38 class nsISupportsArray;
> (I'm not at my desk.)

Wrapped it there. Let's see if this helps.

https://hg.mozilla.org/try-comm-central/rev/91947445af7ed08842207b83933817902c1f1147
(Assignee)

Comment 16

6 months ago
I cancelled that. Let me try something else now.
(Assignee)

Comment 17

6 months ago
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f64b2d92e7971d0eab071e122195d0f2f8ff0a4f

I wrapped the whole file from the include to the end. That should work ;-)
(Assignee)

Comment 18

6 months ago
Actually, it's not 11 files that need fixing but 30:

mailnews/base/search/public/nsMsgSearchAdapter.h
mailnews/base/search/public/nsMsgSearchScopeTerm.h
mailnews/base/search/src/nsMsgFilter.cpp
mailnews/base/search/src/nsMsgFilter.h
mailnews/base/search/src/nsMsgFilterList.cpp
mailnews/base/search/src/nsMsgFilterService.cpp
mailnews/base/search/src/nsMsgImapSearch.cpp
mailnews/base/search/src/nsMsgLocalSearch.cpp
mailnews/base/search/src/nsMsgLocalSearch.h
mailnews/base/search/src/nsMsgSearchAdapter.cpp
mailnews/base/search/src/nsMsgSearchImap.h
mailnews/base/search/src/nsMsgSearchNews.cpp
mailnews/base/search/src/nsMsgSearchNews.h
mailnews/base/search/src/nsMsgSearchSession.cpp
mailnews/base/search/src/nsMsgSearchSession.h
mailnews/base/search/src/nsMsgSearchTerm.cpp
mailnews/base/src/nsMsgAccountManager.cpp
mailnews/base/src/nsMsgFolderDataSource.cpp
mailnews/base/src/nsMsgFolderDataSource.h
mailnews/base/src/nsMsgXFVirtualFolderDBView.cpp
mailnews/base/src/nsSubscribeDataSource.cpp
mailnews/base/src/virtualFolderWrapper.js
mailnews/base/test/unit/test_iteratorUtils.js
mailnews/base/util/iteratorUtils.jsm
mailnews/base/util/nsMsgIncomingServer.cpp
mailnews/base/util/nsMsgUtils.cpp
mailnews/base/util/nsMsgUtils.h
mailnews/extensions/mailviews/src/nsMsgMailViewList.cpp
mailnews/extensions/mailviews/src/nsMsgMailViewList.h
mailnews/imap/src/nsImapMailFolder.cpp
(Assignee)

Comment 19

6 months ago
Oops, need to remove the .js* from the list:

mailnews/base/search/public/nsMsgSearchAdapter.h
mailnews/base/search/public/nsMsgSearchScopeTerm.h
mailnews/base/search/src/nsMsgFilter.cpp
mailnews/base/search/src/nsMsgFilter.h
mailnews/base/search/src/nsMsgFilterList.cpp
mailnews/base/search/src/nsMsgFilterService.cpp
mailnews/base/search/src/nsMsgImapSearch.cpp
mailnews/base/search/src/nsMsgLocalSearch.cpp
mailnews/base/search/src/nsMsgLocalSearch.h
mailnews/base/search/src/nsMsgSearchAdapter.cpp
mailnews/base/search/src/nsMsgSearchImap.h
mailnews/base/search/src/nsMsgSearchNews.cpp
mailnews/base/search/src/nsMsgSearchNews.h
mailnews/base/search/src/nsMsgSearchSession.cpp
mailnews/base/search/src/nsMsgSearchSession.h
mailnews/base/search/src/nsMsgSearchTerm.cpp
mailnews/base/src/nsMsgAccountManager.cpp
mailnews/base/src/nsMsgFolderDataSource.cpp
mailnews/base/src/nsMsgFolderDataSource.h
mailnews/base/src/nsMsgXFVirtualFolderDBView.cpp
mailnews/base/src/nsSubscribeDataSource.cpp
mailnews/base/util/nsMsgIncomingServer.cpp
mailnews/base/util/nsMsgUtils.cpp
mailnews/base/util/nsMsgUtils.h
mailnews/extensions/mailviews/src/nsMsgMailViewList.cpp
mailnews/extensions/mailviews/src/nsMsgMailViewList.h
mailnews/imap/src/nsImapMailFolder.cpp

but need to add these:

mailnews/base/search/public/nsIMsgFilter.idl
mailnews/base/search/public/nsIMsgSearchSession.idl
mailnews/base/search/public/nsIMsgSearchValidityTable.idl
mailnews/extensions/mailviews/public/nsIMsgMailView.idl
(Assignee)

Comment 20

6 months ago
This is a bottom-less pit.

nsISupportsArray.h is included directly by 11 files.
nsIMsgFilter.h included directly by 10 files.
nsIMsgSearchSession.h is included directly by 19 files.
nsIMsgSearchValidityTable.h is included directly by 1 file.
nsIMsgMailView.h is not included anywhere but
nsIMsgMailView.idl is included in nsIMsgMailViewList.idl and
nsIMsgMailViewList.h is included in 1 file.

So far I've only done the first 11 files, and they need to be done differently, the pop at the end needs to go otherwise any of the files which include any of the header files amongst the 11 file will need the same treatment since the "warning disable" was popped.

Nice job two days before the branch day :-(
(Assignee)

Comment 21

6 months ago
Created attachment 8810133 [details] [diff] [review]
1317040.patch (v2).

OK, fixing 11 .h and .cpp files which include nsISupportsArray.h and also 4 .idl files which include nsISupportsArray.idl or use interface nsISupportsArray.

No pop at the end so the warning will be disabled to the end of the respective files or any file that includes them or any .h generated by the .idl files.

See how that goes:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4f8e6ae960be3bdc23814e61b92b9429947ce465
Attachment #8810079 - Attachment is obsolete: true
(Assignee)

Comment 22

6 months ago
This still didn't work since the IDL processor turns this:
%{C++
#if defined(__GNUC__)
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
#elif defined(_MSC_VER)
#pragma warning (disable : 4996)
#endif
%}
#include "nsISupportsArray.idl"


into this:
#ifndef __gen_nsISupportsArray_h__
#include "nsISupportsArray.h"
#endif

/* For IDL files that don't want to include root IDL files. */
#ifndef NS_NO_VTABLE
#define NS_NO_VTABLE
#endif
#if defined(__GNUC__)
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
#elif defined(_MSC_VER)
#pragma warning (disable : 4996)
#endif

turning the order around. Grrrrrr.

Looks like I can't win at 3 AM.
(Assignee)

Comment 23

6 months ago
Another variation:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=49338bc87ba99d9d97e5e887355f30235de135a8
(Assignee)

Comment 24

6 months ago
Created attachment 8810155 [details] [diff] [review]
1317040.patch (v3).

This worked.
Attachment #8810133 - Attachment is obsolete: true
(Assignee)

Comment 25

6 months ago
https://hg.mozilla.org/comm-central/rev/330fade4b94b549426eed14f6f9b44f65b7ef858
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 52.0

Comment 26

6 months ago
Comment on attachment 8810155 [details] [diff] [review]
1317040.patch (v3).

Review of attachment 8810155 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks good for now.
But it will mask other depreciations in the future, so we should get rid of these exceptions soon (in the other bug).
Attachment #8810155 - Flags: review+
(Assignee)

Comment 27

6 months ago
I love post landing reviews especially when they are positive ;-)

You are right this will mask other depreciations. The M-C approach I've seen was

warning push
warning disable
... some code
warning pop.

I tried to adopt this in a hurry, but it failed. Surely, if you do this in a .h file, all the .cpp which include the .h will NOT have the warning disabled. So this approach potentially only makes sense in .cpp files. There however, it also doesn't make sense. Why should I pop the warning at the end of the source file? Maybe to cater for multiple source files to be glued together to a compile unit? Maybe that's why M-C do it.

The patch treated 15 files: 4 .idl, 4 .h and 7 .cpp. If you want, I can see whether I can improve the .cpp files based on the M-C model described above. Most likely for little benefit.

What this bug has taught us is that we are in deep trouble as far as nsISupportsArray is concerned. It's all over the place, see comment #19, and the day it's removed from M-C we're busted big time.
You need to log in before you can comment on or make changes to this bug.