801383 - Build is failing in comm-central MailNews code since m-c Bug 779473

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[Ian Neal]

Recent builds on comm-central are failing in the mork code:
../../../../db/mork/src/morkEnv.cpp: In member function ‘void morkEnv::NewErrorAndCode(const char*, mork_u2)’:
../../../../db/mork/src/morkEnv.cpp:363:59: error: operands to ?: have different types ‘mork_u2’ and ‘tag_nsresult’
../../../../db/mork/src/morkEnv.cpp: In member function ‘void morkEnv::NewError(const char*)’:
../../../../db/mork/src/morkEnv.cpp:375:30: error: cannot convert ‘tag_nsresult’ to ‘mork_u4’ in assignment
../../../../db/mork/src/morkEnv.cpp: In member function ‘virtual nsresult morkEnv::GetHeap(nsIMdbHeap**)’:
../../../../db/mork/src/morkEnv.cpp:592:48: error: no match for ‘operator==’ in ‘heap->nsIMdbHeap::HeapAddStrongRef((&((morkEnv*)this)->morkEnv::<anonymous>)) == 0’

This appears have started landing since the most recent merge of mozilla-inbound / mozilla-central at Sat Oct 13 16:26:43
d750d39139d1	Ryan VanderMeulen — Merge the last PGO-green inbound changeset to m-c.

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

This is because of the enum class change, not the enum change.  It should be relatively easy to fix -- don't try to implicitly convert other things to an nsresult.  As a quick workaround, you can just add appropriate static_cast<>'s everywhere to the expected type while you work on proper fixes, if there are too many to properly fix right away.

Comment 3

[Philip Chee]

c:\t1\hg\comm-central\config\rules.mk:1195:0$ c:/DEV/mozilla-build/python/python2.6.exe -O c:/t1/hg/
comm-central/mozilla/build/cl.py cl -FonsMsgSend.obj -c -D_HAS_EXCEPTIONS=0 -I../../../mozilla/dist/
stl_wrappers  -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GF
X -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DZLIB_INTERNAL -DMOZ_SUITE=1 -DOSTYP
E=\"WINNT6.1\" -DOSARCH=WINNT  -Ic:/t1/hg/comm-central/mailnews/compose/src -I. -I../../../mozilla/d
ist/include -I../../../mozilla/dist/include/nsprpub  -Ic:/t1/hg/objdir-sm/mozilla/dist/include/nspr
-Ic:/t1/hg/objdir-sm/mozilla/dist/include/nss        -TP -nologo -W3 -Gy -Fdgenerated.pdb -wd4800 -G
R-  -DNDEBUG -DTRIMMED -Zi -Zi -UDEBUG -DNDEBUG -O1 -Oy -MD            -FI ../../../comm-config.h -D
MOZILLA_CLIENT c:/t1/hg/comm-central/mailnews/compose/src/nsMsgSend.cpp
nsMsgSend.cpp
c:/t1/hg/comm-central/mailnews/compose/src/nsMsgSend.cpp(3779) : error C2664: 'NS_ERROR_GET_CODE' :
cannot convert parameter 1 from 'int32_t' to 'nsresult'
        Conversion to enumeration type requires an explicit cast (static_cast, C-style cast or function-style cast)

Comment 4

[Ian Neal]

Attachment: [checked in] Fix nsMsgSend

Not tested as yet.

Comment 5

[Ian Neal]

Attachment: Fix nsMsgPrompts with static cast

Comment 6

[Ian Neal]

