Closed
Bug 1209176
Opened 9 years ago
Closed 9 years ago
Fix possible for-loop error in nsMsgDBFolder.cpp
Categories
(MailNews Core :: Database, defect)
MailNews Core
Database
Tracking
(firefox44 affected)
RESOLVED
FIXED
Thunderbird 44.0
Tracking | Status | |
---|---|---|
firefox44 | --- | affected |
People
(Reporter: ssitter, Assigned: ssitter)
Details
(Whiteboard: [pvs-studio])
Attachments
(1 file)
1.64 KB,
patch
|
jcranmer
:
review+
|
Details | Diff | Splinter Review |
From http://www.viva64.com/en/b/0347/ PVS-Studio's diagnostic message: V621 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. mailnews/base/util/nsMsgDBFolder.cpp 4501 NS_IMETHODIMP nsMsgDBFolder::GetDisplayRecipients(bool *displayRecipients) { .... // There's one FCC folder for sent mail, and one for sent news nsIMsgFolder *fccFolders[2]; int numFccFolders = 0; for (int i = 0; i < numFccFolders; i++) { .... } .... }
Assignee | ||
Comment 1•9 years ago
|
||
https://dxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#4485 Not sure how to fix this. Fixing the loop to execute 2 times might be one possibility. But my reading of the loop body is that the call to fccFolders[i] would just access uninitialized memory? Maybe the whole block can just be removed? It does nothing and was not changed since the hg import in 2008.
Comment 2•9 years ago
|
||
/mailnews, that's the guys from good old England ;-) - Let's ask them.
Flags: needinfo?(neil)
Nice catch. It looks to me the whole function is unused and also the corresponding attribute of displayRecipients: http://mxr.mozilla.org/comm-central/search?string=displayRecipients&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central Maybe now it isn't needed as we have the new Correspondents column. Previously there were some hacks in which folders to display From and in which To. Maybe the feature was decided by this attribute.
Comment 4•9 years ago
|
||
There used to be a call to GetFoldersWithFlag(nsMsgFolderFlags::SentMail). Unfortunately it used an obsolete configuration that got the folders from the folder tree rather than the server, and the code was removed in 2003. Of course as you point out the whole function was obsoleted by SetSentFolderColumns back in 2000.
Flags: needinfo?(neil)
Assignee | ||
Comment 5•9 years ago
|
||
What do you suggest here: a) remove DisplayRecipients property from nsIMsgFolder IDL? is it possible that some extension uses it? b) remove just the dead code from GetDisplayRecipients method?
Yes, there are 5 extensions that use it, 2 of them explicitly marked compatible with TB38 (maxVersion). Is there a way to fix the function? Or is there a replacement attribute/function that the extensions could use?
Comment 7•9 years ago
|
||
How about just removing the else branch that does nothing: https://dxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#4493 and marking the function as deprecated in a comment.
Assignee | ||
Comment 8•9 years ago
|
||
Comment 9•9 years ago
|
||
Comment on attachment 8668053 [details] [diff] [review] keep method but remove dead code Review of attachment 8668053 [details] [diff] [review]: ----------------------------------------------------------------- r+, but I do like Jorg's idea of noting the method as deprecated in the IDL file.
Attachment #8668053 -
Flags: review?(Pidgeot18) → review+
Assignee | ||
Comment 10•9 years ago
|
||
I can create another patch for deprecation. Would adding [deprecated] to idl work an be sufficient? Is there some other property / method that should be used instead?
Keywords: checkin-needed
Comment 11•9 years ago
|
||
https://hg.mozilla.org/comm-central/rev/258f0a389e6319d9caee61fe6bc0128942e1b072 Bug 1209176 - Fix possible for-loop error in nsMsgDBFolder.cpp. r=jcranmer a=aleth
Comment 12•9 years ago
|
||
(In reply to Stefan Sitter from comment #10) > I can create another patch for deprecation. Would adding [deprecated] to idl > work an be sufficient? Is there some other property / method that should be > used instead? needinfo?
Updated•9 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 44.0
You need to log in
before you can comment on or make changes to this bug.
Description
•