reduce exposed API of nsMsgHdr

RESOLVED FIXED in Thunderbird 65.0

Status

enhancement
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

Trunk
Thunderbird 65.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

The class nsMsgHdr implementing nsIMsgDBHdr is exposing some methods as public even if there is no user (internal methods, not in the public API of nsIMsgDBHdr).
It also allows access to some internal m_ members from external callers, which should use the available proper accessors.
Posted patch 1508054.patch (obsolete) — Splinter Review
Attachment #9025884 - Flags: review?(jorgk)
Comment on attachment 9025884 [details] [diff] [review]
1508054.patch

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

I stopped looking when I found the ref counting problem.

::: mailnews/db/msgdb/public/nsMsgHdr.h
@@ +19,5 @@
>  class nsMsgHdr : public nsIMsgDBHdr {
>  public:
>      NS_DECL_NSIMSGDBHDR
>      friend class nsMsgDatabase;
> +    friend class nsImapMailDatabase;

Wow, IMAP??

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +2010,5 @@
>  nsMsgDatabase::UndoDelete(nsIMsgDBHdr *aMsgHdr)
>  {
> +  if (aMsgHdr)
> +  {
> +    // force deleted flag, so SetHdrFlag won't bail out because  deleted flag isn't set

Need to fix this comment: Upper case, double space, full stop.

@@ +2013,5 @@
> +  {
> +    // force deleted flag, so SetHdrFlag won't bail out because  deleted flag isn't set
> +    uint32_t result;
> +    aMsgHdr->OrFlags(nsMsgMessageFlags::Expunged, &result);
> +    SetHdrFlag(aMsgHdr, false, nsMsgMessageFlags::Expunged); // clear deleted flag in db

Two spaces before the // (as linting requires it). No harm in uppercase and full stop here.

@@ -5010,2 @@
>        printf("hdr key = %u, author = %s subject = %s\n", key, author.get(), subject.get());
> -      NS_RELEASE(msgHdr);

This isn't right. This header handling is *extremely* ugly in the way it handles references. The addref happens here:
https://searchfox.org/comm-central/rev/c2cff130f8467671b9fdeb2811a1647afdef484f/mailnews/db/msgdb/src/nsMsgDatabase.cpp#704
or here
https://searchfox.org/comm-central/rev/c2cff130f8467671b9fdeb2811a1647afdef484f/mailnews/db/msgdb/src/nsMsgDatabase.cpp#516
which is called from GetMsgHdrForKey(). You must release here!

Would
nsCOMPtr<nsIMsgDBHdr> msg;
rv = GetMsgHdrForKey(key, getter_AddRefs(msg));
compile. Then the smart pointer will take care of it.
Attachment #9025884 - Flags: review?(jorgk)
Posted patch 1508054.patch v1.1 (obsolete) — Splinter Review
Thanks for catching that release problem.
Attachment #9025884 - Attachment is obsolete: true
Attachment #9025908 - Flags: review?(jorgk)
Thanks. I've reduced some indentation.
Attachment #9025908 - Attachment is obsolete: true
Attachment #9025908 - Flags: review?(jorgk)
Attachment #9026089 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0a1e5f9bc313
reduce exposed API of nsMsgHdr. r=jorgk
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 65.0
You need to log in before you can comment on or make changes to this bug.