501392 - Duplicates messages (or missing messages) in IMAP

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Description

[Kent James (:rkent)]

This is a followup to bug 414723, which repaired the damage but did not get at the root cause.

While testing my patches for bug 67241, I ended up in a condition where the symptoms of bug 414723 were occurring all of the time. I was using online IMAP for this test. I traced out the cause as follows.

When threads are created, they are initially created as a table with an id of -1. In some cases, the next time a new thread is created and a new table created, the table with id -1 from a previous thread already exists, with the previous message in it. So when the new thread adds its own messages, the previous messages are also in that thread incorrectly.

I can fix the problem by getting the new thread table before I try to create a new one, and if I find the table removing all existing rows.

I'll try to attach a patch, STR, and unit test soon.

Comment 1

[Kent James (:rkent)]

These are my STR. I am running a June 24-based TB3/mozilla-1.9.1 debug build on Windows Server 2003. I don't know which steps are important and which are not.

I setup with a single IMAP account, with disabled "Keep messages for this account on this computer". I start by deleting all but one message, then compact the folder, and restart, and open the INBOX so that a single message appears.

I send a new message to the INBOX, and wait until it appears (and it appears normally). I then close and restart TB, being careful to NOT mouse over the thread pane, nor open the new message.

Upon restart and selecting the INBOX folder, two messages appear, each with the subject of the original email. It takes at least two more restarts (or possibly selecting an alternate folder, then returning to the INBOX folder) before the correct two messages appear.

If I examine the mork .msf file after I have added the new message then shut down, I can see two thread tables. One table has the id of the first message, and a member row of the first message. The second table has an id of the second message, and two member rows, including both the first and second message.

For my setup, these STR are approximately 70% reliable in demonstrating the bug.

Comment 2

[Kent James (:rkent)]

Attachment: Detect an existing table, and delete its rows.

Although this patch seems to solve the issues from my STR, I have not tested with it thoroughly. I'm asking for review anyway, but please do not check it in until I have run with it for a few days. I don't use threaded views myself very much though, nor online IMAP. So, bienvenu, there may be side effects of this that I have missed. Feel free to propose an alternate approach to the issue.

Comment 3

[David :Bienvenu]

this looks like the right thing to do, thx!

It's not relevant whether you use threaded views, since we still do threading w/o threaded views, and this bug happens in flat views as well. But it does seem to be specific to online IMAP.

Comment 4

[David :Bienvenu]

Is gloda turned on or off when you do these STR's? So far I haven't been able to reproduce this with those STR's, though I hit a breakpoint on the patched code for a completely different IMAP folder that I hadn't entered at all (my gmail spam folder).

One thing I've noticed with this bug is that it really seems to always be a new thread that causes the problem - since IMAP UID's are monotonically increasing, and the thread id comes from the message key/imap uid, there really shouldn't be an existing thread table. I'm going to try to dig a little deeper into this.

Comment 5

[Kent James (:rkent)]

The STR are done in a small test profile that does not have gloda enabled.

The existing thread table is the one that is created with -1 as the thread id (or something similar that was negative). When I dumped info about the threads, it creates a thread table with that id, and then switches it to the correct one. So that does not depending on the monotonically increasing id.

Comment 6

[David :Bienvenu]

there's special code for threads with key 1 (we give them the key -2 so that the table id won't collide with the all msgs table, iirc). Is that the key you're seeing? 0xfffffffe.

I'm looking through the code, but it looks like for all other keys, we create the table id once, and don't change it, that I can see. But, given that we display the message correctly when it arrives, but not after a restart, implies that something changes - perhaps the cached thread id is different from the one stored in the db.

Comment 7

[Kent James (:rkent)]

I'll have to trace this out to tell you more, which I probably won't be able to do for a day or two.

But I do remember that in the tests that I was doing, which I did repeatedly on the same IMAP folder, the message IDs were continuously increasing. What I remember from the code trace is that new threads were created with some code for NOKEY, then changed to the correct id later.

Comment 8

[David :Bienvenu]

ah, that would make some sense - I think that could make mork auto-assign the table id, which would cause all sorts of fun. Why the threadId would be -1 to start with would be the mystery.

Comment 9

[David :Bienvenu]

Comment on attachment 386042 [details] [diff] [review]
Detect an existing table, and delete its rows.

Thx, I think this will help - it's kicking the can down the road in a sense, since we still don't understand why this is happening, but this will help users so it's worth landing. I've poked at it most of the weekend, and I still haven't been able to reproduce it usefully.

Are you comfortable with this landing now, Kent?

Comment 10

[Kent James (:rkent)]

> Are you comfortable with this landing now, Kent?

Well at first glance, it seems relatively low risk (famous last words!) since it just detects something that should not be happening, and fixes it. Yes it does just apply the fix earlier than your previous patch - but, if this is indeed the root problem, it fixes it before any damage is done, so it could be a permanent fix.

So yes I am comfortable with it - though it would be good to have landed a few days before the final beta3 freeze. Land it if you want.

Comment 11

[David :Bienvenu]

ok, thx, fix checked in.

Comment 12

[David :Bienvenu]

One further observation - undo imap delete makes us hit this code every time. I'm not sure if it's because of the offline delete which creates a pseudo-header with a small negative uid, or if we're not cleaning up the thread on the initial delete.