Closed Bug 476911 Opened 11 years ago Closed 11 years ago

Investigate closing of backup message database

Categories

(MailNews Core :: Database, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: rkent, Assigned: rkent)

Details

Attachments

(1 file, 2 obsolete files)

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.
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)
Whiteboard: [needs r neil, sr bienvenu]
(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?
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.
(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.
Although you'd need to forward-declare nsParseMailMessageState.
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 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+
Attachment #364710 - Flags: review?(neil) → review-
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)
Whiteboard: [needs r neil, sr bienvenu] → [needs sr bienvenu]
Attachment #365209 - Flags: superreview?(bienvenu) → superreview+
Keywords: checkin-needed
Whiteboard: [needs sr bienvenu]
Checked in: http://hg.mozilla.org/comm-central/rev/ae12d4b3153d
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.