115228 - collapsing local folders disables filters

Status: VERIFIED FIXED

(MailNews Core :: Filters, defect, P2)

Description

[Bruce Ingalls]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Macintosh; U; PPC; en-US; rv:0.9.6+) Gecko/20011210
BuildID:    2001121008

I click on the triangle next to Local Folders in the side bar, collapsing all my
subdirectories.
I also filter all my email into subdirectories of Local Folders.
This causes the subdirs to not be found, forcing the filters to be disabled.

Reproducible: Sometimes
Steps to Reproduce:
1.Create "spam" as a subfolder of Local Folders
2.Create a filter, which moves spam to the the spam subfolder
3.Collapse all subfolders of Local Folders
4.Send yourself some spam

Actual Results:  Warning dialog pops up, saying that spam folder cannot be found

Expected Results:  Filters should be able to search collapsed folders

Comment 1

[laurel]

This was mentioned in other cases of filters disabling (one instance bug
109855). I'll let naving decide if we should call this one a tracker for the
local folders case, since the other issues have muddled together.

Comment 2

[laurel]

Using dec18 commercial trunk: I still can only reproduce in the case where local
folders is never expanded in the session prior to the filter firing.  If I've
collapsed subfolders and/or the entire Local Folders structure after it was
already expanded, the filter will fire properly (of course, if it had been
disabled you will need to enable it and reselect the destination folder). 

Comment 3

[laurel]

*** Bug 117240 has been marked as a duplicate of this bug. ***

Comment 4

[Navin Gupta]

The folders are not created if they are not open, as I expected. This is because
the datasource never asks for them. We need some mechanism whereby all 
the folders are created on start-up, don't matter if they are open or not. 

Comment 5

[Navin Gupta]

*** Bug 109855 has been marked as a duplicate of this bug. ***

Comment 6

[Navin Gupta]

*** Bug 94968 has been marked as a duplicate of this bug. ***

Comment 7

[Asa Dotzler [:asa]]

navin, can you look into this for 0.9.8. This seems rather bad. 

Comment 8

[Navin Gupta]

still looking into how to fix it. 

Comment 9

[Navin Gupta]

Attachment: proposed fix

The fix is to create the local folders even if they are closed. This
need not be done for imap because server sync does it separately.
Also I don't see any reason why we want to do it for news. firstly newsgroup
are at one level so they are always created because of GetSubFolders call
in createFolderOpenNode, also filters are never set on news.

The folders are created only once because of mInitialized flag so there
is no extra cost.

Comment 10

[Navin Gupta]

I will do some more tests, the scenario mentioned in the bug works with 
the patch.

Comment 11

[Navin Gupta]

Adding correct dependency bug for 0.9.8

Comment 12

[Navin Gupta]

Attachment: same patch, minor changes.

tested and it works

Comment 13

[Asa Dotzler [:asa]]

Navin, great, that was fast!  Who can we get to review and super-review this?

Comment 14

[(not reading, please use [e-mail] instead)]

I think the general idea is ok, but here are some comments:

don't include "nsIMsgLocalMailFolder.h"

nsMsgFolderDataSource should not be depending on local, news or imap.  

I don't even think you need a new method.  

nsMsgFolderDataSource::createFolderOpenNode() is already 
calling nsIMsgFolder->GetSubFolders().

your new method just calls GetSubFolders() recursively.  

