If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Investigate closing of backup message database

RESOLVED FIXED in Thunderbird 3.0b3

Status

MailNews Core
Database
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: rkent, Assigned: rkent)

Tracking

Trunk
Thunderbird 3.0b3

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

9 years ago
Bug 464419 revealed that the backup database in nsParseMailMessageState does not have a listener, so it does not close properly with ForceClosed. It's also peculiar that the backup database does not get removed automatically when used during the creation of a Gmail IMAP account. Investigate.
(Assignee)

Comment 1

9 years ago
Created attachment 364710 [details] [diff] [review]
Add db listeners for references to backup db

I added db listeners to make sure that we can remove the backup db references. I decided to share listeners that were also used for the main db. I think that's OK, because the usage of the backup db is very limited, and we can tell which db is giving the onAnnouncerGoingAway() calls.

I had some troubles moving the db listener from nsMsgMailboxParser to nsParseMailMessageState. When I tried to make nsParseMailMessageState like the previous nsMsgMailboxParser, using multiple interface inheritance like

  class nsParseMailMessageState : public nsIMsgParseMailMsgState, public nsIDBChangeListener

then I would get compiler complaints about ambiguous nsISupports references when do_QueryInterface was called on nsParseMailMessageState objects. I solved that by converting multiple inheritance to an inheritance chain.

This whole process makes it clear that the onAnnouncerGoingAway listener is a quite different animal than the other calls to the db listener. I wonder if perhaps we should separate it into a separate interface at some point - or maybe switch to weak references or something. Adding this listener was a lot more work than it should have been. Still, I think this is what we need to do, as without the OnAnnouncerGoingAway() we cannot reliably close the backup db.
Attachment #364710 - Flags: superreview?(bienvenu)
Attachment #364710 - Flags: review?(neil)
(Assignee)

Updated

9 years ago
Whiteboard: [needs r neil, sr bienvenu]

Comment 2

9 years ago
(In reply to comment #1)
> I had some troubles moving the db listener from nsMsgMailboxParser to
> nsParseMailMessageState. When I tried to make nsParseMailMessageState like the
> previous nsMsgMailboxParser, using multiple interface inheritance like
> 
>   class nsParseMailMessageState : public nsIMsgParseMailMsgState, public
> nsIDBChangeListener
> 
> then I would get compiler complaints about ambiguous nsISupports references
> when do_QueryInterface was called on nsParseMailMessageState objects. I solved
> that by converting multiple inheritance to an inheritance chain.
Chances are that's not actually the best solution. I had a quick look at nsParseMailbox.cpp and I think that where the code uses
nsCOMPtr<nsISupports> mySupports = do_QueryInterface(/*stuff*/(this));
it should really use
nsCOMPtr<nsISupports> mySupports;
CallQueryInterface(this, getter_AddRefs(mySupports));
Or are you seeing a different problem?
(Assignee)

Comment 3

9 years ago
The compile error was occurring in these statements, which appear twice in nsLocalMailFolder.cpp:

mCopyState->m_parseMsgState = do_QueryInterface(parseMsgState, &rv);

I tried two different solutions, both of which seemed to work. One was the inheritance chain in the patch. The second was adding in the nsDBChangeListener interface with multiple inheritance as I mention in the comments, and simply removing the do_QueryInterface at these two calls, which seems to be close to what you are suggesting. I'm really not sure what is the correct approach - which is why I mentioned it in my comments. I would appreciate any input you can give me, Neil.

Comment 4

9 years ago
(In reply to comment #3)
> The compile error was occurring in these statements, which appear twice in
> nsLocalMailFolder.cpp:
> 
> mCopyState->m_parseMsgState = do_QueryInterface(parseMsgState, &rv);

I fixed a similar bug for m_undoMsgTxn; the solution is to change it from an nsCOMPtr<nsIMsgParseMailMsgState> to a nsRefPtr<nsParseMailMessageState> which eliminates the need for the do_QueryInterface.

Comment 5

9 years ago
Although you'd need to forward-declare nsParseMailMessageState.
(Assignee)

Comment 6

9 years ago
Created attachment 364953 [details] [diff] [review]
Variant with multiple inheritance

Here's the variant with a multiple interface, using an nsRefPtr as suggested by Neil. I'll leave the other patch un-obsoleted so that you can tell me which one you prefer.
Attachment #364953 - Flags: superreview?(bienvenu)
Attachment #364953 - Flags: review?(neil)

Comment 7

9 years ago
Comment on attachment 364953 [details] [diff] [review]
Variant with multiple inheritance

Oh yes, I really like this version :-)

> NS_IMETHODIMP
>-nsMsgMailboxParser::OnHdrPropertyChanged(nsIMsgDBHdr *aHdrToChange, PRBool aPreChange, PRUint32 *aStatus, 
>+nsParseMailMessageState::OnHdrPropertyChanged(nsIMsgDBHdr *aHdrToChange, PRBool aPreChange, PRUint32 *aStatus, 
Nit: the old line ended with a space, might as well remove it.
Attachment #364953 - Flags: review?(neil) → review+

Updated

9 years ago
Attachment #364710 - Flags: review?(neil) → review-
(Assignee)

Comment 8

9 years ago
Created attachment 365209 [details] [diff] [review]
Removed trailing spaces, and shortened lines

In addition to removing the trailing space, I also shortened those lines, as they were way long. Carrying over Neil's r+
Attachment #364710 - Attachment is obsolete: true
Attachment #364953 - Attachment is obsolete: true
Attachment #365209 - Flags: superreview?(bienvenu)
Attachment #365209 - Flags: review+
Attachment #364710 - Flags: superreview?(bienvenu)
Attachment #364953 - Flags: superreview?(bienvenu)
(Assignee)

Updated

9 years ago
Whiteboard: [needs r neil, sr bienvenu] → [needs sr bienvenu]

Updated

9 years ago
Attachment #365209 - Flags: superreview?(bienvenu) → superreview+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Whiteboard: [needs sr bienvenu]
Checked in: http://hg.mozilla.org/comm-central/rev/ae12d4b3153d
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.