Closed Bug 59673 Opened 19 years ago Closed 17 years ago

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

Categories

(MailNews Core :: Backend, defect, P3)

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.2alpha

People

(Reporter: rich.burridge, Assigned: bugzilla)

References

Details

Attachments

(3 files, 8 obsolete files)

587 bytes, patch
sspitzer
: review-
Details | Diff | Splinter Review
18.43 KB, patch
bugzilla
: review+
Details | Diff | Splinter Review
967 bytes, patch
bratell
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
[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
----
Blocks: 59652
I've got most of these fixes in my tree.  I love fixing warnings.

taking from mscott.
Assignee: mscott → sspitzer
accepting.  I'll try to land these changes tomorrow.
Status: NEW → ASSIGNED
I've landed fixes for most of these.  I'll and the rest soon.

thanks again for logging a bug on this.
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. 
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
Assignee: sspitzer → rhp
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Target Milestone: --- → Future
Blocks: 60740
Changes also checked into the OEM branch.
Adding blocker. Feel free to take it :).
Depends on: 49159
reassigning to ducarroz
Assignee: rhp → ducarroz
Status: ASSIGNED → NEW
QA Contact: esther → stephend
I don't see anymore any of those warning. WFM
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → WORKSFORME
verified
Status: RESOLVED → VERIFIED
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.
Status: VERIFIED → REOPENED
Resolution: WORKSFORME → ---
Yes, we were looking at tbox, sorry.
Status: REOPENED → ASSIGNED
Target Milestone: Future → mozilla0.9.6
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]
moving to 1.0
Target Milestone: mozilla0.9.6 → mozilla1.0
Target Milestone: mozilla1.0 → mozilla1.2
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

Summary: Occurances of uninitialized variables being used before being set. → Occurances of uninitialized variables being used before being set (mailnews).
Depends on: 124618
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
Keywords: mozilla1.0
Depends on: 126460
Depends on: 132161
Depends on: 132163
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.

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
Keywords: patch, review
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 on attachment 83796 [details] [diff] [review]
Fix for the warnings in mailnews/base

looks like the right thing to do. R=ducarroz
Attachment #83796 - Flags: review+
 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 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.
> 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.
Attachment #83796 - Attachment is obsolete: true
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 on attachment 83804 [details] [diff] [review]
Fix for the warnings in mailnews/base, v2

r=naving, if you incorporate
comment #22
Attachment #83804 - Flags: review+
Changed to use NS_MSG_INVALID_DBVIEW_INDEX
Attachment #83804 - Attachment is obsolete: true
Comment on attachment 83815 [details] [diff] [review]
 Fix for the warnings in mailnews/base, v3

Carrying over r=naving
Attachment #83815 - Flags: review+
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.
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?
rv1, rv2 =NS_ERROR_FAILURE is perfectly fine to me because we want to check if
we entered a certain piece of code.
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 on attachment 83826 [details] [diff] [review]
Fix for the warnings in mailnews/base, v4

sr=bienvenu, looks good.
Attachment #83826 - Flags: superreview+
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 on attachment 83826 [details] [diff] [review]
Fix for the warnings in mailnews/base, v4

r=ducarroz
Attachment #83826 - Flags: review+
Attachment #83815 - Attachment is obsolete: true
Patch v4 has been checked in the trunk. I keep this bug open for remaining
warning. Thanks Aleksey Nogin <ayn2@cornell.edu> for cleanuping our mess.
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

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!
Attachment #83826 - Attachment is obsolete: true
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;
> 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).
Attachment #105785 - Attachment is obsolete: true
Attachment #105871 - Flags: superreview?(bienvenu)
Cavin should look at the ab sync changes.
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.
Absync changes look fine to me. Thanks for catching the warnings.
Depends on: 179775
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 :-(
Attachment #105871 - Flags: superreview?(bienvenu) → review-
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(-)
Attachment #105871 - Attachment is obsolete: true
Comment on attachment 107610 [details] [diff] [review]
Fix a bunch of warnings in mailnews, v2

Pls review. Thanks!
Attachment #107610 - Flags: superreview?(bienvenu)
Attachment #107610 - Flags: review?(ducarroz)
   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 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!
Attachment #107611 - Flags: superreview?
Attachment #107611 - Flags: review?(sspitzer)
Updated the numKeys part to address bienvenu's comment.
Attachment #107610 - Attachment is obsolete: true
Attachment #107610 - Flags: superreview?(bienvenu)
Attachment #107610 - Flags: review?(ducarroz)
Attachment #107612 - Flags: superreview?(bienvenu)
Attachment #107612 - Flags: review?(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
Attachment #107612 - Flags: review?(ducarroz) → review-
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;
Incorporated ducarroz's fix to nsMsgCompose.cpp
Attachment #107612 - Attachment is obsolete: true
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)...
Attachment #107612 - Flags: superreview?(bienvenu)
Attachment #107617 - Flags: superreview?(bienvenu)
Attachment #107617 - Flags: review?(ducarroz)
Comment on attachment 107617 [details] [diff] [review]
Fix a bunch of warnings in mailnews, v2.2

R=ducarroz
Attachment #107617 - Flags: review?(ducarroz) → review+
-				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.
> 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)
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.
this is all checked in, except for the nsMsgDatabase.cpp changes, because I have
other changes in that file in my tree.
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
Summary: Occurances of uninitialized variables being used before being set (mailnews). → Occurences of uninitialized variables being used before being set (mailnews).
Attachment #107611 - Attachment description: Fixe the "unused" warning but propagating the error out of nsSpamSettings::ClearLog → Fixe the "unused" warning by propagating the error out of nsSpamSettings::ClearLog
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.
Attachment #109595 - Attachment description: Fix the warning in mimetpfl.cpp → Fix the warning (and a potential bug) in mimetpfl.cpp
Attachment #109595 - Flags: superreview?(bienvenu)
Attachment #109595 - Flags: review?(bratell)
Comment on attachment 109595 [details] [diff] [review]
Fix the warning (and a potential bug) in mimetpfl.cpp

sr=bienvenu
Attachment #109595 - Flags: superreview?(bienvenu) → superreview+
Attachment #109595 - Flags: review?(bratell) → review+
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=bratell@lysator.liu.se
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!
I'll check in the fix as soon the tree open...
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).
OS: Solaris → All
Hardware: Sun → All
Summary: Occurences of uninitialized variables being used before being set (mailnews). → Occurrences of uninitialized variables being used before being set (mailnews).
I've fixed the warning in nsMsgDatabase.cpp.

marking fixed.  if there are others, let's open new bugs.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago17 years ago
Resolution: --- → FIXED
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.
Attachment #107611 - Flags: review?(sspitzer) → review-
Attachment #107611 - Flags: superreview?
Attachment #107617 - Flags: superreview?(bienvenu)
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.
V, brad TBox currently does not show any "uninitialized" warnings in mailnews.
Thanks to everybody who made this possible!
Status: RESOLVED → VERIFIED
Depends on: 209335
Product: MailNews → Core
Duplicate of this bug: 132161
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.