I think the proper fix is to fix just nsMsgLocalMailFolder::GetSubFolders() 
[since imap and news don't need this].

nsMsgLocalMailFolder::GetSubFolders() could the work of calling GetSubFolders() 
on all it's children [in the if (!mInitialized) {} block].

I don't think you need to use an enumerator, since you can just do a for loop 
over mSubFolders.

if this works, make sure to add a clear comment to 
nsMsgLocalMailFolder::GetSubFolders() about why we aren't initializing local 
folder lazily.

as for why your approach works, I think a relatively similar code patch 
(calling GetSubFolders() for every folder) would happen if started up and you 
had persisted everything to open.

Comment 15

[(not reading, please use [e-mail] instead)]

s/relatively similar code patch/relatively similar code path

Comment 16

[(not reading, please use [e-mail] instead)]

for testing, make sure to test 4.x migration and new (local) account creation.

Comment 17

[Navin Gupta]

Attachment: proposed fix

This patch moves the subfolder creation part inside GetSubFolders. I have
tested Rename/Move/delete local folders. There are some changes with
mInitialized flag and will be taken care of in bug 120203. 
tested migration and creation of new local folder accts and it works. 

seth, please review.

Comment 18

[(not reading, please use [e-mail] instead)]

Comment on attachment 66009 [details] [diff] [review]
proposed fix

the patch looks good.  

are there any scenarios where we would return before even getting to this code?

some minor nits:

>+         /*we need to create all the folders at start-up because if a folder having subfolders is
>+      closed then the datasource will not ask for subfolders */

Can you expand comment, and explain why this is not necessary for IMAP and
news?

>+      PRUint32 cnt=0;

you don't have to initialize cnt.

>+      rv = mSubFolders->Count(&cnt);

should we be checking this rv?

>+      nsCOMPtr<nsIEnumerator> enumerator;
>+      for (PRUint32 i=0; i< cnt;i++)
>+      {
>+        nsCOMPtr<nsISupports> supports = getter_AddRefs(mSubFolders->ElementAt(i));
>+        nsCOMPtr<nsIMsgFolder> folder = do_QueryInterface(supports, &rv);
>+        if (folder && NS_SUCCEEDED(rv))
>+          folder->GetSubFolders(getter_AddRefs(enumerator));

should we be checking the return value of GetSubFolders()?  It shouldn't be
failing,
so if it does, we'd want to know (by asserting), but we don't want to return
early.
so add 

>+        if (folder && NS_SUCCEEDED(rv)) 
>+        {
>+          rv = folder->GetSubFolders(getter_AddRefs(enumerator));
>+          NS_ASSERTION(NS_SUCCEEDED(rv),"GetSubFolders failed");
>+        }

Comment 19

[Navin Gupta]

Attachment: proposed fix

patch w/ changes, checking rv etc...

Comment 20

[(not reading, please use [e-mail] instead)]

Comment on attachment 66046 [details] [diff] [review]
proposed fix

sr=sspitzer

sounds like you tested well.

Comment 21

[Asa Dotzler [:asa]]

a=asa (on behalf of drivers)

Comment 22

[Navin Gupta]

fixed

Comment 23

[laurel]

OK feb01 commercial trunk build, win98. 
Tested collapsing subfolder hierarchies, collapsing entired Local Folders. Tried
filters to local folders collapsed in session where they were open on startup.
Tried filters firing to varied levels of local folders which had never been
expanded in session.  All OK so far.  Need more testing for migrated profiles,
other platforms...just wanted to note some testing progress.

Comment 24

[Jan Schnackenberg]

Here the filters typically work, too. But just that last comment did not get
filtered from my IMAP-Account to my local folder for mails regarding Bug 115228.
There was no alert, or any notification, that anything went wrong, and when I
looked the filter was enabled.

I'm not sure, though, if this is a new bug.

Comment 25

[laurel]

I'm not sure I understand what you meant in comment #24, please clarify
(briefly). If it's another issue, please open another bug.

Collapsed local folders generally tested OK with feb04 commercial trunk on Linus
rh6.2, mac OS 10.1

Still need to do testing with migration, will do that and log specific, separate
bug(s) if any issues found.

Marking this one verified. 
If Jan Schnackenberg feels comment #24 was indeed this issue, please reopen with
details for reproducibility.

Comment 26

[Jan Schnackenberg]

I have a filter, that filters any message containing "[Bug 115228]" in the
subject to a folder "Local Folders/Software/Mozilla/Bugzilla/_Bug 115228_". This
filter seems to work "occasionally".

When I received the CC for comment #23 it was not filtered to the designated
folder. But there was no warning that the local folder does not exist, nor was
the filter itself disabled when I checked on that.

I'm absolutely not sure whether I this behavior is easily reproducible, but I'll
try to find out tonight. I'm also not sure whether to consider this as the same
bug, because before the filter would just pop up a message that the destination
folder doesn't exist and that the filter is disabled.

Comment 27

[Christoph Sold]

Filtering ceases to work when
- The folder hierarchiy is closed completely
- the folder to filter into is now invisible
- Mozilla is quit _completely_
- Mozilla is restarted
- Mail is fetched via POP while the folder hierarchy is still closed
I have not verified the bug in recent mozilla builds; just wanted to add the
test case.

Comment 28

[laurel]

Yes, that's what I called out in comment #23.

Comment 29

[Michael Buckland]

Adding to status whiteboard AOLTW and plussing bugs definite requirements,
adding just AOLTW for possibles.

Comment 30

[laurel]

*** Bug 129325 has been marked as a duplicate of this bug. ***

Comment 31

[Scott MacGregor]

fixed on the AOLTW branch.