92359 - Check filterList on all the servers when a folder is renamed/deleted

Status: VERIFIED FIXED

(MailNews Core :: Filters, defect)

Description

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

assertion in nsMsgFolder::GetFilterList().

today's trunk build.  I'll go look at my filters.

NTDLL! 77f9f9df()
nsDebug::Assertion(const char * 0x0267e3e8, const char * 0x0267e3dc, const char 
* 0x0267e3a4, int 0x0000098b) line 290 + 13 bytes
nsMsgFolder::GetFilterList(nsMsgFolder * const 0x045abd4c, nsIMsgFilterList * * 
0x0012f7d0) line 2443 + 32 bytes
nsMsgDBFolder::ChangeFilterDestination(nsMsgDBFolder * const 0x045abd4c, 
nsIMsgFolder * 0x00000000, int 0x00000000, int * 0x0012f860) line 1536 + 39 
bytes
nsMsgFolder::WarnAndDisableFilter(nsIMsgWindow * 0x00000000) line 2579 + 27 
bytes
nsMsgFolder::RecursiveDelete(nsMsgFolder * const 0x045abd4c, int 0x00000001, 
nsIMsgWindow * 0x00000000) line 1157
nsMsgFolder::PropagateDelete(nsMsgFolder * const 0x03fe88ec, nsIMsgFolder * 
0x045abd4c, int 0x00000001, nsIMsgWindow * 0x00000000) line 1085 + 34 bytes
nsMsgFolder::DeleteSubFolders(nsMsgFolder * const 0x03fe88ec, nsISupportsArray 
* 0x0457d700, nsIMsgWindow * 0x00000000) line 1050
nsImapMailFolder::RemoveSubFolder(nsImapMailFolder * const 0x03fe89a8, 
nsIMsgFolder * 0x045abd4c) line 854 + 26 bytes
nsImapIncomingServer::DeleteNonVerifiedFolders(nsIFolder * 0x045abd4c) line 1802
nsImapIncomingServer::DiscoveryDone(nsImapIncomingServer * const 0x03fa189c) 
line 1552
XPTC_InvokeByIndex(nsISupports * 0x03fa189c, unsigned int 0x00000004, unsigned 
int 0x00000000, nsXPTCVariant * 0x00000000) line 139
EventHandler(PLEvent * 0x03b56b70) line 509 + 41 bytes
PL_HandleEvent(PLEvent * 0x03b56b70) line 590 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00cf6900) line 520 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x003903f8, unsigned int 0x0000c0e2, unsigned 
int 0x00000000, long 0x00cf6900) line 1071 + 9 bytes
USER32! 77e13eb0()
USER32! 77e1401a()
USER32! 77e192da()

Comment 1

[Navin Gupta]

It is asserting because we want to get the filter-List on a non-root folder. 
the filter list should be only on the server. 

Comment 2

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

something to think about, news filters (when ever they come back) will be on
newsgroups (not on news servers)

Comment 3

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

weird, that look like on startup, during folder discover, I'm finding folders 
I'm not subscribed to.

so, we delete them, which exercises the code to check if deleting that folder 
should do a warning.  that seems ok.

but it looks like the existing code is trying to get only the filters for that 
given server.  shouldn't we be checking all filters for all server?  (a filter 
can filter messages from one server to another, right?  pop to local folders, 
for example.)
 
I don't think we should have GetFilterList() on nsMsgFolder

nsMsgDBFolder::ChangeFilterDestination should be going through something else 
and getting all filters and checking each one.

Comment 4

[Navin Gupta]

Attachment: proposed fix

Comment 5

[Navin Gupta]

The fix is to remove the assertion because we are getting the filter list 
from the server. It is not a per-folder thing. Also I have included the 
patch where if we have filters on pop/imap server pointing to local folders and 
the user deletes once such folder, we check the filters and warn the user. 
Seth and David, please review. 

Comment 6

[David :Bienvenu]

I don't understand why we're only looking at other servers filters if we don't
have any filters of our own.

+  rv = GetFilterList(getter_AddRefs(filterList));
+  if (!filterList || NS_FAILED(rv))

And why the check for type "none"?

Comment 7

[Navin Gupta]

Because "none" or local folders can be desination for filters set on other 
servers (pop/imap). 

