59673 - Occurrences of uninitialized variables being used before being set (mailnews).

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect, P3)

Description

[Rich Burridge]

[Please note that all these warnings are not in the Mail Back End,
but they are in the various mail/news modules. Please reassign as
appropriate].

For more details on this problem, see:

http://bugzilla.mozilla.org/show_bug.cgi?id=59652

This bug is just for the warnings in various source files in the Mail/
News modules:

nsMsgMessageDataSource.cpp:1523: warning: `PRUnichar * prustr' might be used
uninitialized in this function
----
nsMsgViewNavigationService.cpp:261: warning: `PRBool checkStartMessage' might be
used uninitialized in this function
----
nsMsgHdr.cpp:405: warning: `nsresult ret' might be used uninitialized in this
function
----
nsMsgDBFactory.cpp:106: warning: `nsresult rv' might be used uninitialized in
this function
----
nsNntpService.cpp:1255: warning: `PRBool havePref' might be used uninitialized
in this function
----
nsNntpIncomingServer.cpp:213: warning: `PRBool havePref' might be used
uninitialized in this function
----
nsPop3Service.cpp:444: warning: `PRBool havePref' might be used uninitialized in
this function
----
nsLocalStringBundle.cpp:70: warning: `nsresult rv' might be used uninitialized
in this function
----
nsNoneService.cpp:78: warning: `PRBool havePref' might be used uninitialized in
----
nsMsgLocalFactory.cpp:146: warning: `nsresult rv' might be used uninitialized in
this function
----
mimetpfl.cpp:209: warning: `struct MimeInlineTextPlainFlowedExData * exdata'
might be used uninitialized in this function
----
nsMimeXULEmitter.cpp:461: warning: `nsresult rv' might be used uninitialized in
this function
----
nsVCard.cpp:487: warning: `const char * p2' might be used uninitialized in this
----
nsMimeModule.cpp:123: warning: `nsresult rv' might be used uninitialized in this
function
----
nsSmtpProtocol.cpp:107: warning: `nsresult rv' might be used uninitialized in
this function
----
nsMsgComposeStringBundle.cpp:72: warning: `nsresult rv' might be used
uninitialized in this function
----
nsImapService.cpp:2889: warning: `PRBool havePref' might be used uninitialized
in this function
----
nsAbSync.cpp:741: warning: `char * phoneType' might be used uninitialized in
this function
----

Comment 1

[(not reading, please use [e-mail] instead)]

I've got most of these fixes in my tree.  I love fixing warnings.

taking from mscott.

Comment 2

[(not reading, please use [e-mail] instead)]

accepting.  I'll try to land these changes tomorrow.

Comment 3

[(not reading, please use [e-mail] instead)]

I've landed fixes for most of these.  I'll and the rest soon.

thanks again for logging a bug on this.

Comment 4

[Rich Burridge]

Thanks for responding to this so quickly Seth. If you get a
chance, could you add an attachment to this bug for this fixes
please, as we'll need to apply them to our special OEM branch
for our RTM/FCS release. No biggie; i'm sure I can work out the
changes from the Mozilla trunk if not. 

Comment 5

[(not reading, please use [e-mail] instead)]

I've checked in fixes for all but 4 of these.

the fix to nsMsgDBFactory.cpp is waiting on the tree reopening.

the other three:

