511609 - leak-avoiding nulling of nsIMsgFolder.msgDatabase kills message db enumerators, breaks gloda indexing

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[Andrew Sutherland [:asuth] (he/him)]

We current null out the msgDatabase on nsIMsgFolders all over the place to avoid ending up with all the folders open at the same time.

Unfortunately, this operation is more annoying than initially believed.  nsMsgDBFolder::SetMsgDatabase(null) calls nsMsgDatabase::ClearCachedHdrs() which calls nsMsgDatabase::ClearCachedObjects(PR_FALSE).  That dude kills all outstanding enumerators.  Because of how fixIterator works, gloda sees this as a normal termination and assumes it is done with the folder.

http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#950

At the least, I propose we only kill enumerators when dbGoingAway.  We might also be able to just stop calling ClearCachedHdrs().

Gloda would probably benefit from being more suspicious of the enumerator too, but we definitely need this failure case eliminated.

Comment 1

[David :Bienvenu]

I can look at this...it may be a bit more complicated because of the tangled reference web between the hdrs and the db - I think we will need to call ClearCachedHdrs.

Comment 2

[David :Bienvenu]

Attachment: proposed fix

I haven't looked at the gloda unit tests - I assume I could insert a folder.msgDatabase = null line in there somewhere and verify that indexing continues. I'll go look for a nice place for that...

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 395733 [details] [diff] [review]
proposed fix

Thank you for the speedy turnaround.

Probably the easiest way to inject the fault is to add logic to glodaTestHelper.js' messageCollectionListener.  It gets hooked up to a wildcard collection that gets notified about every new message as it gets indexed.  So if you just have a flag (possibly global) that nulls out GlodaIndexer._indexingFolder.msgDatabase and clears the flag, that should work.

Comment 4

[Mark Banner (:standard8)]

Having a test for this would definitely be a good idea.

Comment 5

[David :Bienvenu]

Attachment: tweak gloda tests to simulate bug

With this patched test case, gloda tests w/o the fix fail, and succeed with.

If we want, we could do a mod instead of a comparison to test clearing the db at regular intervals, which is why I used a counter.

Comment 6

[David :Bienvenu]

patch checked in - I'll land the test case once it passes review.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 395857 [details] [diff] [review]
tweak gloda tests to simulate bug

Thanks for the test!

Comment 8

[David :Bienvenu]

test changed pushed.