565056 - nsMsgRecentFoldersDataSource::WantsThisFolder and nsMsgFlatFolderDataSource::GetTargets both try to change m_folders at the same time

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[[e-mail]]

nsMsgFlatFolderDataSource::GetTargets has a pretty significant bug in that it unconditionally adds the wanted folders to m_folders each time it is called. Worse still though is when it is used with nsMsgRecentFoldersDataSource as it calls WantsThisFolder which also adds the wanted folders to m_folders.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Requesting review from bienvenu as he's most likely to remember about RDF but as Thunderbird doesn't use this code I'll accept review from Mnyromyr or IanN.

* Split the folder-getting code out of nsMsgRecentFoldersDataSource::WantsThisFolder into EnsureFolders
* Split the folder-getting code out of nsMsgFlatFolderDataSource::GetTargets into EnsureFolders
* Moved the built folders flag into nsMsgFlatFolderDataSource so that its EnsureFolders can also use it
* Some code cleanup as allFolders no longer gets used after building m_folders
* Moved the rest of WantsThisFolder although it wasn't strictly necessary

Comment 2

[David :Bienvenu]

Comment on attachment 444681 [details] [diff] [review]
Proposed patch

is kNotFound going to be available when we go to frozen linkage? Otherwise, the patch looks reasonable at first glance.

Comment 3

[[e-mail]]

(In reply to comment #2)
> is kNotFound going to be available when we go to frozen linkage?
Yes, we added it to nsMsgUtils.h

Comment 4

[Ian Neal]

Comment on attachment 444681 [details] [diff] [review]
Proposed patch

>+++ b/mailnews/base/src/nsMsgFolderDataSource.cpp	Tue May 11 17:32:28 2010 +0100
>@@ -2239,28 +2253,16 @@
>             for (PRUint32 newEntryIndex = lastEntry; newEntryIndex < newLastEntry;)
>             {
>               nsCOMPtr <nsIMsgFolder> curFolder = do_QueryElementAt(allFolders, newEntryIndex);
>-              if (!WantsThisFolder(curFolder))
>+              if (WantsThisFolder(curFolder))
>               {
>-                allFolders->RemoveElementAt(newEntryIndex);
>-                newLastEntry--;
>-              }
>-              else
>-              {
>-                // unfortunately, we need a separate array for this since
>-                // ListDescendents takes an nsISupportsArray. But we want
>-                // to use an nsCOMArrray for the DS since that's the
>-                // preferred mechanism.
>                 m_folders.AppendObject(curFolder);
>-                newEntryIndex++;
>               }
>             }
>           }
>         }
>       }
>-      return NS_NewArrayEnumerator(targets, allFolders);
>     }
>   }
>-  return NS_NewSingletonEnumerator(targets, property);
> }
> 
As discussed on IRC the for loop has lost its increment, so if this code does get reached (which you say isn't at the moment) then it will get stuck. If the code is going to be left in, then need to fix the for loop.
r=me with that addressed.

Comment 5

[David :Bienvenu]

Comment on attachment 444681 [details] [diff] [review]
Proposed patch

I don't think you need an official review from me at this point.

Comment 6

[Karsten Düsterloh]

Comment on attachment 444681 [details] [diff] [review]
Proposed patch

>+void nsMsgFlatFolderDataSource::EnsureFolders()
>+{
>+  if (!m_builtFolders) {
>+    m_builtFolders = PR_TRUE; // in case something goes wrong

This file's style requests the { on its own line.

>             for (PRUint32 newEntryIndex = lastEntry; newEntryIndex < newLastEntry;)
>             {
>               nsCOMPtr <nsIMsgFolder> curFolder = do_QueryElementAt(allFolders, newEntryIndex);
>-              if (!WantsThisFolder(curFolder))
>+              if (WantsThisFolder(curFolder))
>               {
>-                allFolders->RemoveElementAt(newEntryIndex);
>-                newLastEntry--;
>-              }
>-              else
>-              {
>-                // unfortunately, we need a separate array for this since
>-                // ListDescendents takes an nsISupportsArray. But we want
>-                // to use an nsCOMArrray for the DS since that's the
>-                // preferred mechanism.
>                 m_folders.AppendObject(curFolder);
>-                newEntryIndex++;
>               }
>             }

What IanN said. Plus, the inner if braces aren't needed.

r=me with those fixed.

Comment 7

[[e-mail]]

Pushed changeset d79084efefd9 to comm-central.

I didn't remove the braces around the innermost if because I thought it made the close brace cascade look odd.