mimetpfl.cpp:209: warning: `struct MimeInlineTextPlainFlowedExData * exdata'
might be used uninitialized in this function
----
nsVCard.cpp:487: warning: `const char * p2' might be used uninitialized in this
----
nsAbSync.cpp:741: warning: `char * phoneType' might be used uninitialized in
this function

are in rhp code, re-assign to him

Comment 6

[(not reading, please use [e-mail] instead)]

I've fixed the uninitialized variable in nsMsgDBFactory.cpp

richb, to get a list of the diffs you can use bonsai:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=SeaMonkeyAll&branch=HEAD&branchtype=match&dir=mozilla%2Fmailnews&file=&filetype=match&who=sspitzer%25netscape.com&whotype=match&sortby=Change+Size&hours=2&date=explicit&mindate=11%2F10%2F2000&maxdate=11%2F12%2F2000&cvsroot=%2Fcvsroot

Comment 7

[Rich Burridge]

Changes also checked into the OEM branch.

Comment 8

[Ben Bucksch (:BenB)]

Adding blocker. Feel free to take it :).

Comment 9

[scottputterman]

reassigning to ducarroz

Comment 10

[Jean-Francois Ducarroz]

I don't see anymore any of those warning. WFM

Comment 11

[[e-mail] (gone - use [e-mail] instead)]

verified

Comment 12

[Aleksey Nogin]

I just checked nsVCard.cpp and that warning is still there:

nsVCard.cpp: In function `void enterAttr (const char *, const char *)':
nsVCard.cpp:502: warning: `const char *p2' might be used uninitialized in this
function

I will post the complete list of mailnews "uninitialized" warnings once my build
is done.

I am guessing, the reason you are not seeng these warning is because you are
looking at the Tinderbox logs. However Tinderbox is not configured to output
these warnings - see also bug 84508 about that.

Comment 13

[[e-mail] (gone - use [e-mail] instead)]

Yes, we were looking at tbox, sorry.

Comment 14

[Aleksey Nogin]

nsMsgAccountManager.cpp: In function `PRBool nsMsgAccountManager::cleanupOnExit
(nsHashKey *, void *, void *)':
nsMsgAccountManager.cpp:988: warning: `nsresult rv1' might be used uninitialized
in this function
nsMsgAccountManager.cpp:988: warning: `nsresult rv2' might be used uninitialized
in this function
nsMsgFolderDataSource.cpp: In method `nsresult
nsMsgFolderDataSource::OnItemPropertyFlagChanged (nsISupports *, nsIAtom *,
unsigned int, unsigned int)':
nsMsgFolderDataSource.cpp:933: warning: `nsresult rv' might be used
uninitialized in this function
nsMsgSearchDBView.cpp: In method `nsresult
nsMsgSearchDBView::GetMsgHdrForViewIndex (unsigned int, nsIMsgDBHdr **)':
nsMsgSearchDBView.cpp:134: warning: `nsresult rv' might be used uninitialized in
this function
nsMsgThread.cpp: In method `nsresult nsMsgThread::RerootThread (nsIMsgDBHdr *,
nsIMsgDBHdr *, nsIDBChangeAnnouncer *)':
nsMsgThread.cpp:209: warning: `nsresult rv' might be used uninitialized in this
function
nsMsgThread.cpp: In method `nsresult nsMsgThread::GetFirstUnreadChild
(nsIMsgDBHdr **)':
nsMsgThread.cpp:1044: warning: `nsresult rv' might be used uninitialized in this
function
mimetpfl.cpp: In function `int MimeInlineTextPlainFlowed_parse_eof (MimeObject
*, int)':
mimetpfl.cpp:225: warning: `MimeInlineTextPlainFlowedExData *exdata' might be
used uninitialized in this function
comi18n.cpp: In function `char *intl_decode_mime_part2_str (const char *, const
char *, int)':
comi18n.cpp:1150: warning: `const char *q' might be used uninitialized in this
function
nsVCard.cpp: In function `void enterAttr (const char *, const char *)':
nsVCard.cpp:502: warning: `const char *p2' might be used uninitialized in this
function
nsImapMailFolder.cpp: In method `nsresult nsImapMailFolder::GetBodysToDownload
(nsMsgKeyArray *)':
nsImapMailFolder.cpp:2147: warning: `nsresult rv' might be used uninitialized in
this function
nsAbSync.cpp: In method `nsresult nsAbSync::GenerateProtocolForCard (nsIAbCard
*, int, nsString &)':
nsAbSync.cpp:766: warning: `char *phoneType' might be used uninitialized in this
function

If you are interested in seeing the log with all 278 warnings, not just
mailnews, go to bug 59652, attachment 52326 [details]

Comment 15

[scottputterman]

moving to 1.0

Comment 16

[Aleksey Nogin]

