1711176 - nsMsgDatabase::Close() doesn't invalidate outstanding enumerators.

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Ben Campbell]

Attachment: fix_and_printf_debugging.patch

(Arising from Bug 1710768, comment 4)

nsMsgDatabase maintains a list of message enumerators it has given out, and when the database is killed, it tells all the enumerators to invalidate themselves.
It does this upon nsMsgDatabase destruction, and in ForceClosed().
But it doesn't currently do it in nsMsgDatabase::Close(), and it really seems like it should - it doesn't seem right to continue iterating over a closed database.

It's an easy fix... but then one of the gloda unit tests starts failing.

STEPS TO REPRODUCE:

Apply the fix:

+++ b/mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ -1425,4 +1425,5 @@ NS_IMETHODIMP nsMsgDatabase::Commit(nsMs
 
 NS_IMETHODIMP nsMsgDatabase::Close(bool forceCommit /* = TRUE */) {
+  InvalidateEnumerators();
   return CloseMDB(forceCommit);
 }

Run the test_index_messages_imap_offline.js test:

./mach xpcshell-test comm/mailnews/db/gloda/test/unit/test_index_messages_imap_offline.js

Observed:

The test_indexing_sweep fails, in the never before indexed folders subtest.
It never gets past mailnews/db/gloda/test/unit/base_index_messages.js:143.

From peppering the C++ and JS with printf debugging, I can tell it's trying to index 5 messages. The first one seems OK, but then when it goes to index the second one, the nsMsgDatabase::Close() is called, invalidating the iterators. The indexing stops and the unit test fails.

Here's a snippet of a run with the fix and my ugly printf debugging, annotated with #comments:

# the enumerator is created:
 0:02.90 pid:12216 XYZZY nsMsgDatabase[0x7f5603a93e40] GetFilterEnumerator => 0x7f5603c4bc80
# first message seems to index OK:
 0:02.90 pid:12216  foo3 0/5
 0:02.90 pid:12216 >>>  calling _indexMessage
 0:03.08 pid:12216 <<<  back from _indexMessage
# second message...
 0:03.09 pid:12216  foo3 1/5
 0:03.09 pid:12216 >>>  calling _indexMessage
# uh-oh... something is closing the database! bye bye enumerators...
 0:03.09 pid:12216 XYZZY nsMsgDatabase[0x7f5603a93e40] Close()
 0:03.09 pid:12216 XYZZY nsMsgDatabase[0x7f5603a93e40] invalidating 2/2 0x7f5603c4bc80
 0:03.09 pid:12216    XYZZY nsMsgDBEnumerator[0x7f5603c4bc80] Invalidate() (db=0x7f5603a93e40)
 0:03.09 pid:12216 XYZZY nsMsgDatabase[0x7f5603a93e40] invalidating 1/2 0x7f5603c4b4a0
 0:03.09 pid:12216    XYZZY nsMsgDBEnumerator[0x7f5603c4b4a0] Invalidate() (db=0x7f5603a93e40)
 0:03.24 pid:12216 <<<  back from _indexMessage
# the indexer tries to get the 3rd message, but the DB is now closed and the enumerator is now bogus:
 0:03.24 pid:12216 XYZZY nsMsgDBEnumerator[0x7f5603c4bc80] GetNext() on INVALID

# ...and the unit test fails because not all the expected messages were indexed.

And at this point I'm a little lost. I can't tell if gloda is actually doing something wrong, or if the unit test is doing something wrong...
The non-imap gloda tests seem to work OK, so perhaps theres some odd async racing happening where the test folder is removed before the indexing completes? I'm a little short on ideas now.

Comment 1

[Ben Campbell]

Just some notes, mostly to myself:

I don't think it's a problem with gloda or with the gloda tests. I think the issue is with the IMAP folder and how it manages it's database. During folder initialisation there's some crazy churn going on where the database is opened and closed 3 or 4 times. For example, calling the nsImapMailFolder::GetDatabase() can cause a database Close()!).

From the point of view of the tests, this manifests as a the function which sets up dummy folders and emails dropping folders during creation...

There's definitely some scope for vastly improving the life cycle of the DB managed by nsImapMailFolder, but it's all a bit twisty and entwined with all the other folder classes and will need some careful unpicking. But I think the main culprit is the Close() call in nsMsgDBFolder::ReadDBFolderInfo(), so I'm hoping I can fix that first.

Comment 2

[Ben Campbell]

Attachment: Bug 1711176 - Make nsMsgDatabase::Close() invalidate any outstanding enumerators. r=mkmelin

Includes a fix to nsMsgDBFolder::ReadDBFolderInfo(), where it was closing the
database while others were still using it.

Comment 3

[Ben Campbell]

Try build for the patch in comment 2:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=1187310b50182cda06ce761515e55e7b039dabdb&selectedTaskRun=EXQBGUlcRXern8ZcKDPMTw.0

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/993f9d222bd2
Make nsMsgDatabase::Close() invalidate any outstanding enumerators. r=mkmelin