5.39 KB, text/plain
13.50 KB, patch
|Details | Diff | Splinter Review|
909 bytes, patch
|Details | Diff | Splinter Review|
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.
Does what I wrote in the other bug also helps with this one ?
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.
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.
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.
No message filters, but I do have junk filtering on.
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.
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.
Created attachment 618416 [details] [diff] [review] potential fix I haven't tried this yet, but this is my first pass at a fix.
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.
try server builds with this patch will be here: http://firstname.lastname@example.org
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.
(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.
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.
(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.
(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 on attachment 618791 [details] [diff] [review] fix with unit test [Triage Comment] a=me for landing regression fix on branches.
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
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
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.
Is the printf() in the patch intentional? http://hg.mozilla.org/releases/comm-release/file/fc4c8114809d/mailnews/local/src/nsMovemailService.cpp#l406
Created attachment 619719 [details] [diff] [review] follow on patch to remove printf - checked in
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