Here are the warnings I currently
(http://tinderbox.mozilla.org/SeaMonkey/warn1010759460.5770.html) see in all of
MailNews:

mailnews/absync/src/nsAbSync.cpp:767
 `const char * phoneType' might be used uninitialized in this function

mailnews/base/src/nsMsgAccountManager.cpp:985
 `nsresult rv1' might be used uninitialized in this function
 `nsresult rv2' might be used uninitialized in this function

mailnews/base/src/nsMsgSearchDBView.cpp:133
 `nsresult rv' might be used uninitialized in this function

mailnews/db/msgdb/src/nsMsgThread.cpp:1049
 `nsresult rv' might be used uninitialized in this function

mailnews/db/msgdb/src/nsMsgThread.cpp:214
 `nsresult rv' might be used uninitialized in this function

mailnews/extensions/smime/src/nsMsgComposeSecure.cpp:441
 `nsresult rv' might be used uninitialized in this function

mailnews/imap/src/nsImapMailFolder.cpp:2181
 `nsresult rv' might be used uninitialized in this function

mailnews/imap/src/nsImapProtocol.cpp:5403
 `PRInt32 longestIndex' might be used uninitialized in this function

mailnews/mime/cthandlers/vcard/nsVCard.cpp:502
 `const char * p2' might be used uninitialized in this function

mailnews/mime/src/mimecryp.cpp:461
 `struct MimeHeaders * outer_headers' might be used uninitialized in this function

mailnews/mime/src/mimetext.cpp:433
 `nsresult res' might be used uninitialized in this function

mailnews/mime/src/mimetext.cpp:434
 `int status' might be used uninitialized in this function

mailnews/mime/src/mimetpfl.cpp:225
 `struct MimeInlineTextPlainFlowedExData * exdata' might be used uninitialized
in this function

Comment 17

[Aleksey Nogin]

Any chances of some of the warning being fixed for 1.0? Some of them might be
real stability problems and in general it would be nice to have fewer warnings
in 1.0

Comment 18

[Aleksey Nogin]

Yesterday's checkin for bug 138342 added yet another warning:

mailnews/base/util/nsMsgTxn.cpp:113
 `nsresult rv' might be used uninitialized in this function

I'll attach a patch in a second.

Comment 19

[Aleksey Nogin]

Attachment: Fix for the warnings in mailnews/base

This patch fixes the following warnings:

mailnews/base/src/nsMsgAccountManager.cpp:987
 `nsresult rv1' might be used uninitialized in this function
 `nsresult rv2' might be used uninitialized in this function

mailnews/base/src/nsMsgSearchDBView.cpp:133
 `nsresult rv' might be used uninitialized in this function

mailnews/base/util/nsMsgTxn.cpp:113
 `nsresult rv' might be used uninitialized in this function

Comment 20

[Navin Gupta]

Comment on attachment 83796 [details] [diff] [review]
Fix for the warnings in mailnews/base

Can you move rv1, rv2 down
to where it is used ?

Also have you tested these areas you are touching. changing rv to fix warnings
could break 
something unintentionally.

Comment 21

[Jean-Francois Ducarroz]

Comment on attachment 83796 [details] [diff] [review]
Fix for the warnings in mailnews/base

looks like the right thing to do. R=ducarroz

Comment 22

[David :Bienvenu]

 nsresult nsMsgSearchDBView::GetMsgHdrForViewIndex(nsMsgViewIndex index,
nsIMsgDBHdr **msgHdr)
 {
-  nsresult rv;
+  nsresult rv = NS_ERROR_FAILURE;

I don't like this middle part - NS_ERROR_FAILURE is not informative.
NS_MSG_INVALID_DBVIEW_INDEX would be better, since I think that's the most
likely error.

The last part of the patch looks OK. I'll have to look at the context of the
first part of the patch to figure out a better error than NS_ERROR_FAILURE.

Comment 23

[David :Bienvenu]

Comment on attachment 83796 [details] [diff] [review]
Fix for the warnings in mailnews/base

this is wrong - it should be rv1 = NS_OK. And rv2 = NS_OK as well.

Comment 24

[Aleksey Nogin]

Attachment: Fix for the warnings in mailnews/base, v2

> Can you move rv1, rv2 down to where it is used ?

Done.

> Also have you tested these areas you are touching.

No (not even sure how :-( to test them) - I am just following the logic in the
ocde that's already there.

> changing rv to fix warnings
> could break something unintentionally.

I am only changing "uninitialized" -> something definite. The worse that could
happen is that it would turn some intermittent error into a more reproducible
(and thus easier to track) one. 
Also:
- the rv2 change is only there to get rid of the warning (it can not really be
used uninitialized, but compiler is not smart enough).
- the rv1 change seems the follow the logic of the code - there we are testing
NS_SUCCEEDED(rv1) to figure out whether the previous block of code protected by
an "if NS_SUCCEEDED(rv1)" was exected, so if the code path never even took us
near the previous "if NS_SUCCEEDED(rv1)", the second NS_SUCCEEDED(rv1) clearly
ought to fail.
- nsMsgSearchDBView::GetMsgHdrForViewIndex and nsMsgTxn::CheckForToggleDelete
changes only affect the (rare?) cases when something unexpectedly turns out to
be null.

Comment 25

[Aleksey Nogin]

Bienvenu, are you sure about the rv1 = NS_OK???

Is rv1 stayed uninitialized, then it means that SetFolderDoingCleanupInbox was
never called. So IMHO the initial value should be negative so that we know not
to do the whole GetCleanupInboxInProgress loop (although, of course, it would
never hurt so it's not that important).

Comment 26

[Navin Gupta]

Comment on attachment 83804 [details] [diff] [review]
Fix for the warnings in mailnews/base, v2

r=naving, if you incorporate
comment #22

Comment 27

[Aleksey Nogin]

Attachment: Fix for the warnings in mailnews/base, v3

Changed to use NS_MSG_INVALID_DBVIEW_INDEX

Comment 28

[Aleksey Nogin]

Comment on attachment 83815 [details] [diff] [review]
 Fix for the warnings in mailnews/base, v3

Carrying over r=naving

Comment 29

[Jean-Francois Ducarroz]

Let see if bienvenu still thinks the default value for rv1 & rv2 should be
NS_OK... Personally I think you are doing the righ thing.

Comment 30

[David :Bienvenu]

Frankly, I hate it when people use NS_ERROR_FAILURE - when someone starts
reporting they're getting that error code, then I have to grep through all the
code that uses NS_ERROR_FAILURE, and figure out if it could possibly be
involved. This code couldn't be, since it's not returned, but I would have to
figure that out, and that's a waste of time. Adn this isn't a real failure, is
it? It's just a shorthand for detecting that we didn't enter a certain piece of
code...I know you didn't write this code; it was sloppy to begin with and you're
trying to fix it. It's harmless to leave it as NS_OK, but if you want to be
truly correct, you'd add a boolean that says whether we started cleaning up.
But, we're actually already doing that, with SetFolderDoingCleanupInbox and
GetCleanupInboxInProgress. So, I think NS_OK is perfectly fine. JF, why do you
think that won't work?

Comment 31

[Navin Gupta]

rv1, rv2 =NS_ERROR_FAILURE is perfectly fine to me because we want to check if
we entered a certain piece of code.

Comment 32

[Aleksey Nogin]

Attachment: Fix for the warnings in mailnews/base, v4

OK, then how about we just get rid of rv1,rv2 completely and just use the
booleans that are already there (GetCleanupInboxInProgress and
GetEmptyTrashInProgress)?

Comment 33

[David :Bienvenu]

Comment on attachment 83826 [details] [diff] [review]
Fix for the warnings in mailnews/base, v4

sr=bienvenu, looks good.

Comment 34

[Aleksey Nogin]

naving, ducarroz - can one of you please review the "v4" patch (attachment
83826 [details] [diff] [review]) and then check it in if you like what you see? Thanks a lot!

P.S. Two new warnings in mailnews were added over the last 2 days:

+mailnews/compose/src/nsSmtpService.cpp:901
+ `nsresult rv' might be used uninitialized in this function

+mailnews/db/msgdb/src/nsMailDatabase.cpp:335
+ `PRInt32 folderStreamPos' might be used uninitialized in this function

Comment 35

[Jean-Francois Ducarroz]

Comment on attachment 83826 [details] [diff] [review]
Fix for the warnings in mailnews/base, v4

r=ducarroz

Comment 36

[Jean-Francois Ducarroz]

Patch v4 has been checked in the trunk. I keep this bug open for remaining
warning. Thanks Aleksey Nogin <[e-mail]> for cleanuping our mess.

Comment 37

[Aleksey Nogin]

There are 5 "uninitialized" warnings left in mailnews now:

mailnews/absync/src/nsAbSync.cpp:766
 `const char * phoneType' might be used uninitialized in this function

mailnews/compose/src/nsSmtpService.cpp:901
 `nsresult rv' might be used uninitialized in this function

mailnews/extensions/smime/src/nsMsgComposeSecure.cpp:513
 `nsresult rv' might be used uninitialized in this function

mailnews/imap/src/nsImapProtocol.cpp:5544
 `PRInt32 longestIndex' might be used uninitialized in this function

mailnews/mime/src/mimetpfl.cpp:228
 `struct MimeInlineTextPlainFlowedExData * exdata' might be used uninitialized
in this function

Comment 38

[Aleksey Nogin]

Attachment: Fix a bunch of warnings in mailnews.

This fixes a bunch of warning in mailnews - not only "might be used
uninitialized", but also "unused variable", "comparison between signed and
unsigned" and others.

I tried only fixing places when it seemed that a fix clearly makes sense,
leaving alone (at least for now) unclear cases and cases where a warning is an
indication of a TODO.

diffstat:
 absync/src/nsAbSync.cpp		     |	 24 ++++--------------------
 absync/src/nsAbSyncPostEngine.cpp	     |	  2 --
 addrbook/src/nsAddrDatabase.cpp	     |	  2 +-
 base/search/src/nsMsgFilter.cpp	     |	  4 ++--
 base/search/src/nsMsgLocalSearch.cpp	     |	  1 -
 base/src/nsMsgOfflineManager.cpp	     |	  1 -
 base/src/nsMsgPurgeService.cpp 	     |	  1 -
 base/src/nsMsgQuickSearchDBView.cpp	     |	  2 +-
 base/src/nsMsgSpecialViews.cpp 	     |	  2 +-
 base/src/nsSpamSettings.cpp		     |	  2 +-
 base/util/nsMsgMailNewsUrl.cpp 	     |	  4 ++--
 compose/src/nsSmtpService.cpp		     |	  2 ++
 db/msgdb/src/nsMsgDatabase.cpp 	     |	 13 ++++++-------
 db/msgdb/src/nsMsgOfflineImapOperation.cpp  |	  2 +-
 db/msgdb/src/nsMsgThread.cpp		     |	  1 -
 extensions/smime/src/nsMsgComposeSecure.cpp |	  5 ++---
 imap/src/nsImapIncomingServer.cpp	     |	  2 --
 imap/src/nsImapMailFolder.cpp		     |	  4 ++--
 imap/src/nsImapProtocol.cpp		     |	  2 +-
 imap/src/nsImapUndoTxn.cpp		     |	  1 -
 local/src/nsParseMailbox.cpp		     |	  2 +-
 local/src/nsPop3Sink.cpp		     |	  2 +-
 mime/emitters/src/nsMimeHtmlEmitter.cpp     |	  2 +-
 mime/src/mimecms.cpp			     |	  6 ++----
 mime/src/mimemoz2.cpp			     |	  1 -
 25 files changed, 31 insertions(+), 59 deletions(-)

Please review. Thanks!

Comment 39

[Jean-Francois Ducarroz]

Comment on attachment 105785 [details] [diff] [review]
Fix a bunch of warnings in mailnews.

@@ -241,7 +241,7 @@
   NS_ASSERTION(NS_SUCCEEDED(rv), "failed to truncate filter log");

   mLoggingEnabled = loggingEnabled;
-  return NS_OK;
+  return rv;

Seems the right thing to do but I not sure it's what was intended...Please
check with the author sspitzer

-----
@@ -478,7 +478,7 @@
 {
   if (m_buildMessageUri && m_baseMessageUri)
   {
-      PRUint32 msgKey = -1;
+      PRUint32 msgKey;
       m_newMailParser->GetEnvelopePos(&msgKey);
       m_messageUri.SetLength(0);
       nsBuildLocalMessageURI(m_baseMessageUri, msgKey, m_messageUri);

@@ -478,7 +478,7 @@
 {
   if (m_buildMessageUri && m_baseMessageUri)
   {
-      PRUint32 msgKey = -1;
+      PRUint32 msgKey;
       m_newMailParser->GetEnvelopePos(&msgKey);
       m_messageUri.SetLength(0);
       nsBuildLocalMessageURI(m_baseMessageUri, msgKey, m_messageUri);

Could GetEnvelopePos return without initializing msgKey? if yes, you cannot do
that!

-----
@@ -131,7 +131,7 @@
   NS_ENSURE_ARG(aFlagOperation);
   nsresult rv = m_mdb->GetUint32Property(m_mdbRow, PROP_OPERATION_FLAGS,
&m_operationFlags, 0);
   *aFlagOperation = m_operationFlags;
-  return NS_OK;
+  return rv;
 }

Same here, please check with the author (bienvenu) to be sure it's ok to return
an error

-----
@@ -508,7 +508,7 @@
 /* void finishCryptoEncapsulation (in boolean aAbort); */
 NS_IMETHODIMP nsMsgComposeSecure::FinishCryptoEncapsulation(PRBool aAbort,
nsIMsgSendReport *sendReport)
 {
-  nsresult rv;
+  nsresult rv=NS_OK;

would be nicer if add some spaces here: nsresult rv = NS_OK;

Comment 40

[Aleksey Nogin]

Attachment: Fix a bunch of warnings in mailnews, v.1.1

> would be nicer if add some spaces here: nsresult rv = NS_OK;

Updated, attaching new patch.

> Seems the right thing to do but I not sure it's what was intended...Please
> check with the author sspitzer

Here is the same change with more context. 

 NS_IMETHODIMP nsSpamSettings::ClearLog()
 {
   PRBool loggingEnabled = mLoggingEnabled;

   // disable logging while clearing
   mLoggingEnabled = PR_FALSE;

   nsresult rv = TruncateLog();
   NS_ASSERTION(NS_SUCCEEDED(rv), "failed to truncate filter log");

   mLoggingEnabled = loggingEnabled;
-  return NS_OK;
+  return rv;
 }

My reasonning was that since ClearLog only does on function call, it would be
reasonable to propagate errors, but may be I am wrong. sspitzer, can you please
say whether this is OK? Thanks!

> Same here, please check with the author (bienvenu) to be sure it's ok to 
> return an error

More context:

 /* attribute imapMessageFlagsType flagOperation; */
 NS_IMETHODIMP nsMsgOfflineImapOperation::GetFlagOperation(imapMessageFlagsType
*aFlagOperation)
 {
   NS_ENSURE_ARG(aFlagOperation);
   nsresult rv = m_mdb->GetUint32Property(m_mdbRow, PROP_OPERATION_FLAGS,
&m_operationFlags, 0);
   *aFlagOperation = m_operationFlags;
-  return NS_OK;
+  return rv;
 }

Here if GetUint32Property failed, then the aFlagOperation is likely to get a
meaningless value, so returning rv seemed more reasonable than returning NS_OK.
bienvenu, can you please say if you agree? Thanks!

> Could GetEnvelopePos return without initializing msgKey? if yes, you cannot
do
> that!

Yes, I looked into that. Currently it will always set msgKey, if it gets a
non-NULL pointer (which is clearly the case here since it gets a pointer to an
actual variable).

Comment 41

[David :Bienvenu]

Cavin should look at the ab sync changes.

Comment 42

[David :Bienvenu]

actually, I'm not sure it's OK for ClearLog to propagate the error, since that
would throw a js exception. You should really check with Seth - it might be OK
but we should check with Seth. I think the offline change is OK. The ABSync
changes look OK, but Cavin is the module owner. I'm ready to give a provisional
sr, as long as Cavin and Seth sign off on their bits.

Comment 43

[Cavin Song]

Absync changes look fine to me. Thanks for catching the warnings.

Comment 44

[Aleksey Nogin]

Comment on attachment 105871 [details] [diff] [review]
Fix a bunch of warnings in mailnews, v.1.1

An overlapping "anti-warnings" patch was checked in in bug 179775, so this
patch is now inconsistent with the CVS trunk :-(

Comment 45

[Aleksey Nogin]

Attachment: Fix a bunch of warnings in mailnews, v2

Updated to match the current CVS trunk; fixed a few more warnings.

This patch does *not* include the "controversial" nsSpamSettings::ClearLog
change, I'll handle it in a separate patch.

diffstat:

 absync/src/nsAbSync.cpp		     |	 24 ++++--------------------
 addrbook/src/nsAddrDatabase.cpp	     |	  2 +-
 base/search/src/nsMsgFilter.cpp	     |	  4 ++--
 base/search/src/nsMsgSearchAdapter.cpp      |	  2 +-
 base/src/nsMsgQuickSearchDBView.cpp	     |	  2 +-
 base/util/nsMsgMailNewsUrl.cpp 	     |	  4 ++--
 compose/src/nsMsgCompose.cpp		     |	  6 +++---
 compose/src/nsSmtpService.cpp		     |	  2 ++
 db/msgdb/src/nsMsgDatabase.cpp 	     |	 12 ++++++------
 db/msgdb/src/nsMsgOfflineImapOperation.cpp  |	  4 ++--
 db/msgdb/src/nsMsgThread.cpp		     |	  1 -
 extensions/smime/src/nsMsgComposeSecure.cpp |	  4 ++--
 imap/src/nsImapIncomingServer.cpp	     |	  2 +-
 imap/src/nsImapMailFolder.cpp		     |	  6 ++----
 imap/src/nsImapMoveCoalescer.cpp	     |	  2 +-
 imap/src/nsImapProtocol.cpp		     |	  2 +-
 imap/src/nsImapServerResponseParser.cpp     |	  4 ++--
 local/src/nsParseMailbox.cpp		     |	  2 +-
 local/src/nsPop3Sink.cpp		     |	  2 +-
 mime/emitters/src/nsMimeHtmlEmitter.cpp     |	  2 +-
 20 files changed, 36 insertions(+), 53 deletions(-)

Comment 46

[Aleksey Nogin]

Attachment: Fixe the "unused" warning by propagating the error out of nsSpamSettings::ClearLog

Comment 47

[Aleksey Nogin]

Comment on attachment 107610 [details] [diff] [review]
Fix a bunch of warnings in mailnews, v2

Pls review. Thanks!

Comment 48

[David :Bienvenu]

   nsresult rv = NS_OK;
-  PRUint32 startSequence; // no need to init; we won't use it unless numKeys > 0
-  if (numKeys > 0)
-    startSequence = keys[0];
+  PRUint32 startSequence = (numKeys > 0) ? keys[0] : 0;

this is wrong - if you want to fix this, you should just do this at the top of
the function.

if (numKeys <= 0)
  return NS_ERROR_INVALID_ARG;

Comment 49

[Aleksey Nogin]

Comment on attachment 107611 [details] [diff] [review]
Fixe the "unused" warning by propagating the error out of nsSpamSettings::ClearLog

Seth, is it OK to propagate the error this way? Thanks!

Comment 50

[Aleksey Nogin]

Attachment: Fix a bunch of warnings in mailnews, v2.1

Updated the numKeys part to address bienvenu's comment.

Comment 51

[Jean-Francois Ducarroz]

Comment on attachment 107612 [details] [diff] [review]
Fix a bunch of warnings in mailnews, v2.1

in nsMsgCompose.cpp, instead of casting those -1 which looks very bad (how come
can I put negative value in an unsigned variable!), I will provide you a better
way to fix that...

The rest of the patch seems fine to me

Comment 52

[Jean-Francois Ducarroz]

better fix for nsMsgCompose.cpp:

Index: mozilla/mailnews/compose/src/nsMsgCompose.cpp
===================================================================
RCS file: /cvsroot/mozilla/mailnews/compose/src/nsMsgCompose.cpp,v
retrieving revision 1.366
diff -w -u -2 -r1.366 nsMsgCompose.cpp
--- mozilla/mailnews/compose/src/nsMsgCompose.cpp	13 Nov 2002 23:56:28 -0000	1.366
+++ mozilla/mailnews/compose/src/nsMsgCompose.cpp	27 Nov 2002 21:09:49 -0000
@@ -3929,5 +3929,5 @@
     }
 
-    *_retval = -1;
+    *_retval = nsIAbPreferMailFormat::html;
     for (i = 0; i < MAX_OF_RECIPIENT_ARRAY; i ++)
     {
@@ -3963,10 +3963,9 @@
           {
             case nsIAbPreferMailFormat::html :
-              if (*_retval == -1)
-                *_retval = nsIAbPreferMailFormat::html;
+              // nothing to do
               break;
 
             case nsIAbPreferMailFormat::plaintext :
-              if (*_retval == -1 || *_retval == nsIAbPreferMailFormat::html)
+              if (*_retval == nsIAbPreferMailFormat::html)
                 *_retval = nsIAbPreferMailFormat::plaintext;
               break;

Comment 53

[Aleksey Nogin]

Attachment: Fix a bunch of warnings in mailnews, v2.2

Incorporated ducarroz's fix to nsMsgCompose.cpp

Comment 54

[Aleksey Nogin]

Comment on attachment 107612 [details] [diff] [review]
Fix a bunch of warnings in mailnews, v2.1

Ah, cancelling requests on obsoleted attachments should really happen
automatically (bug 180652)...

Comment 55

[Jean-Francois Ducarroz]

Comment on attachment 107617 [details] [diff] [review]
Fix a bunch of warnings in mailnews, v2.2

R=ducarroz

Comment 56

[David :Bienvenu]

-				int accum = 0;
+				unsigned accum = 0;
this should be PRUint32

+                  for (unsigned i = 0; i < numFolders; i++)
as should this.

this should simply be, at the very top before anything else:
if (numKeys <= 0)
  return NS_ERROR_INVALID_ARG;

if you want me to make those changes and check in for you, I can.

Comment 57

[Aleksey Nogin]

> this should simply be, at the very top before anything else:
> if (numKeys <= 0)
>  return NS_ERROR_INVALID_ARG;

Ah, OK. And then just 
PRUint32 startSequence = keys[0];
? (if it still uses the "if (numKeys > 0)", the compiler will probably still
give a warning).

> if you want me to make those changes and check in for you, I can.

Yes, that would be great, thanks! (I already have 3 "anti-warning" patches with
r/sr waiting for somebody to check them in, would be nice not to have a 4-th one)

Comment 58

[David :Bienvenu]

yes, we would remove the second numKeys check since it would be redundant. OK,
I'll apply, fix, and checkin sometime in the next few days.

Comment 59

[David :Bienvenu]

this is all checked in, except for the nsMsgDatabase.cpp changes, because I have
other changes in that file in my tree.

Comment 60

[Aleksey Nogin]

Besides nsMsgDatabase.cpp there is still one "uninitialized" warning left in
mailnews:

mailnews/mime/src/mimetpfl.cpp:228
 `struct MimeInlineTextPlainFlowedExData * exdata' might be used uninitialized
in this function

The reason I didn't cover it in my patch is that I do not understand why gcc
complains about it. It seems that compiler is not realizing that 

while ((exdata = *prevexdata) != nsnull) { ... }

would initialize exdata no matter what. Is this a known gcc bug? Should I report it?

See
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/mime/src/mimetpfl.cpp&mark=228,232#220

Comment 61

[Aleksey Nogin]

Attachment: Fix the warning (and a potential bug) in mimetpfl.cpp

Re: comment #60

I finally realized what was wrong and where the warning was from. The code had:


  if (status < 0) goto EarlyOut;
  ...
  struct MimeInlineTextPlainFlowedExData *exdata;
  ...
EarlyOut:
  PR_Free(exdata);

Which means that if the first EarlyOut branch is taken, it would attempt to
free a random pointer!

I fixed it by moving the declaration to the beginning of the function and
setting exdata to NULL.

Comment 62

[David :Bienvenu]

Comment on attachment 109595 [details] [diff] [review]
Fix the warning (and a potential bug) in mimetpfl.cpp

sr=bienvenu

Comment 63

[Daniel Bratell]

Comment on attachment 109595 [details] [diff] [review]
Fix the warning (and a potential bug) in mimetpfl.cpp

Thanks! I've looked at that one myself without understanding the reason for it.

r=[e-mail]

Comment 64

[Aleksey Nogin]

Comment on attachment 109595 [details] [diff] [review]
Fix the warning (and a potential bug) in mimetpfl.cpp

Can somebody please check this in (I do not have CVS access)? Thanks!

Comment 65

[Jean-Francois Ducarroz]

I'll check in the fix as soon the tree open...

Comment 66

[Jean-Francois Ducarroz]

patch http://bugzilla.mozilla.org/attachment.cgi?id=109595&action=view checked in

Comment 67

[Aleksey Nogin]

Comment on attachment 107617 [details] [diff] [review]
Fix a bunch of warnings in mailnews, v2.2

Status update: the only remaining warning in mailnews is

mailnews/db/msgdb/src/nsMsgDatabase.cpp:574
 `nsresult rv' might be used uninitialized in this function

which is covered by this patch - bienvenu checked in most of it, but not the
nsMsgDatabase.cpp part just yet - see comment #59).

Comment 68

[(not reading, please use [e-mail] instead)]

I've fixed the warning in nsMsgDatabase.cpp.

marking fixed.  if there are others, let's open new bugs.

Comment 69

[(not reading, please use [e-mail] instead)]

Comment on attachment 107611 [details] [diff] [review]
Fixe the "unused" warning by propagating the error out of nsSpamSettings::ClearLog

I fixed the warning another way, since I want to return NS_OK here.

Comment 70

[(not reading, please use [e-mail] instead)]

whoops!

the change to mailnews/db/msgdb/src/nsMsgDatabase.cpp cause some regressions.

see bug #189466

I fixed the warnings another way.

no cookie for me.

Comment 71

[Aleksey Nogin]

V, brad TBox currently does not show any "uninitialized" warnings in mailnews.
Thanks to everybody who made this possible!