Last Comment Bug 748726 - Received messages contain parts of other messages with movemail account
: Received messages contain parts of other messages with movemail account
Status: RESOLVED FIXED
: regression
Product: MailNews Core
Classification: Components
Component: Movemail (show other bugs)
: 12
: All All
: -- normal with 1 vote (vote)
: Thunderbird 15.0
Assigned To: David :Bienvenu
:
Mentors:
: 713491 743889 748593 748597 750100 751833 (view as bug list)
Depends on: 787605
Blocks: 402392
  Show dependency treegraph
 
Reported: 2012-04-25 05:07 PDT by Joshua Root
Modified: 2013-04-01 07:14 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed


Attachments
message.txt (5.39 KB, text/plain)
2012-04-25 05:07 PDT, Joshua Root
no flags Details
potential fix (1.06 KB, patch)
2012-04-25 13:44 PDT, David :Bienvenu
no flags Details | Diff | Review
fix with unit test (13.50 KB, patch)
2012-04-26 13:55 PDT, David :Bienvenu
standard8: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
standard8: approval‑comm‑release+
Details | Diff | Review
follow on patch to remove printf - checked in (909 bytes, patch)
2012-04-30 15:33 PDT, David :Bienvenu
standard8: review+
Details | Diff | Review

Description Joshua Root 2012-04-25 05:07:27 PDT
Created attachment 618210 [details]
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] 2012-04-25 06:31:17 PDT
Does what I wrote in the other bug also helps with this one ?
Comment 2 David :Bienvenu 2012-04-25 07:00:48 PDT
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 2012-04-25 07:12:27 PDT
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 2012-04-25 07:45:44 PDT
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 2012-04-25 08:01:12 PDT
No message filters, but I do have junk filtering on.
Comment 6 David :Bienvenu 2012-04-25 08:04:46 PDT
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 2012-04-25 13:31:09 PDT
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 2012-04-25 13:44:56 PDT
Created attachment 618416 [details] [diff] [review]
potential fix

I haven't tried this yet, but this is my first pass at a fix.
Comment 9 Ludovic Hirlimann [:Usul] 2012-04-26 01:59:48 PDT
*** Bug 743889 has been marked as a duplicate of this bug. ***
Comment 10 David :Bienvenu 2012-04-26 07:05:54 PDT
*** Bug 713491 has been marked as a duplicate of this bug. ***
Comment 11 David :Bienvenu 2012-04-26 13:55:18 PDT
Created attachment 618791 [details] [diff] [review]
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 2012-04-26 13:57:10 PDT
try server builds with this patch will be here: http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/bienvenu@nventure.com-e4cba7544872
Comment 13 Joshua Root 2012-04-26 17:42:54 PDT
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 2012-04-26 17:45:51 PDT
(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] 2012-04-26 20:35:11 PDT
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 2012-04-26 21:08:58 PDT
sorry, tony, I have no idea about how to do that. Serge could probably do it, though.
Comment 17 Tony Mechelynck [:tonymec] 2012-04-26 21:50:29 PDT
(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) 2012-04-26 22:41:37 PDT
(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) 2012-04-28 06:58:17 PDT
Comment on attachment 618791 [details] [diff] [review]
fix with unit test

[Triage Comment]
a=me for landing regression fix on branches.
Comment 21 Mark Banner (:standard8) 2012-04-28 10:46:02 PDT
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] 2012-04-29 16:13:49 PDT
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 24 Martijn Dekker 2012-04-30 15:28:29 PDT
*** Bug 750100 has been marked as a duplicate of this bug. ***
Comment 25 David :Bienvenu 2012-04-30 15:33:17 PDT
Created attachment 619719 [details] [diff] [review]
follow on patch to remove printf - checked in
Comment 26 David :Bienvenu 2012-05-01 06:37:05 PDT
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
Comment 27 Mark Banner (:standard8) 2012-05-04 04:34:35 PDT
*** Bug 751833 has been marked as a duplicate of this bug. ***
Comment 28 Joshua Root 2013-04-01 07:06:11 PDT
*** Bug 748593 has been marked as a duplicate of this bug. ***
Comment 29 Joshua Root 2013-04-01 07:14:34 PDT
*** Bug 748597 has been marked as a duplicate of this bug. ***

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