662792 - Thunderbird (too) aggressively marks messages as read after display

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Jonathan Protzenko [:protz]]

The idea is that libmime calls msgWindow.msgHeaderSink.onEndHeaders as soon as it's done displaying the message. What happens next is onEndHeaders calls msgLoaded.

msgLoaded does:

2923   if (msgHdr && !msgHdr.isRead && markReadAutoMode)

and then goes on marking the message as read. What I do with Conversations is if a conversation is currently displayed, and new messages arrive in that conversation, I append them at the bottom, expand them, but do not mark them as read, so that the user can figure out there are new messages pending in a conversation.

This breaks us because in some cases, the message is marked as read when displayed. This doesn't happen all the time, because most of the time 

2908   var msgHdr = gMessageDisplay.displayedMessage;
2909   if (!folder || !msgHdr)
2910     return;

returns earlier, gMessageDisplay.displayedMessage being null.

I need to investigate this more, but I'm considering doing 

2923   if (msgHdr && !msgHdr.isRead && markReadAutoMode && gMessageDisplay.singleMessageDisplay)

just to be on the safe side.

Comment 1

[David :Bienvenu]

OK, this happens when someone streams the message with header=filter&emitter=js&fetchCompleteMessage=false, specifically, the last option, which causes us to use the body structure command, but it turns out that the body structure is such that we should fetch the complete message (e.g., no external parts). But when we fall back to fetching the whole message, we've forgotten the fact that we should be using messagerfc822peek instead of messagerfc822. This should be easy enough to fix.

I'm a little surprised that we're trying to use body structure on such a small message (3K in the log you sent me). I'm not sure if that's because the code that checks the message size before trying bodystructure is getting bypassed with fetchcompletemessage=false. In general, it's not worth doing a body structure for smaller messages because it causes a bit of extra server overhead and roundrips, and we won't be avoiding downloading large parts, since the message is small.

Comment 2

[Jonathan Protzenko [:protz]]

So when I added the fetchCompleteMessage=false option, I figured out this would make things generally faster, I wasn't aware of the roundtrip issue. Are you suggesting that I should check for the size of the message before setting FetchPartsOnDemand in StreamMessage, and only ask for a BODYSTRUCTURE if the size makes it worth the extra roundtrip? I think the code in DisplayMessage does that already, maybe I should just copy the logic in there...

Comment 3

[David :Bienvenu]

yeah something like this code:

      imapUrl->SetFetchPartsOnDemand(
        useMimePartsOnDemand && messageSize >= (PRUint32) gMIMEOnDemandThreshold);

or maybe display message and stream message could share this somehow. If you want me to deal with that as the same time I fix this issue, I'm happy to.

Comment 4

[Jonathan Protzenko [:protz]]

I'd love it if you could fix both at the same it :-). Thanks!

Comment 5

[David :Bienvenu]

I do have a patch that fixes this, but doing the unit test is taking some time...

Comment 6

[David :Bienvenu]

Attachment: proposed fix with unit test

This patch does 3 things:

1. Don't do body structure fetch from StreamMessage if the message is less than the mime threshold.

2. When falling back from body structure to a full message fetch, make sure we honor the imap action (i.e., peek if appropriate)

3. Add a test to test_partsOnDemand to make sure that 2) is fixed.

Protz, asking you for review since I figure you're in the best position to test this fix in the real world.

Comment 7

[David :Bienvenu]

I believe this bug also affects users who choose the mark read delay, or turn off automatic marking read completely, since the fallback to full message fetch happens for both display and stream message. So there's probably several other bugs that can be marked fixed when this is checked in. The STR would simply be to

1. Turn off offline storage for your imap inbox.
2. Send yourself a message whose size is > the the mime parts threshold (25 or 30K, iirc) and which has no external parts. 
3. click on the message.

It should get marked read on the server, and eventually thunderbird should notice that (depending on whether the server tells us directly or not). Fakeserver does not, which made writing the xpcshell test harder, and I think is a bug in fakeserver; I'll check the RFC.

I'm surprised that people with this issue didn't notice that it only happened on larger messages but I strongly suspect this is the same issue. Autosync definitely hides the issue, however, since, once the message is in the offline store, you can't run into this bug. I'll check that I can reproduce it, and search for any other related bugs.

Comment 8

[Wayne Mery (:wsmwk)]

(In reply to comment #7)
> I believe this bug also affects users 

yes, I should think so. A starting point is bug 614232

Comment 10

[David :Bienvenu]

Because of memory cache issues, you may need to add step 4 and 5 for the STR:

4. shutdown and restart
5. Click on the message again

before you see the message get marked read.

Comment 13

[Jonathan Protzenko [:protz]]

I've been running with the patch for a few days and I haven't experienced the issue. Looks good so far! I'll try to use the reproductible procedure given by :bienvenu to confirm that it works for me, and then I'll review the patch.

Comment 14

[David :Bienvenu]

(In reply to comment #13)
> I've been running with the patch for a few days and I haven't experienced
> the issue. Looks good so far! I'll try to use the reproductible procedure
> given by :bienvenu to confirm that it works for me, and then I'll review the
> patch.

great, pinging for review, then.

Comment 15

[Jonathan Protzenko [:protz]]

Attachment: IMAP Protocol log

While testing this, I managed to still have a message marked as read against my will. I have:
- "automatically mark messages read" *unchecked*
- browser.cache.disk.enable set to false,
- browser.cache.memory.enable set to false.

I do have :bienvenu's patch applied, running make -C mailnews/imap && make -C mailnews/ && make -C mail/ && make -C mozilla/toolkit/library produced nothing.

Interestingly enough, the message ends up being marked as read only the second time it's displayed.

Comment 16

[David :Bienvenu]

There's no fetching of the message in that log that would mark it read, so it's definitely a different issue.

Comment 17

[Jonathan Protzenko [:protz]]

Comment on attachment 541436 [details] [diff] [review]
proposed fix with unit test

I've been trying to follow the STR but so far I've been unable to see this happen. I've tested on both Miramar and an unpatched Shredder, and I didn't manage to reproduce (imap servers: gmail.com and free.fr). Nonetheless, as I said, I've been running this patch for some time, and the issue of silently marking messages as read didn't pop up, and I can confirm that the test actually exercises the patch, so this makes me confident enough to give r+.

I'd love to see this on 6.0, hence the approval?-flag.

Comment 18

[David :Bienvenu]

fixed on trunk - http://hg.mozilla.org/comm-central/rev/0bde656fe3ae

yeah, this can be hard to reproduce, but the unit test sets up the failure scenario and reproduces the failure. Besides making the Conversations add-on happier, this should also get rid of one of the main causes of messages getting marked read behind the user's back, and I hope make any other remaining bugs stand out.

Comment 19

[Mark Banner (:standard8)]

Checked into aurora: http://hg.mozilla.org/releases/comm-aurora/rev/ca1276827413