Closed Bug 1593582 Opened 5 months ago Closed 5 months ago

Stop using [array] in nsICollation on the Thunderbird side

Categories

(MailNews Core :: Backend, task, P1)

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 72.0

People

(Reporter: benc, Assigned: benc)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #1588260 +++

This bug is a place to hang patches for ridding nsICollation of xpidl [array] use on the Thunderbird side, so we can land them simultaneously with Bug #1588260.

Blocks: 1562158
Assignee: nobody → benc
Component: General → Backend
Priority: P3 → P1
Product: Thunderbird → MailNews Core
Summary: Stop using [array] in nsICollation → Stop using [array] in nsICollation on the Thunderbird side

A bit of a snowball patch - the changes kept on rolling downward, getting bigger and bigger...
But it's been very satisfying removing some of the manual memory management in there!

Running the unit tests locally seems good here. Just kicked off a try build to confirm:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bca646ee40990dd64d876ada5ee0987a605edbb1

Attachment #9107117 - Flags: review?(jorgk)

(doh - busted try build because I forgot the M-C patches... will look it up in the morning)

Thanks for that! I think that looks OK from a unit-test point of view...

That's the static analysis picking up the missing explicit, right?
I should be able to run the check locally but ./mach static-analysis check doesn't spit out any complaints. Hmm. I suspect I'm missing some arcane mach configuration somewhere...

Surely looks OK. If only changed this line:
https://hg.mozilla.org/try-comm-central/rev/02ce718031eab262160d4fad84fc48f1527a5e19#l2.17

I'll do the review tomorrow.

I'll leave the existing patch as is, but will roll your explicit change into whatever other changes I'll need to make after feedback.
Sorry it's such a sprawling patch :-(

Comment on attachment 9107117 [details] [diff] [review]
1593582-remove-xpidl-array-in-nsicollation-1.patch

Review of attachment 9107117 [details] [diff] [review]:
-----------------------------------------------------------------

You've got changes to these files: nsAbView.cpp | nsAbView.h | nsIMsgHdr.idl | nsMsgDBView.cpp | nsMsgDBView.h | nsMsgDBFolder.cpp | nsMsgDBFolder.h | nsIMsgDatabase.idl | nsMsgDatabase.h | nsMsgDatabase.cpp | nsMsgHdr.cpp.

I'll comment on nsAbView.cpp and nsMsgDBView.cpp, the rest looks fine.

::: 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?

@@ +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?

@@ -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?

@@ -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.

@@ +798,5 @@
>        directory.get() == mDirectory.get()) {
>      nsCOMPtr<nsIAbCard> addedCard = do_QueryInterface(item);
>      if (addedCard) {
>        // Malloc these from an arena
> +      AbCard *abcard = new AbCard(addedCard);

Same comment as above.

@@ -819,5 @@
> -      AbCard *abcard = (AbCard *)PR_Calloc(1, sizeof(struct AbCard));
> -      if (!abcard) return NS_ERROR_OUT_OF_MEMORY;
> -
> -      abcard->card = addedCard;
> -      NS_IF_ADDREF(abcard->card);

And here.

@@ +964,5 @@
>    int32_t index = FindIndexForCard(card);
>    if (index == -1) return NS_OK;
>  
>    AbCard *oldCard = mCards.ElementAt(index);
> +  AbCard *newCard = new AbCard(card);

And here.

@@ -992,5 @@
> -  AbCard *newCard = (AbCard *)PR_Calloc(1, sizeof(struct AbCard));
> -  if (!newCard) return NS_ERROR_OUT_OF_MEMORY;
> -
> -  newCard->card = card;
> -  NS_IF_ADDREF(newCard->card);

And here.

@@ +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.

::: mailnews/addrbook/src/nsAbView.h
@@ +22,5 @@
>  #include "nsMemory.h"
>  #include "nsIStringBundle.h"
>  
>  typedef struct AbCard {
> +  AbCard(nsIAbCard *c) : card(c) {}

You need explicit AbCard(nsIAbCard *c) : card(c) {} here (as already discussed).

::: 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?

@@ +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.

@@ +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?

(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)

  • nsMsgDBView: Added some notes on the viewSortInfo struct and primary/secondary sorting in general.
  • added the missing explicit on AbCard ctor.

False alarm on the memory leak I thought I'd spotted (the nsAbView::RemoveCardAt() does a delete on the AbCard - I thought the caller had to do it themselves).

Attachment #9107117 - Attachment is obsolete: true
Attachment #9107117 - Flags: review?(jorgk)
Attachment #9107793 - Flags: review?(jorgk)
Comment on attachment 9107793 [details] [diff] [review]
1593582-remove-xpidl-array-in-nsicollation-2.patch

Thanks for the explanations and extra comments. I think we're almost ready.

(In reply to Ben Campbell from comment #9)
> 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`.

Yes, if it's not too much hassle, move the view stuff back and do the same for the card stuff for consistency.

One last item:
> Explicit AddRef not needed now that ->card is a nsCOMPtr<>.
> The AbCard dtor handles releasing the nsCOMPtr<nsIAbCard>, so no explicit Release needed now.

Aha, so we're relying on the fact that in C++ land the DTOR if run on a struct considers that struct to be an object and runs the DTORs of the member variables.
Attachment #9107793 - Flags: review?(jorgk) → review+

Addressed my review comment. This is what should land.

Attachment #9107793 - Attachment is obsolete: true
Attachment #9107978 - Flags: review+

(In reply to Jorg K (GMT+2) from comment #12)

Created attachment 9107978 [details]
1593582-remove-xpidl-array-in-nsicollation-2.patch - JK

Addressed my review comment. This is what should land.

Thanks for that (got waylaid this morning, just catching up now).

(In reply to Jorg K (GMT+2) from comment #11)

Aha, so we're relying on the fact that in C++ land the DTOR if run on a
struct considers that struct to be an object and runs the DTORs of the
member variables.

Yes - no real difference between struct and class except for the default visibility of the members (public vs private), so they both work for RAII.
There's an argument for turning it into a class now (it's not just plain old data any more), but not a massively compelling one.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/e504cc780de9
Remove xpidl [array] usage in nsICollation. r=jorgk

Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 72.0
No longer blocks: 1562158
Blocks: 1551704
You need to log in before you can comment on or make changes to this bug.