748726 - Received messages contain parts of other messages with movemail account

Status: RESOLVED FIXED

(MailNews Core :: Movemail, defect)

Description

[Joshua Root]

Attachment: message.txt

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120420145725

Steps to reproduce:

System: OS X 10.6.8. Account type: movemail.

Click "Get Mail" button.


Actual results:

A newly received message sometimes contains a portion of a different message.


Expected results:

Messages should stay separate.

I'm attaching one of the affected messages, with personal info redacted.

This never happened with previous releases. May be related to bug 748597.

Comment 1

[Ludovic Hirlimann [:Usul]]

Does what I wrote in the other bug also helps with this one ?

Comment 2

[David :Bienvenu]

movemail might be the issue - we have very few users who use that, and it probably didn't get tested in the nightly/alpha/beta periods.

Comment 3

[Joshua Root]

The bisection info in bug 748593 may also be relevant here. It's difficult to reproduce this one on demand. I wouldn't be surprised if the 3 bugs I've filed against TB 12 turned out to be symptoms of the same problem(s), but I don't have any real evidence of that yet so I filed them separately.

Comment 4

[David :Bienvenu]

Do you have any filters, or are these messages simply moved into your inbox from the spool? I looked at the movemail code and it's flushing the output stream correctly, so I'm not sure where things would go awry.

Comment 5

[Joshua Root]

No message filters, but I do have junk filtering on.

Comment 6

[David :Bienvenu]

I take it back - we're only flushing the stream at the end of downloading all the messages...I'll look at this a bit and if I can find a potential fix, I'll put up a try server build for you to try.

Comment 7

[David :Bienvenu]

code level details for anyone interested - the reason this used to work is that nsMsgMailboxParser::HandleLine used to call PublishMsgHdr when it saw an envelope line here - http://mxr.mozilla.org/comm-esr10/source/mailnews/local/src/nsParseMailbox.cpp#501

If m_newMsgHdr is not set, this is a noop, so when we incorporate new pop3 mail, parsing the envelope line of the new message didn't cause any msg hdr to get published, and when parsing a mail folder or movemail spool file, we would publish the msg hdr for the previous message.

With the pluggable store work, I moved that PublishMsgHeader call into a virtual function OnNewMessage(), which nsParseNewMailState made a noop, and nsMsgMailboxParser called PublishMsgHdr from. The reason was that with pluggable stores, m_newMsgHdr gets set *before* we start parsing the message, which, in the new mail case meant we tried to publish the msg header after parsing the envelope line, which is the very first line.

I didn't realize that movemail counted on the old behavior. I'll have to tweak movemail a bit, I think. I'd love to have an xpcshell test for movemail, but it's extremely difficult if not impossible to set env vars in xpcshell tests, which movemail requires to find the spool file. I could tweak the movemail core code to check for a special pref that only the xpcshell test would set for the spool file, and if not set, the movemail code could use the env var stuff it does now.

Comment 8

[David :Bienvenu]

Attachment: potential fix

I haven't tried this yet, but this is my first pass at a fix.

Comment 11

[David :Bienvenu]

Attachment: fix with unit test

my one worry about this is if the test fails, we leave the .lock file in the profile, so the profile needs to be cleaned up before the next run...that happens for free, I assume.

Comment 12

[David :Bienvenu]

try server builds with this patch will be here: http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/[e-mail]-e4cba7544872

Comment 13

[Joshua Root]

Bug 748593 is fixed in this build, and there's no sign of this bug or bug 748597 so far. I'll keep using it and see what happens.

Comment 14

[David :Bienvenu]

(In reply to Joshua Root from comment #13)
> Bug 748593 is fixed in this build, and there's no sign of this bug or bug
> 748597 so far. I'll keep using it and see what happens.

Thx, Joshua, that's very helpful.

Comment 15

[Tony Mechelynck [:tonymec]]

Bienvenu: could you please request a linux64 try-build of comm-central SeaMonkey with this patch, so I could test it with the profile where I noticed bug 713491 (duped to here)?

Comment 16

[David :Bienvenu]

sorry, tony, I have no idea about how to do that. Serge could probably do it, though.

Comment 17

[Tony Mechelynck [:tonymec]]

(In reply to David :Bienvenu from comment #16)
> (In reply to Tony Mechelynck [:tonymec] from comment #15)
> > Bienvenu: could you please request a linux64 try-build of comm-central
> > SeaMonkey with this patch, so I could test it with the profile where I
> > noticed bug 713491 (duped to here)?
>
> sorry, tony, I have no idea about how to do that. Serge could probably do
> it, though.

Well, if it isn't possible I'll wait for an hourly or nightly with the fix and check that the bug is gone from it.

Comment 18

[Serge Gautherie (:sgautherie)]

(In reply to Tony Mechelynck [:tonymec] from comment #17)
> Well, if it isn't possible I'll wait for an hourly or nightly with the fix
> and check that the bug is gone from it.

I am not going to look at (MoMe) Try again until bug 698843 is (fully) fixed :-|

Comment 19

[Mark Banner (:standard8)]

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

[Triage Comment]
a=me for landing regression fix on branches.

Comment 20

[Mark Banner (:standard8)]

Checked in:

https://hg.mozilla.org/comm-central/rev/14888d952b42
https://hg.mozilla.org/releases/comm-aurora/rev/fc567a79a47a
https://hg.mozilla.org/releases/comm-beta/rev/947483070d72
https://hg.mozilla.org/releases/comm-release/rev/fc4c8114809d

Comment 21

[Mark Banner (:standard8)]

Just pushed a minor unit-test only bustage fix for this to beta & release (TB 14 had changed the rules so that QI isn't needed so frequently):

http://hg.mozilla.org/releases/comm-beta/rev/3745a9f5d228
http://hg.mozilla.org/releases/comm-release/rev/b5ed052dcc54

Comment 22

[Tony Mechelynck [:tonymec]]

Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120429 Firefox/15.0a1 SeaMonkey/2.12a1 ID:20120429003037

Bug 713491 (duped to here) has disappeared. I'll go VERIFY the dupe, leaving it to Thunderbird QA to decide if more verification is necessary on this bug.

Comment 23

[kitchin]

Is the printf() in the patch intentional?
http://hg.mozilla.org/releases/comm-release/file/fc4c8114809d/mailnews/local/src/nsMovemailService.cpp#l406

Comment 25

[David :Bienvenu]

Attachment: follow on patch to remove printf - checked in

Comment 26

[David :Bienvenu]

Comment on attachment 619719 [details] [diff] [review]
follow on patch to remove printf - checked in

printf removal landed on trunk - http://hg.mozilla.org/comm-central/rev/d3a9bb8b85f9