1209176 - Fix possible for-loop error in nsMsgDBFolder.cpp

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[Stefan Sitter [:ssitter]]

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++)
  {
    ....
  }
  ....
}

Comment 1

[Stefan Sitter [:ssitter]]

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

[Jorg K (CEST = GMT+2)]

/mailnews, that's the guys from good old England ;-) - Let's ask them.

Comment 3

[:aceman]

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

[[e-mail]]

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.

Comment 5

[Stefan Sitter [:ssitter]]

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?

Comment 6

[:aceman]

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

[Jorg K (CEST = GMT+2)]

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.

Comment 8

[Stefan Sitter [:ssitter]]

Attachment: keep method but remove dead code

Comment 9

[Joshua Cranmer [:jcranmer]]

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.

Comment 10

[Stefan Sitter [:ssitter]]

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?

Comment 11

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/258f0a389e6319d9caee61fe6bc0128942e1b072
Bug 1209176 - Fix possible for-loop error in nsMsgDBFolder.cpp. r=jcranmer a=aleth

Comment 12

[aleth [:aleth]]

(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?