(In reply to Jorg K (GMT+2) from comment #8) > 1593582-remove-xpidl-array-in-nsicollation-1.patch > ::: mailnews/addrbook/src/nsAbView.cpp > @@ +97,2 @@ > > mCards.RemoveElementAt(row); > > + delete abcard; > > Our (2nd in command) MailNews reviewer Kent James (now retired) always used > to pick on two things: Error management and memory management. > > When I see an NS_IF_ADDREF() or worse, an NS_IF_RELEASE() (in the removed > code) then the alarm lights about "home-grown" ref-counting go on. > > Why do we delete the card here and not let ref-counting take care of that? It's a helper struct `AbCard` (internal to `nsAbView`) which associates collation keys with a `nsIAbCard` so they can be sorted. Can't use a `RefPtr<>` on it because it lacks `AddRef` and `Release` methods. I think it's an optimisation - calculate the collation keys once in advance rather than multiple times on the fly as the sort runs. You make a valid point though: I removed the need for manual memory management *within* `AbCard`, but haven't done anything to tidy up lifetime management of `AbCard` itself. And while I was composing this reply I'm pretty sure I spotted a memory leak (already present in the old version!). I've got a couple of ideas on how to better manage AbCards, but maybe it's better handled in a followup? > @@ +286,5 @@ > > while (NS_SUCCEEDED(cardsEnumerator->HasMoreElements(&more)) && more) { > > rv = cardsEnumerator->GetNext(getter_AddRefs(item)); > > if (NS_SUCCEEDED(rv)) { > > nsCOMPtr<nsIAbCard> card = do_QueryInterface(item); > > + AbCard *abcard = new AbCard(card); > > That looks suspicious. Why not use a RefPtr? And get rid of the delete above? No `AddRef()`/`Release()` support on AbCard. > @@ -293,5 @@ > > - // Malloc these from an arena > > - AbCard *abcard = (AbCard *)PR_Calloc(1, sizeof(struct AbCard)); > > - if (!abcard) return NS_ERROR_OUT_OF_MEMORY; > > - > > - abcard->card = card; > > Why is it OK to remove this? What is ->card used for? `->card` is the `nsIAbCard` the `AbCard` is associated with. It's now passed in via the `AbCard` constructor. `->card` was a raw `nsIAbCard *`, but it's now a `nsCOMPtr<nsIAbCard>`. > @@ -294,5 @@ > > - AbCard *abcard = (AbCard *)PR_Calloc(1, sizeof(struct AbCard)); > > - if (!abcard) return NS_ERROR_OUT_OF_MEMORY; > > - > > - abcard->card = card; > > - NS_IF_ADDREF(abcard->card); > > Looks like a reference was explicitly added here to make sure this thing > doesn't die unexpectedly. Explicit AddRef not needed now that `->card` is a `nsCOMPtr<>`. > @@ +984,4 @@ > > // No need to remove and add, since the collation keys haven't changed. > > // Since they haven't changed, the card will sort to the same place. > > // We just need to clean up what we allocated. > > + delete newCard; > > and here. The `AbCard` dtor handles releasing the `nsCOMPtr<nsIAbCard>`, so no explicit Release needed now. > ::: mailnews/base/src/nsMsgDBView.cpp > @@ +77,5 @@ > > > > static void UpdateCachedName(nsIMsgDBHdr *aHdr, const char *header_field, > > const nsAString &newName); > > > > +// Helper struct for sorting by numeric fields. > > This was in the .h file before, so why move it? It's a helper class for the implementation of `nsMsgDBView` and not part of the public interface, so it seemed prudent to move it over to the `.cpp` file. I probably should have left it to reduce diff-churn. To be consistent I should have done the same for `AbCard` above - I didn't notice it was private to `nsAbView`. Got any preference about where such helper classes should live? > @@ +4269,5 @@ > > > > + // We'll use a tweaked copy of the primary context. > > + viewSortInfo ctx = *comparisonContext; > > + ctx.isSecondarySort = true; // To avoid recursing back here! > > + ctx.ascendingSort = (sortOrder == nsMsgViewSortOrder::ascending); > > What is that ctx magic about? It's not quite clear from the snippets in the > patch here. The viewSortInfo is context passed into the sort comparison functions. Those functions are used twice - firstly for the primary ordering, during the Sort(). If the comparison function finds two elements with equal ordering, they call `SecondaryCompare()` to break the deadlock. `SecondaryCompare()` uses the same comparison functions as the primary, but using the secondary key and potentially with different criteria (eg secondary sort might be descending rather than ascending). The `viewSortInfo::isSecondarySort` lets the comparison functions know not to call `SecondaryCompare()` again (and again and again...) Previously, the same `viewSortInfo` was used for secondary as for primary. Some of the fields were modified before (re)calling the comparison function, then restored afterward. I changed it to just take a copy and modify that instead, so no restoration required. I'll add some more comments to try and clarify it. > @@ +4456,5 @@ > > + rv = GetLongField(msgHdr, sortType, &info->dword, colHandler); > > + NS_ENSURE_SUCCESS(rv, rv); > > + } > > + } > > + // Perform the sort. > > The code that follows looks the same as the one above. Can what be merged? I agonised over that for a bit and decided in the end that it was clearer to just replicate that chunk twice. (The complication is that `pPtrBase` is tracking different types for each branch, so even though the code looks identical it's actually operating on different types. In fact the two types are binary compatible where it matters for that section, so I definitely _could_ do some casting to merge them, but I figured that it was clearer avoid that and keep the paths separate). (will upload revised patch soon to add the missing explicit, fix the memory leak and clarify the context stuff)
Bug 1593582 Comment 9 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
(In reply to Jorg K (GMT+2) from comment #8) ``` > 1593582-remove-xpidl-array-in-nsicollation-1.patch > ::: mailnews/addrbook/src/nsAbView.cpp > @@ +97,2 @@ > > mCards.RemoveElementAt(row); > > + delete abcard; ``` > Our (2nd in command) MailNews reviewer Kent James (now retired) always used > to pick on two things: Error management and memory management. > > When I see an NS_IF_ADDREF() or worse, an NS_IF_RELEASE() (in the removed > code) then the alarm lights about "home-grown" ref-counting go on. > > Why do we delete the card here and not let ref-counting take care of that? It's a helper struct `AbCard` (internal to `nsAbView`) which associates collation keys with a `nsIAbCard` so they can be sorted. Can't use a `RefPtr<>` on it because it lacks `AddRef` and `Release` methods. I think it's an optimisation - calculate the collation keys once in advance rather than multiple times on the fly as the sort runs. You make a valid point though: I removed the need for manual memory management *within* `AbCard`, but haven't done anything to tidy up lifetime management of `AbCard` itself. And while I was composing this reply I'm pretty sure I spotted a memory leak (already present in the old version!). I've got a couple of ideas on how to better manage AbCards, but maybe it's better handled in a followup? ``` > @@ +286,5 @@ > > while (NS_SUCCEEDED(cardsEnumerator->HasMoreElements(&more)) && more) { > > rv = cardsEnumerator->GetNext(getter_AddRefs(item)); > > if (NS_SUCCEEDED(rv)) { > > nsCOMPtr<nsIAbCard> card = do_QueryInterface(item); > > + AbCard *abcard = new AbCard(card); ``` > That looks suspicious. Why not use a RefPtr? And get rid of the delete above? No `AddRef()`/`Release()` support on AbCard. ``` > @@ -293,5 @@ > > - // Malloc these from an arena > > - AbCard *abcard = (AbCard *)PR_Calloc(1, sizeof(struct AbCard)); > > - if (!abcard) return NS_ERROR_OUT_OF_MEMORY; > > - > > - abcard->card = card; ``` > Why is it OK to remove this? What is ->card used for? `->card` is the `nsIAbCard` the `AbCard` is associated with. It's now passed in via the `AbCard` constructor. `->card` was a raw `nsIAbCard *`, but it's now a `nsCOMPtr<nsIAbCard>`. ``` > @@ -294,5 @@ > > - AbCard *abcard = (AbCard *)PR_Calloc(1, sizeof(struct AbCard)); > > - if (!abcard) return NS_ERROR_OUT_OF_MEMORY; > > - > > - abcard->card = card; > > - NS_IF_ADDREF(abcard->card); ``` > Looks like a reference was explicitly added here to make sure this thing > doesn't die unexpectedly. Explicit AddRef not needed now that `->card` is a `nsCOMPtr<>`. ``` > @@ +984,4 @@ > > // No need to remove and add, since the collation keys haven't changed. > > // Since they haven't changed, the card will sort to the same place. > > // We just need to clean up what we allocated. > > + delete newCard; ``` > and here. The `AbCard` dtor handles releasing the `nsCOMPtr<nsIAbCard>`, so no explicit Release needed now. ``` > ::: mailnews/base/src/nsMsgDBView.cpp > @@ +77,5 @@ > > > > static void UpdateCachedName(nsIMsgDBHdr *aHdr, const char *header_field, > > const nsAString &newName); > > > > +// Helper struct for sorting by numeric fields. ``` > This was in the .h file before, so why move it? It's a helper class for the implementation of `nsMsgDBView` and not part of the public interface, so it seemed prudent to move it over to the `.cpp` file. I probably should have left it to reduce diff-churn. To be consistent I should have done the same for `AbCard` above - I didn't notice it was private to `nsAbView`. Got any preference about where such helper classes should live? ``` > @@ +4269,5 @@ > > > > + // We'll use a tweaked copy of the primary context. > > + viewSortInfo ctx = *comparisonContext; > > + ctx.isSecondarySort = true; // To avoid recursing back here! > > + ctx.ascendingSort = (sortOrder == nsMsgViewSortOrder::ascending); ``` > What is that ctx magic about? It's not quite clear from the snippets in the > patch here. The viewSortInfo is context passed into the sort comparison functions. Those functions are used twice - firstly for the primary ordering, during the Sort(). If the comparison function finds two elements with equal ordering, they call `SecondaryCompare()` to break the deadlock. `SecondaryCompare()` uses the same comparison functions as the primary, but using the secondary key and potentially with different criteria (eg secondary sort might be descending rather than ascending). The `viewSortInfo::isSecondarySort` lets the comparison functions know not to call `SecondaryCompare()` again (and again and again...) Previously, the same `viewSortInfo` was used for secondary as for primary. Some of the fields were modified before (re)calling the comparison function, then restored afterward. I changed it to just take a copy and modify that instead, so no restoration required. I'll add some more comments to try and clarify it. ``` > @@ +4456,5 @@ > > + rv = GetLongField(msgHdr, sortType, &info->dword, colHandler); > > + NS_ENSURE_SUCCESS(rv, rv); > > + } > > + } > > + // Perform the sort. ``` > The code that follows looks the same as the one above. Can what be merged? I agonised over that for a bit and decided in the end that it was clearer to just replicate that chunk twice. (The complication is that `pPtrBase` is tracking different types for each branch, so even though the code looks identical it's actually operating on different types. In fact the two types are binary compatible where it matters for that section, so I definitely _could_ do some casting to merge them, but I figured that it was clearer avoid that and keep the paths separate). (will upload revised patch soon to add the missing explicit, fix the memory leak and clarify the context stuff)