Closed Bug 59673 Opened 24 years ago Closed 22 years ago

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

Categories

(MailNews Core :: Backend, defect, P3)

Product:
MailNews Core
Component:
Backend
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.2alpha

People

(Reporter: rich.burridge, Assigned: bugzilla)

References

Details

Attachments

(3 files, 8 obsolete files)

Fixe the "unused" warning by propagating the error out of nsSpamSettings::ClearLog
587 bytes, patch
sspitzer
: review-
Details | Diff | Splinter Review
Fix a bunch of warnings in mailnews, v2.2
18.43 KB, patch
bugzilla
: review+
Details | Diff | Splinter Review
Fix the warning (and a potential bug) in mimetpfl.cpp
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: 23 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: 23 years ago → 22 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
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: