Remove uses of PL_DHashTableEnumerate() in comm-central

RESOLVED FIXED in Thunderbird 42.0

Status

MailNews Core
Backend
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
Thunderbird 42.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Because PLDHashTable::Iterator is much nicer. I'm now *really* close to being
able to removing PL_DHashTableEnumerate() from the tree.
(Assignee)

Comment 1

2 years ago
Created attachment 8629170 [details] [diff] [review]
Remove uses of PL_DHashTableEnumerate() in comm-central
Attachment #8629170 - Flags: review?(rkent)

Comment 2

2 years ago
Comment on attachment 8629170 [details] [diff] [review]
Remove uses of PL_DHashTableEnumerate() in comm-central

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

Looks good to me with a couple of retained nullptr checks. r+ with the suggested changes.

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +636,5 @@
>      PLDHashTable  *saveCachedHeaders = m_cachedHeaders;
>      m_cachedHeaders = nullptr;
> +    for (auto iter = saveCachedHeaders->Iter(); !iter.Done(); iter.Next()) {
> +      auto element = static_cast<MsgHdrHashElement*>(iter.Get());
> +      NS_IF_RELEASE(element->mHdr);

We've had a lot of trouble with crashing in msgDatabase, so you need to leave the null check. That is, something like:

if (element && element->mHdr)
  NS_RELEASE(element->mHdr)

@@ +778,5 @@
>      // clear mdb row pointers of any headers still in use, because the
>      // underlying db is going away.
> +    for (auto iter = m_headersInUse->Iter(); !iter.Done(); iter.Next()) {
> +      auto element = static_cast<const MsgHdrHashElement*>(iter.Get());
> +      if (element->mHdr) {

Same null check here,

if (element && element->mHdr) {
Attachment #8629170 - Flags: review?(rkent) → review+
(Assignee)

Comment 3

2 years ago
> We've had a lot of trouble with crashing in msgDatabase, so you need to
> leave the null check. That is, something like:
> 
> if (element && element->mHdr)
>   NS_RELEASE(element->mHdr)

This null check is a placebo. PLDHashTable::Iterator::Get() never returns a nullptr. If you think about it, that wouldn't make much sense -- Get() is returning a pointer to a live entry in the hash table. And if there aren't any more live entries to iterate, then Done() will be false and you shouldn't be calling Get(). (Indeed, Get() has an assertion that Done() is true.)

There are literally hundreds of Get() calls in the codebase and I think this is the only one that does a nullptr check.

But if you really want the check, I'll add it back in.
(Assignee)

Comment 4

2 years ago
Created attachment 8629867 [details] [diff] [review]
Remove uses of PL_DHashTableEnumerate() in comm-central

Here's the same patch but with the null checks.
Attachment #8629867 - Flags: review?(rkent)
(Assignee)

Updated

2 years ago
Attachment #8629170 - Attachment is obsolete: true

Comment 5

2 years ago
The null checks were originally added specifically because there were crashes occurring that were interpreted as caused by a null entry. Spending just a couple of minutes looking at the hash code, I could not convince myself that a nullptr return is impossible. We also tend to be much more conservative about null checks in comm-central than is Firefox.

So given that the cost is minimal, and there is history of needing this, I'd like to just keep this in rather than spend time trying to understand why a null return is impossible.

I'll go ahead an land this.

Comment 6

2 years ago
Comment on attachment 8629867 [details] [diff] [review]
Remove uses of PL_DHashTableEnumerate() in comm-central

http://hg.mozilla.org/comm-central/rev/b8fa88ed19bc
Attachment #8629867 - Flags: review?(rkent) → review+

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 42.0
You need to log in before you can comment on or make changes to this bug.