421428 - nsIFolderListener should admit that it's passing nsIMsgFolders

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Joey Minta]

Attachment: patch v1

More fun rdf removal, this time removing the rdf-objects passed into and out of nsIFolderListeners.  All these objects are actually just nsIMsgFolders, and we should just admit that.

Comment 1

[[e-mail]]

Comment on attachment 307848 [details] [diff] [review]
patch v1

1. What about JS listeners?
2. You didn't fix the callers that wanted an nsIMsgFolder all along
3. Is there any point doing this before removing the account manager/folder data sources?

Comment 2

[Joey Minta]

(In reply to comment #1)
> 1. What about JS listeners?
At the moment, they may do extra QIs, but I don't think that should be fatal.  I'll audit them in the next round. (Subject to #3)

> 2. You didn't fix the callers that wanted an nsIMsgFolder all along
I'm not quite sure what you mean by this.

> 3. Is there any point doing this before removing the account manager/folder
> data sources?
The only point here is to avoid having the removal patch be a 500k mega-patch.  I was trying to break these things off.  We can WONTFIX this if you prefer to do it all at once though.

Comment 3

[Joey Minta]

Attachment: patch v2

I only found 2 js implementations of nsIFolderListener, and those have now been tweaked.  I also changed a bit of the few substantive implementations of functions that cared about the type of items they were dealing with.  (I think that should address #2.)

Comment 4

[[e-mail]]

(In reply to comment #2)
> (In reply to comment #1)
> > Is there any point doing this before removing the account manager/folder
> > data sources?
> The only point here is to avoid having the removal patch be a 500k mega-patch. 
I was only talking about the data sources, not all of the RDF support code.

(In reply to comment #3)
> I only found 2 js implementations of nsIFolderListener,
Well, you didn't look hard enough...

> I also changed a bit of the few substantive implementations of
> functions that cared about the type of items they were dealing with.
All the nontrivial functions care about the type of the items...

Comment 5

[Joey Minta]

Attachment: patch v3

Ok, this patch adds 2 more js consumers, the identical copies of messageWindow and msgMail3PaneWindow in mailnews/.  The SearchDialog and FilterEditor, and unit-test implementations don't care about the type of item they get.  According to http://mxr.mozilla.org/seamonkey/search?string=OnItemBoolPropertyChanged that's everyone.

Comment 6

[[e-mail]]

(In reply to comment #3)
> I also changed a bit of the few substantive implementations of
> functions that cared about the type of items they were dealing with.
> (I think that should address #2.)

Here's an example of what I mean:
(From update of attachment 312839 [details] [diff] [review])
> NS_IMETHODIMP
>-nsMessengerOSXIntegration::OnItemIntPropertyChanged(nsIRDFResource *aItem, nsIAtom *aProperty, PRInt32 aOldValue, PRInt32 aNewValue)
>+nsMessengerOSXIntegration::OnItemIntPropertyChanged(nsIMsgFolder *aItem, nsIAtom *aProperty, PRInt32 aOldValue, PRInt32 aNewValue)
> {
>   nsresult rv;
>   // if we got new mail bounce the Dock icon and/or apply badge to Dock icon
>   if (mBiffStateAtom == aProperty && mFoldersWithNewMail)
>   {
>     nsCOMPtr<nsIMsgFolder> folder = do_QueryInterface(aItem);
>     NS_ENSURE_TRUE(folder, NS_OK);

Comment 7

[Joey Minta]

Attachment: patch v4

OK, this gets rid of the superfluous QIs that the previous patch left in.

Comment 8

[[e-mail]]

Comment on attachment 322524 [details] [diff] [review]
patch v4

>     const char *itemURI = nsnull;
>     nsresult rv;
>-    rv = aItem->GetValueConst(&itemURI);
>+    nsCOMPtr<nsIRDFResource> resource = do_QueryInterface(aItem);
>+    NS_ENSURE_TRUE(resource, NS_OK);
>+    rv = resource->GetValueConst(&itemURI);
>     NS_ENSURE_SUCCESS(rv,rv);
IMHO you should change this to use aItem->GetURI

>+  nsCOMPtr<nsIRDFResource> resource(do_QueryInterface(aItem));
>   if (aProperty == kDefaultServerAtom)
>-    NotifyObservers(aItem, kNC_IsDefaultServer, kTrueLiteral, nsnull, aNewValue, PR_FALSE);
>+    NotifyObservers(resource, kNC_IsDefaultServer, kTrueLiteral, nsnull, aNewValue, PR_FALSE);
Nit: might want to QI after checking the property is the one you want

>   // and the folder being added or removed. Our flat data source root (i.e. mailnewsunreadfolders:/) has
>   // an arc with the child property to every folder in the data source.  We must change parentItem
>   // to be our data source root before calling nsMsgFolderDataSource::OnItemAddedOrRemoved. This ensures
>   // that datasource listeners such as the template builder properly handle add and remove
>   // notifications on the flat datasource.
>-  return nsMsgFolderDataSource::OnItemAddedOrRemoved(m_rootResource, item, added);
>+  nsCOMPtr<nsIMsgFolder> folder(do_QueryInterface(m_rootResource));
This isn't going to work, because m_rootResource isn't a folder

Comment 9

[Joey Minta]

Attachment: patch v5

Addresses the comments from the previous review.

Comment 10

[Mark Banner (:standard8)]

I'm just joining some dots up here, and apologies if I am wrong, in bug 296453, Andrew changed some checks from QI to instanceof in mail-folder-bindings.xml. According to bug 296453 comment 21:

> mail-folder-bindings.xml provides the XBL binding that gives us the "Move To"
> and "Copy To" menus that we love.  (Apparently recently XBLified on bug 413781,
> commit landed 2008/04/10.)  It adds itself as an nsIFolderListener to
> nsIMsgMailSession so that it can hear about changes to the folder structure
> that would invalidate its menu (additions/removals) or change the display of
> menu items (property changes, renames).
> 
> The fundamental problem with this is that the binding is fired for
> message-related events too, not just the folder events it cares about.

... snip ...

> The first bug is that the listener calls QueryInterface(C.i.nsIMsgFolder) on
> the passed-in item without using instanceof first.  This generates an exception
> for every message.  This is the cause of the memory explosion observed.

So, this hooks into nsIMsgMailSession with nsIFolderListener, and from those comments mail-folder-bindings.xml doesn't always get nsIMsgFolder items, which is what this bug/patch is implying nsIFolderListener actually gives you all the time.

So have I missed something? Apologies in advance, but please explain.

Comment 11

[[e-mail]]

I think the same notifications are used for messages as well as subfolders.

Comment 12

[Mark Banner (:standard8)]

(In reply to comment #11)
> I think the same notifications are used for messages as well as subfolders.
> 
Sorry my fault, I forgot to check which parameters were being checked/changed. There isn't a conflict here.

Comment 13

[[e-mail]]

Comment on attachment 325125 [details] [diff] [review]
patch v5

>     const char *itemURI = nsnull;
>     nsresult rv;
>-    rv = aItem->GetValueConst(&itemURI);
>+    rv = aItem->GetURI(&itemURI);
>     NS_ENSURE_SUCCESS(rv,rv);
> 
>     if (itemURI && mInboxURI.Equals(itemURI))
When I said "use GetURI" I didn't mean a textual substitution but adapting the code to work with the fact that the nsIMsgFolder method takes an nsACString& ...

Comment 14

[Joey Minta]

Attachment: patch v6 [Checkin: Comment 16]

Found some code to model what I think the uri changes need to be. Again, I'm not on windows, and my c++ is sketchy, so I'm kinda shooting in the dark on those changes. The rest of the patch remains unchanged, just updated for bit-rot.

Comment 15

[[e-mail]]

Comment on attachment 336849 [details] [diff] [review]
patch v6
[Checkin: Comment 16]

>-    const char *itemURI = nsnull;
>+    nsCString itemURI;
>     nsresult rv;
>-    rv = aItem->GetValueConst(&itemURI);
>+    rv = aItem->GetURI(itemURI);
>     NS_ENSURE_SUCCESS(rv,rv);
> 
>-    if (itemURI && mInboxURI.Equals(itemURI))
>+    if (!itemURI.IsEmpty() && mInboxURI.Equals(itemURI))
Actually you can get rid of the !itemURI.IsEmpty() && check. There are a number of reasons for this, but I won't bore you with them all unless you ask ;-)

Comment 16

[Joey Minta]

Checked in changeset 264:b5776223fe06

Comment 17

[Serge Gautherie (:sgautherie)]

(In reply to comment #15)
> Actually you can get rid of the !itemURI.IsEmpty() && check.

(In reply to comment #16)
> Checked in changeset 264:b5776223fe06

To be explicit, the review comment was addressed in the check-in:
http://hg.mozilla.org/comm-central/rev/b5776223fe06#l10.85