Post commit error - indexing needs to track folder rebuilds

RESOLVED FIXED in Thunderbird 3.0rc1

Status

MailNews Core
Database
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Usul, Assigned: asuth)

Tracking

({fixed-seamonkey2.0.1})

1.9.1 Branch
Thunderbird 3.0rc1
fixed-seamonkey2.0.1
Bug Flags:
blocking-thunderbird3 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [gloda key])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

8 years ago
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.
(Reporter)

Updated

8 years ago
Whiteboard: [gloda key]
(Reporter)

Comment 1

8 years ago
(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.
(Assignee)

Comment 2

8 years ago
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.
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
Flags: blocking-thunderbird3?
OS: Mac OS X → All
Hardware: x86 → All
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

8 years ago
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.
(Assignee)

Comment 5

8 years ago
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.

Updated

8 years ago
Flags: blocking-thunderbird3? → blocking-thunderbird3+

Updated

8 years ago
Target Milestone: --- → Thunderbird 3.0rc1
(Assignee)

Comment 6

8 years ago
Created attachment 411337 [details] [diff] [review]
Adding notification the clears out PendingCommitTracker, add try block on header access v1
Attachment #411337 - Flags: review?(bienvenu)
(Assignee)

Updated

8 years ago
Whiteboard: [gloda key] → [gloda key][has patch needs review by bienvenu]
(Assignee)

Comment 7

8 years ago
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.
Attachment #411337 - Attachment is obsolete: true
Attachment #411337 - Flags: review?(bienvenu)
(Assignee)

Comment 8

8 years ago
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.
Attachment #411342 - Flags: review?(bienvenu)

Updated

8 years ago
Attachment #411342 - Flags: review?(bienvenu) → review+

Updated

8 years ago
Whiteboard: [gloda key][has patch needs review by bienvenu] → [gloda key]
(Assignee)

Comment 9

8 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Keywords: fixed-seamonkey2.0.1
Summary: Post commit error → Post commit error - indexing needs to track folder rebuilds
You need to log in before you can comment on or make changes to this bug.