Closed Bug 438035 Opened 14 years ago Closed 14 years ago

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

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.1a1

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Keywords: memory-leak)

Attachments

(1 file, 1 obsolete file)

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).
Priority: -- → P1
Attached patch The fix (obsolete) — Splinter Review
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.
Attachment #327587 - Flags: superreview?(bienvenu)
Attachment #327587 - Flags: review?(neil)
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);
Attachment #327587 - Flags: superreview?(bienvenu) → superreview+
Attached patch The fix v2Splinter Review
Addressed David's comments.
Attachment #327587 - Attachment is obsolete: true
Attachment #327789 - Flags: superreview+
Attachment #327789 - Flags: review?(neil)
Attachment #327587 - Flags: review?(neil)
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!)
Attachment #327789 - Flags: review?(neil) → review+
Patch checked in -> fixed.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1a1
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.