424570 - ###!!! ASSERTION: shouldn't have any listeners: 'm_ChangeListeners.Length() == 0' after adding and removing a mailing list

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Mark Banner (:standard8)]

In writing the xpcshell test for bug 424567, I came across this assertion:

###!!! ASSERTION: shouldn't have any listeners: 'm_ChangeListeners.Length() == 0', file /mozilla/master/mozilla/mailnews/addrbook/src/nsAddrDatabase.cpp, line 168

The part of the tests causing the problem were:

1) Add a mail list.
2) Remove the mail list.

Note it has to be a mail list added in the same session.

I narrowed down the problem to:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/addrbook/src/nsAbMDBDirectory.cpp&rev=1.80&mark=718-719#710

Basically when we're adding the mailing list, we are also forcing it to add itself to the database listeners list. Then when the mailing list is removed, it doesn't remove itself from the database listeners list, because it hasn't got an mDatabase to itself.

There's a number of possible fixes and I'm not sure which one is the best yet. As its not a critical issue, I'm filing the bug to track it, but this may go away with the address book interface redesign.

Comment 1

[Mark Banner (:standard8)]

When this bug is fixed we need to enable the following lines of the mailing list test: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/addrbook/test/unit/test_mailList1.js&rev=1.1&mark=63-83#63

Comment 2

[Mark Banner (:standard8)]

Attachment: [checked in] The fix

This patch fixes the problem. The parent directory of the mailing list was adding the mailing list to the set of database listeners. The mailing list didn't know about, and neither the mailing list or the parent directory were removing the additional listener on shutdown.

I'm happy that removing the additional listener will work as whether the mailing list is queried (or modified), it will get the database and add itself as a listener then.

I've also done various testing to try and break this, and I haven't managed it yet. This will also help with various other mailing list bugs (including bug 406921).

Comment 3

[Mark Banner (:standard8)]

Patch checked in -> fixed

Comment 5

[Mark Banner (:standard8)]

Attachment: Follow-up - remove the listener hack when getting directories

Now that the mailing lists don't get extra additions to the database listeners, I've just remembered that we can drop the hack code that removes the mailing lists from the listeners when we do getDirectories. This should make things a little quicker on startup (though it obviously depends on what you've got defined).

Comment 6

[Mark Banner (:standard8)]

(In reply to comment #5)
> Created an attachment (id=317552) [details]
> Follow-up - remove the listener hack when getting directories
> 
> Now that the mailing lists don't get extra additions to the database listeners,
> I've just remembered that we can drop the hack code that removes the mailing
> lists from the listeners when we do getDirectories. This should make things a
> little quicker on startup (though it obviously depends on what you've got
> defined).

I forgot to mention, the fact I haven't removed this code doesn't have any effect on the current address book code - RemoveListener just fails but we don't check that value anyway. Therefore it is safe for this to wait until after the freeze for it to go in.

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 317552 [details] [diff] [review]
Follow-up - remove the listener hack when getting directories

I backed this out, its causing a test failure in test_collection.js. Not sure why. I'll raise a new bug for it and deal with it there.

Comment 8

[Mark Banner (:standard8)]

(In reply to comment #7)
> (From update of attachment 317552 [details] [diff] [review])
> I backed this out, its causing a test failure in test_collection.js. Not sure
> why. I'll raise a new bug for it and deal with it there.

Note: the initial reason for this bug is still fixed, that bit hasn't been backed out.