527405 - Post commit error - indexing needs to track folder rebuilds

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[Ludovic Hirlimann [:Usul]]

I don't have STRs yet but with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.6pre) Gecko/20091107 Shredder/3.0pre I spotted  the following error in my Error console :

2009-11-08 14:39:38	gloda.ds.pch	ERROR	PostCommitHandler callback (undefined:156) threw: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIMsgDBHdr.getUint32Property]"  nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame :: file:///Applications/Shredder.app/Contents/MacOS/modules/gloda/index_msg.js :: PendingCommitTracker_commitCallback :: line 156"  data: no]

This might have been triggered by the rebuild of an index - which I did around that time I think.

Comment 1

[Ludovic Hirlimann [:Usul]]

(In reply to comment #0)

> This might have been triggered by the rebuild of an index - which I did around
> that time I think.

Probably unlikely, as I just rebuild te same index as yesterday and the message didn't show in the console.

Comment 2

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

The event sequence probably went like this:
1) Gloda indexes messages in folder A.
2) You cause a rebuild of the msg for folder A.
3) Gloda commits the transaction that contains the messages for folder A.
4) Gloda tries to fix-up the message headers from folder A.  Message header is no longer valid, exception happens.

I probably lost the protection for this case when I refactored from per-message commit trackers to a single per-transaction commit helper.

The compaction unit test doesn't show this problem because we explicitly clean out all the message headers when we hear about the compaction pass, so it can't happen.

We should probably listen for an event when the rebuild gets triggered so we can handle it similarly to the compaction case.  I do not know if such an event exists off the top of my head.

We should likely also either check for mooted headers and ignore them or wrap the processing of each message in a try block (with error reporting!) so that one bad header doesn't spoil the bunch and break the PermitCommitTracker for the session.

Requesting blocking since the perma-break of PermitCommitTracker is less than ideal and it avoids introducing the latency of asking for patch approval.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

We can't block just for an exception in the error console.  What would the user-visible consequences of leaving the PermitCommitTracker broken be?  How complex is the fix likely to be and how long would it likely take?

Comment 4

[David :Bienvenu]

I'll let Andrew be more specific, but in my experience, if the CommitTracker isn't correct, lots of subtle bugs can get introduced, because Gloda doesn't accurately know the state of messages.

Comment 5

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

It will result in a relatively unbounded memory leak as we end up holding onto every header that gloda ever sees.  This will result in us leaking the databases that do not get ForceClosed.  We will never write that messages were indexed to their folders so the next time gloda starts up it will end up reindexing every message from the point that PendingCommitTracker broke onwards.

Gloda actually will continue to generally operate correctly since the PendingCommitTracker now doesn't screw up when messages get marked dirty.

Adding an appropriate try wrapper will take a few minutes.

Adding the notification event for reindexing should only take a short period of time to investigate.  If it's hard we can punt on that and the previous fix will save us.

Comment 6

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

Attachment: Adding notification the clears out PendingCommitTracker, add try block on header access v1

Comment 7

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

Comment on attachment 411337 [details] [diff] [review]
Adding notification the clears out PendingCommitTracker, add try block on header access v1

uh, Ci does not exist problem.

Comment 8

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

Attachment: Adding notification the clears out PendingCommitTracker, add try block on header access v2

Actually tested from the UI this time.

Comment 9

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

pushed to trunk:
http://hg.mozilla.org/comm-central/rev/ee3426ab0412

pushed to comm-1.9.1:
http://hg.mozilla.org/releases/comm-1.9.1/rev/a77ccafc92fe