438035 - nsAbView does dangerous things to nsIAbDirectory objects and doesn't clean up itself as a listener

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect, P1)

Description

[Mark Banner (:standard8)]

nsAbView will re-initialise nsIAbDirectory objects via their nsIRDFResource. This isn't really very good, as we normally expect a different nsIRDFResource for each object, and don't expect them to mutate.

Although our js code protects against this happening for different address book types (e.g. mdb -> ldap), nsAbView doesn't. This could be bad.

Additionally nsAbView messes up adding and removing listeners which means we leave nsAbView objects hanging about until shutdown for search cases (I think it'll only be one nsAbView per different AB searched, but its still unnecessary).

Comment 1

[Mark Banner (:standard8)]

Attachment: The fix

Here's the fix. I have redone the function names in nsIAbView which should force any extensions that are using nsIAbView to update as appropriate (and hopefully read the new documentation).

Through some simplifications in the onItemRemoved listeners I have managed to remove the gCurDirectory global variable (the changes were needed anyway because the main change for this patch broke them both).

The listener additional/removal is correct as well so memory gets freed correctly at the right times.

Comment 2

[David :Bienvenu]

Comment on attachment 327587 [details] [diff] [review]
The fix

since you're changing these lines in the idl

+  void sortBy(in wstring sortColumn, in wstring sortDirection);

might as well use aSortColumn, aSortDirection...

similarly, 
+  nsIAbCard getCardFromRow(in long row);

Comment 3

[Mark Banner (:standard8)]

Attachment: The fix v2

Addressed David's comments.

Comment 4

[[e-mail]]

Comment on attachment 327789 [details] [diff] [review]
The fix v2

>+NS_IMETHODIMP nsAbView::SetView(nsIAbDirectory *aAddressBook,
>+                                  nsIAbViewListener *aAbViewListener,
>+                                  const nsAString &aSortColumn,
>+                                  const nsAString &aSortDirection,
>+                                  nsAString &aResult)
Nit: indentation wrong

>+    rv = GetCardValue(card, nsString(aSortColumn).get(), value);
I'd prefer if you used PromiseFlatString for this as it's clearer
(and more efficient while we're still on internal API!)

Comment 5

[Mark Banner (:standard8)]

Patch checked in -> fixed.