(In reply to Ian Neal from comment #5)
> Created attachment 671206 [details] [diff] [review]
> Fix nsMsgPrompts with static cast

To fix without static cast, all the mailnews NS_ERROR type defines would probably have to be converted to nsresult along the lines of nsErrorList.h I guess.

Comment 7

[Philip Chee]

Note: static_cast probably needed here:
http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsAbLDAPAutoCompFormatter.cpp#222

Comment 8

[Ian Neal]

Attachment: Fix nsMsgPrompts without static cast

This version doesn't use a static cast, instead it changes the relevant defines to provide nsresults.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 671198 [details] [diff] [review]
[checked in] Fix nsMsgSend

Looks right.

Comment 10

[[e-mail]]

Comment on attachment 671229 [details] [diff] [review]
Fix nsMsgPrompts without static cast

> #define SMTP_DELIV_MAIL                             12521
Anyone think it might be a good idea to follow up with all of these to make all of these nsresults and then have the consumer extract the code, or seeing as we already have a weird mixture, it makes no difference to add a few more into the mix?

Actually thinking about it we should probably switch these to string IDs.

>-#define NS_MSG_FOLLOWUPTO_ALERT                     12564
>+#define NS_MSG_FOLLOWUPTO_ALERT                     NS_MSG_GENERATE_FAILURE(12564)
I was in two minds over some of these as to whether they should be SUCCESS or FAILURE...

>-  bundle->GetStringFromID(NS_IS_MSG_ERROR(msgID) ? NS_ERROR_GET_CODE(msgID) : msgID, getter_Copies(msg));
>+  nsMsgGetMessageByID(msgID, msg);
Apart from this change I think I ended up making all the same other changes locally. (For this one I had just changed it to unconditionally get the code.)

Comment 11

[Mark Banner (:standard8)]

(In reply to [e-mail] from comment #10)
> Comment on attachment 671229 [details] [diff] [review]
> Fix nsMsgPrompts without static cast
> 
> > #define SMTP_DELIV_MAIL                             12521
> Anyone think it might be a good idea to follow up with all of these to make
> all of these nsresults and then have the consumer extract the code, or
> seeing as we already have a weird mixture, it makes no difference to add a
> few more into the mix?

Yes, I think we should make all of them nsresults in a follow-up.

> Actually thinking about it we should probably switch these to string IDs.

I think we need to keep some of the nsresult values, but change the string IDs themselves from "12564" to "msgFollowuptoAlert" (or something like).

> >-#define NS_MSG_FOLLOWUPTO_ALERT                     12564
> >+#define NS_MSG_FOLLOWUPTO_ALERT                     NS_MSG_GENERATE_FAILURE(12564)
> I was in two minds over some of these as to whether they should be SUCCESS
> or FAILURE...

Urgh, yeah, that's definitely a string-only ID.

Comment 12

[Mark Banner (:standard8)]

Attachment: [checked in] Fix nsMsgPrompts without static cast v2

Ok, I've taken Ian's patch and updated it with two things:

- More changes to make mailnews/compose compile on Mac.
- For NS_MSG_FOLLOWUPTO_ALERT change this string to use a string name rather than an ID, and provide appropriate functions which we can use for future additional replacements.

Comment 13

[Mark Banner (:standard8)]

Attachment: [checked in] Get db/mork compiling

This is what it takes to get db/mork compiling on Mac. There's various inconsistencies here like what AddWeakRef etc return, but I think that's a future clean up if they are needed at all.

Note that HeapAddStrongRef, HeapCutStrongRef and morkEnv::NewErrorAndCode are all unused functions.

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 671198 [details] [diff] [review]
[checked in] Fix nsMsgSend

Checked in: https://hg.mozilla.org/comm-central/rev/af0697548fb2

Comment 15

[[e-mail]]

Comment on attachment 671365 [details] [diff] [review]
[checked in] Fix nsMsgPrompts without static cast v2

>-      bundle->GetStringFromID(NS_MSG_FAILURE_ON_OBJ_EMBED_WHILE_SENDING, getter_Copies(msg));
>+      bundle->GetStringFromID(NS_ERROR_GET_CODE(NS_MSG_FAILURE_ON_OBJ_EMBED_WHILE_SENDING), getter_Copies(msg));
As this is the only use of NS_MSG_FAILURE_ON_OBJ_EMBED_WHILE_SENDING we should change it to be a string ID instead of an nsresult which would make it clearer especially for any subsequent string name conversion. r=me with that fixed.

>-      if (SendQuit(SMTP_ERROR_DONE) < 0)
>+      if (NS_FAILED(SendQuit(SMTP_ERROR_DONE)))
Eek! Does this need to land on aurora, as it's basically a regression fix from bug 779130? (nsresult is unsigned to the check wasn't working.)

Comment 16

[Neil Lee [:neilio]]

Guys, please make sure you're flagging the correct person for review.

Comment 17

[:Irving Reid (No longer working on Firefox)]

(In reply to [e-mail] from comment #15)
> Comment on attachment 671365 [details] [diff] [review]

> >-      if (SendQuit(SMTP_ERROR_DONE) < 0)
> >+      if (NS_FAILED(SendQuit(SMTP_ERROR_DONE)))
> Eek! Does this need to land on aurora, as it's basically a regression fix
> from bug 779130? (nsresult is unsigned to the check wasn't working.)

Good catch; we should back port this - it also overlaps bug 780124.

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 671365 [details] [diff] [review]
[checked in] Fix nsMsgPrompts without static cast v2

https://hg.mozilla.org/comm-central/rev/b9df7f0be3cd

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Attachment: Partial patch to fix OSX and Linux builds...

So I started working on cleaning up some more of our nsresult abusage.

It turns out that I'm not 100% familiar with how a lot of our back-end works. So I'm kind of wary of messing with this stuff.

This patch fixes some of the issues I felt comfortable fixing, but somebody who's more familiar with our backend should probably take the ball here.

Comment 20

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 671457 [details] [diff] [review]
Partial patch to fix OSX and Linux builds...

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

Other than removing the NS_ENSURE_SUCCESS noted below, looks good.

::: mailnews/addrbook/src/nsAddrDatabase.cpp
@@ +624,1 @@
>    return ret;

All the functions we call above *should* be returning valid nsresult values now (I looked at a few, and they seem OK). We shouldn't need the NS_ENSURE_SUCCESS macro here, we can just return ret.

Comment 21

[[e-mail]]

Comment on attachment 671368 [details] [diff] [review]
[checked in] Get db/mork compiling

>-  mEnv_ErrorCode = morkEnv_kGenericError;
>+  mEnv_ErrorCode = (mork_u4)morkEnv_kGenericError;
[Ideally you would make mEnv_ErrorCode an mdb_err instead.]

Comment 22

[Mark Banner (:standard8)]

(In reply to [e-mail] from comment #21)
> Comment on attachment 671368 [details] [diff] [review]
> Get db/mork compiling
> 
> >-  mEnv_ErrorCode = morkEnv_kGenericError;
> >+  mEnv_ErrorCode = (mork_u4)morkEnv_kGenericError;
> [Ideally you would make mEnv_ErrorCode an mdb_err instead.]

Filed bug 801718.

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 671368 [details] [diff] [review]
[checked in] Get db/mork compiling

https://hg.mozilla.org/comm-central/rev/eac22e3d7ba3

Comment 24

[:aceman]

Is anybody fixing these?
mailnews/base/util/nsMsgKeySet.cpp: In member function 'int nsMsgKeySet::Add(int32_t)':
mailnews/base/util/nsMsgKeySet.cpp:712:24: error: cannot convert 'tag_nsresult' to 'int' in return
mailnews/base/util/nsMsgKeySet.cpp: In member function 'int nsMsgKeySet::Remove(int32_t)':
mailnews/base/util/nsMsgKeySet.cpp:829:41: error: cannot convert 'tag_nsresult' to 'int' in return
mailnews/base/util/nsMsgKeySet.cpp: In member function 'int nsMsgKeySet::AddRange(int32_t, int32_t)':
mailnews/base/util/nsMsgKeySet.cpp:929:30: error: cannot convert 'tag_nsresult' to 'int' in return

mailnews/base/util/nsMsgLineBuffer.cpp: In member function 'int32_t nsMsgLineBuffer::ConvertAndSendBuffer()':
mailnews/base/util/nsMsgLineBuffer.cpp:233:85: error: cannot convert 'nsresult {aka tag_nsresult}' to 'int32_t {aka int}' in return
mailnews/base/util/nsMsgLineBuffer.cpp: In member function 'virtual int32_t nsMsgLineBuffer::FlushLastLine()':
mailnews/base/util/nsMsgLineBuffer.cpp:242:85: error: cannot convert 'nsresult {aka tag_nsresult}' to 'int32_t {aka int}' in return
mailnews/base/util/nsMsgLineBuffer.cpp:245:1: error: control reaches end of non-void function [-Werror=return-type]
mailnews/base/util/nsMsgLineBuffer.cpp: In member function 'int32_t nsMsgLineBuffer::ConvertAndSendBuffer()':
mailnews/base/util/nsMsgLineBuffer.cpp:234:1: error: control reaches end of non-void function [-Werror=return-type]

etc.

Comment 25

[:Irving Reid (No longer working on Firefox)]

Attachment: WIP: fix nsresult enum class bustage under Clang

This began with mconley's patch and has covered many more files. I need to step out of the office so I wanted to get my WIP up.

There are still compile errors to fix, and after the compile errors are fixed we probably also want to clean up the following...


Sweep for == NS_OK and != NS_OK and replace with NS_SUCCEEDED / NS_FAILED

Go through Pop3 and NNTP and replace -1 returns with more descriptive #defined error codes where definitions exist

Comment 26

[:aceman]

(In reply to Irving Reid (:irving) from comment #25)
> Sweep for == NS_OK and != NS_OK and replace with NS_SUCCEEDED / NS_FAILED
If this does not prevent building and can be done after this bug is closed then I can do it, just file the bug and assign it to me.

Comment 27

[Ian Neal]

(In reply to Irving Reid (:irving) from comment #25)
> Created attachment 671571 [details] [diff] [review]
> WIP: fix nsresult enum class bustage under Clang
> 
I presume nsNntpIncomingServer.cpp has yet to be looked at as that has a HandleLine too?

Comment 28

[Ian Neal]

Comment on attachment 671571 [details] [diff] [review]
WIP: fix nsresult enum class bustage under Clang

>+++ b/mailnews/addrbook/src/nsAbLDAPAutoCompFormatter.cpp
>@@ -214,17 +214,17 @@ nsAbLDAPAutoCompFormatter::FormatExcepti

>+        errCodeNum.AppendInt(static_cast<uint32_t>aErrorCode, 16);    
Missing () round aErrorCode

Comment 29

[Mark Banner (:standard8)]

Comment on attachment 671571 [details] [diff] [review]
WIP: fix nsresult enum class bustage under Clang

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

This is looking reasonable, I'd almost give it r+ except for wanting to look at a couple of files a bit more when I'm more awake. I've not tested it as I'm assuming there's more to come.

::: mailnews/local/src/nsPop3Protocol.cpp
@@ +1331,2 @@
>    }
>    else

nit: we can drop the else here as we're returning early.

Comment 30

[Ian Neal]

Attachment: Updated WIP patch

Changes since Irving's patch:
* Fixed Mark's comment
* Corrected a couple of typos
* Added fixes to more files

Build completes locally with this patch now, I can start the application but no thorough testing done.

Comment 31

[Joshua Cranmer [:jcranmer]]

Egads! On the nsNNTPProtocol.cpp, it's far better to change all of the int32_t methods called by ProcessProtocolState to nsresults instead of changing all the defines to be int32_t--it's what I want to do in the long run anyways (change -1 to NS_ERROR_FAILURE), and I don't think it'd be substantively more lines of changes

Comment 32

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 671677 [details] [diff] [review]
Updated WIP patch

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

I have a couple more fixes so I'll include the changes I mention below and put a new version of the patch up

::: mailnews/addrbook/src/nsAbLDAPAutoCompFormatter.cpp
@@ +218,5 @@
>  
>          // put the entire nsresult in string form
>          //
>          errCodeNum.AppendLiteral("0x");
> +        errCodeNum.AppendInt(static_cast<uint32_t>(aErrorCode), 16);    

Trailing white space

::: mailnews/news/src/nsNntpIncomingServer.cpp
@@ +1208,4 @@
>  nsNntpIncomingServer::HandleLine(const char* line, uint32_t line_size)
>  {
>    NS_ASSERTION(line, "line is null");
> +  if (!line) return NS_OK;

Return on its own line

@@ +1212,3 @@
>  
>    // skip blank lines and comments
> +  if (line[0] == '#' || line[0] == '\0') return NS_OK;

Return on its own line

Comment 33

[:Irving Reid (No longer working on Firefox)]

Attachment: [checked in] Fix clang build errors caused by nsresult enum class - v3

Fix the last of the clang build errors (and a couple of warnings). Still fails a lot of xpcshell tests, and could use a review of compile warning messages - I only paid thorough attention to errors.

Comment 34

[Joshua Cranmer [:jcranmer]]

Attachment: [checked in] Fix all errors in nsNNTPProtocol

This replaces the nsNNTPProtocol-relevant portions of the last patch to make the entire state machine use nsresults instead of int32_t, combined with a few style changes locally (nsNNTPProtocol is in horrible shape, stylistically).

Basic rules of thumb:
1. AlertError ignores its first parameter, so the one place that calls it with an nsresult got a 0 shoved in instead because I didn't want to mutilate another dozen callsites.

2. nsresult rv's added and used where appropriate, as is NS_ENSURE_SUCCESS

3. nsMsgLineBuffer is an annoying interface. Anywhere where we did "return status" where status is passed in as the second parameter to the ReadLine function there becomes a return rv, where rv is passed in the appropriate parameter. The "status" is really "how many bytes are in the line", which should be -1 only when the return value gets set to some error code (I've read it three times, I think that's what happens...).

4. MK_NNTP_LOADED is equivalent to NS_OK (it's #define'd 1 IIRC).

5. All of the other MK_NNTP_* stuff should be negative values; these (and -1 returns) all became NS_ERROR_FAILURE (or rv if one looked more appropriate).

6. I gave up with renaming the status variable in ProcessProtocolState. It's really an rv, but there's too many uses of it. All other nsresults should be named rv, though.

... I guess I forgot about how much cleaning up this code needed.

I'm currently attempting to test these changes to see if they pass...

Comment 35

[Joshua Cranmer [:jcranmer]]

Confirmation: all NNTP tests pass with my change.

Comment 36

[Mark Banner (:standard8)]

Comment on attachment 671719 [details] [diff] [review]
[checked in] Fix all errors in nsNNTPProtocol

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

Looks good, I see failures in test_internalUris.js and test_nntpPassword2.js, but I think the password one could be related to something else and is covered by bug 801916. The test_internalUris.js we could have a look at once this lands.

::: mailnews/news/src/nsNNTPProtocol.cpp
@@ +1369,5 @@
>    {
>      AlertError(MK_NNTP_ERROR_MESSAGE, m_responseText);
>  
>      m_nextState = NNTP_ERROR;
> +    return NS_ERROR_FAILURE;

The define for MK_BAD_NNTP_CONNECTION can be removed with this change.

@@ +1437,1 @@
>    uint32_t status = 0;

nit: I'd prefer status to be moved inside the if, next to where it is really used.

Comment 37

[Mark Banner (:standard8)]

Comment on attachment 671714 [details] [diff] [review]
[checked in] Fix clang build errors caused by nsresult enum class - v3

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

This looks generally fine. I do wonder if we should change the nsPop3Protocol items to use nsresult as Joshua did with nsNNTPProtocol, the other option would be to change status to a boolean there, as I think that's what it is essentially. We can cover those in follow-ups anyway.

r=me without the nsNNTPProtocol changes.

Comment 38

[Mark Banner (:standard8)]

Comment on attachment 671714 [details] [diff] [review]
[checked in] Fix clang build errors caused by nsresult enum class - v3

https://hg.mozilla.org/comm-central/rev/65b8e2fa4eba

Comment 39

[Mark Banner (:standard8)]

Comment on attachment 671719 [details] [diff] [review]
[checked in] Fix all errors in nsNNTPProtocol

https://hg.mozilla.org/comm-central/rev/cc0ff188f5bc

Comment 40

[Mark Banner (:standard8)]

We should now be compiling on all platforms.

There are some test failures still, most of these are covered by bug 801916, but test_internalUris.js and test_nntpPassword2.js do not appear to be.

Comment 41

[:Irving Reid (No longer working on Firefox)]

Attachment: [checked in] Fix error in NNTP search caused by missed function signature change

We changed the IDL signature and the parent class definition of CurrentUrlDone() but missed the override in nsMsgSearchNews

Thanks to jcranmer for spotting the error.

This fixes test_internalUris.js; other tests are still failing

Comment 42

[Mark Banner (:standard8)]

Comment on attachment 671995 [details] [diff] [review]
[checked in] Fix error in NNTP search caused by missed function signature change

Looks fine, minor nit, can you remove the spaces before the open brackets on the lines you're changing please.

Comment 43

[Mark Banner (:standard8)]

Comment on attachment 671995 [details] [diff] [review]
[checked in] Fix error in NNTP search caused by missed function signature change

https://hg.mozilla.org/comm-central/rev/a64aee9cd17e

Comment 44

[Mark Banner (:standard8)]

I believe this is all fixed now apart from the outstanding issues in bug 801916.

Comment 45

[Mark Banner (:standard8)]

(In reply to Mark Banner (:standard8) from comment #43)
> Comment on attachment 671995 [details] [diff] [review]
> [checked in] Fix error in NNTP search caused by missed function signature
> change
> 
> https://hg.mozilla.org/comm-central/rev/a64aee9cd17e

That was the bustage fix, this was the full fix landing:

https://hg.mozilla.org/comm-central/rev/6ad31d127ae2