Last Comment Bug 527405 - Post commit error - indexing needs to track folder rebuilds
: Post commit error - indexing needs to track folder rebuilds
Status: RESOLVED FIXED
[gloda key]
: fixed-seamonkey2.0.1
Product: MailNews Core
Classification: Components
Component: Database (show other bugs)
: 1.9.1 Branch
: All All
: -- normal (vote)
: Thunderbird 3.0rc1
Assigned To: Andrew Sutherland [:asuth]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-11-08 22:22 PST by Ludovic Hirlimann [:Usul]
Modified: 2010-11-01 09:55 PDT (History)
4 users (show)
dmose: blocking‑thunderbird3+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Adding notification the clears out PendingCommitTracker, add try block on header access v1 (11.32 KB, patch)
2009-11-09 17:42 PST, Andrew Sutherland [:asuth]
no flags Details | Diff | Splinter Review
Adding notification the clears out PendingCommitTracker, add try block on header access v2 (11.36 KB, patch)
2009-11-09 17:51 PST, Andrew Sutherland [:asuth]
mozilla: review+
Details | Diff | Splinter Review

Description Ludovic Hirlimann [:Usul] 2009-11-08 22:22:54 PST
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] 2009-11-08 22:27:59 PST
(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] 2009-11-08 22:43:57 PST
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 (:dmose) 2009-11-09 10:04:45 PST
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 2009-11-09 12:54:45 PST
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] 2009-11-09 15:16:34 PST
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] 2009-11-09 17:42:16 PST
Created attachment 411337 [details] [diff] [review]
Adding notification the clears out PendingCommitTracker, add try block on header access v1
Comment 7 Andrew Sutherland [:asuth] 2009-11-09 17:48:43 PST
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] 2009-11-09 17:51:39 PST
Created attachment 411342 [details] [diff] [review]
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] 2009-11-09 20:58:04 PST
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

Note You need to log in before you can comment on or make changes to this bug.