Comment 8

[David :Bienvenu]

but why do we not do any of that code if we have a filter list? And you're only
checking for "none", you're not checking for local/pop accounts, right?

Comment 9

[Navin Gupta]

I thought we never allow cross-server filtering except for local folders, or
do we ?  "none" is only for local folders.

Comment 10

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

wait, this isn't the fix we discussed.

we discussed:

remove getFilterList() from the nsMsgDBFolder and add it to the
nsIMsgIncomingServer interface.

then, in nsMsgDBFolder::ChangeFilterDestination(), you get the
nsIMsgIncomingServer for the current folder, and call GetFilterList() on it.

the implementations for GetFilterList() would be as follows:

the base implementation (used by pop, imap and movemail) return the list of
filters for that server.

for nsNntpIncomingServer would override to return no filters.  (we'll change
this later when we support news filters.)

for nsNoIncomingServer, since there are no filters, you have to get all other
servers (from the account manager) and return the filter list of all the
combined filters.

Comment 11

[Navin Gupta]

seth, what you are saying is already in place. GetFilterList is on 
nsMsgIncomingServer and we have an additional attribute m_canHaveFilters
that tells us whether we support filters for that server. This attribute
is true for pop/imap and false for nntp/none

Comment 12

[Navin Gupta]

getFilterList() on nsMsgFolder gets the filterList on the server. 
It is a sort of helper function that gets the filter list for a 
folder. There is no reason why it should be removed. 

Comment 13

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

ah, I see getfilterlist() on the server already has a use.

what we want then is a new method on nsIMsgIncomingServer that will get the list
of filters we need to check when delete a folder on that server.

getFilterListToCheck()

then, in nsMsgDBFolder::ChangeFilterDestination(), you get the
nsIMsgIncomingServer for the current folder, and call getFilterListToCheck() on it.

the implementations for getFilterListToCheck() would be as follows:

the base implementation (used by pop, imap and movemail) return the list of
filters for that server.

for nsNntpIncomingServer would override to return no filters.  (we'll change
this later when we support news filters.)

for nsNoIncomingServer, since there are no filters, you have to get all other
servers (from the account manager) and return the filter list of all the
combined filters.

this way, you won't need to do that "none" check.  the code in
ChangeFilterDestination() would be generic.

Comment 14

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

I take that back.  discussing with naving:  pop, none and movemail behave the
same, since they are all local folders.

so, the impl of GetFilterListToCheck() in nsMsgIncomingServer.cpp, we get all
servers, call GetFilterList(), build up a giant nsIMsgFilterList, and return it.

the impl of GetFilterListToCheck() in nsImapIncomingServer.cpp calls
GetFilterList() and returns.

the impl of GetFilterFilterToCheck() in nsNntpIncomingServer.cpp returns null
because it can't be the destination of a filter.

Comment 15

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

naving has an even better idea.

make it so nsMsgDBFolder::ChangeFilterDestination() we iterate over all 
servers, get the filter list for each server and ChangeFilterTarget() on the 
filter lists.

sounds great navin.  can you attach a patch so I can review?
   

Comment 16

[Navin Gupta]

Attachment: proposed fix.

Comment 17

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

+    accountMgr->GetAllServers(getter_AddRefs(allServers));
should be:

     rv = accountMgr->GetAllServers(getter_AddRefs(allServers));

since you check rv right after it.

+        nsCOMPtr <nsIMsgIncomingServer> server = do_QueryInterface(serverSupports);
+        if (server && NS_SUCCEEDED(rv))

should either be

+        nsCOMPtr <nsIMsgIncomingServer> server =
do_QueryInterface(serverSupports, &rv);
+        if (server && NS_SUCCEEDED(rv))

or

+        nsCOMPtr <nsIMsgIncomingServer> server = do_QueryInterface(serverSupports);
+        if (server)

otherwise you are checking the wrong rv.

Comment 18

[Navin Gupta]

Attachment: revised patch

Comment 19

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

r/sr=sspitzer

Comment 20

[Cavin Song]

r=cavin.

Comment 21

[Navin Gupta]

fix checked in. 

Comment 22

[laurel]

OK with aug29 commercial trunk build.
I renamed folder(s) which were specified as filter destinations in other
server's filter.  Worked ok.