Closed Bug 438035 Opened 14 years ago Closed 14 years ago
Ab View does dangerous things to ns IAb Directory objects and doesn't clean up itself as a listener
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).
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 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+
Addressed David's comments.
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
You need to log in before you can comment on or make changes to this bug.