Closed Bug 226669 Opened 21 years ago Closed 21 years ago

Thunderbird tries to download nonexisting messages with POP3

Categories

(MailNews Core :: Networking: POP, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: tim.kosse, Assigned: ch.ey)

References

Details

(Keywords: fixed-aviary1.0, fixed1.7.3, Whiteboard: [sg:fix])

Attachments

(1 file, 1 obsolete file)

User-Agent: Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; .NET CLR 1.1.4322) Build Identifier: Mozilla Tunderbird 0.4a (20031119) I'm using a small pop3 proxy which scans my incoming mail headers and deletes spam directly on the server. However if I try to get my mail using Thunderbird over my proxy, it always fails, I get an "-ERR message is deleted" response from the server. Thunderbird correctly sends LIST and UIDL to the server. My proxy did already delete some messages, so that they aren't included in the response to LIST/UIDL anymore. Example (message with index 2 has been deleted): UIDL +OK mailbox has 2 messages (23612 octets) 1 4dc74a92bfa4d7s8 3 613d71dfdc905ee0 The next thing Thunderbird does is to retrieve the mails with the UIDs it doesn't know yet. The problem is that Thunderbird also sends a RETR 2 command, which fails. I could think of the following reasons for this bug: - Thunderbird does not correctly parse the output of LIST/UIDL - the internal UID list does not get initialized properly (no UID for mail with index 2 received -> 0 or random data as UID) - Thunderbird tries to download messages with 0 UIDs (in case the internal UID list gets zero-initialized) I've tested the Win32 build of Mozilla Tunderbird 0.4a (20031119) Reproducible: Always Steps to Reproduce: I've been redirected to BugZilla, the original report can be found here: http://forums.mozillazine.org/viewtopic.php?p=274792#274792
Oh, that's a problem, yes. We imply that we're the only one doing something with the mails in the current session. Therefore we take line one as message one, line two as message two a.s.o. But that's not true if a proxy deletes messages in the same session. RFC clearly says "Note that messages marked as deleted are not listed." I guess we have to include the message number in the Pop3MsgInfo struct and not take index x for message x. That would be easy, but accessing the infos for a message would be complicated (loop with m_pop3ConData->msg_info[i].msgnum == msg_num instead of msg_info[msg_num]). David, any other idea?
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
If that turns out to be painful, we could just leave (initialized) holes in the msginfo struct and ignore them when we iterate over it. I'd have to look at the code more closely.
Assignee: sspitzer → bienvenu
The problem with these holes is, that with say highest message number 20 we'd need 20 entries in m_pop3ConData->msg_info. But we don't know about the number needed before the complete listing. Currently we use number_of_messages filled by the STAT answer in GetStat() - but "Note that messages marked as deleted are not counted in either total." So we'd either could always allocate kLargeNumberOfMessages entries. Or issue the LIST command to just look at the last message number, allocate it and then issue LIST again to use it like now. Both not satisfying.
*** Bug 230294 has been marked as a duplicate of this bug. ***
Blocks: 237131
David, I think it's better you're doing this. I don't get what you mean with "level of indirection" and have no own idea to fix the problem from my comment #1.
by level of indirection, I just meant what you described in commment 1, but that's probably not a good way of describing it. I'll look and see how hard it will be. Someone to help test this is going to be important, since I don't have access to a server that has this behaviour.
Ah, ok, so you meant the "looping-thing". Well, it should be quite easy - and quite ugly. But as I wrote, I don't know better way. I can test it with my Perl script that I always use - send it to you if you like.
Attached patch example patch (obsolete) (deleted) — Splinter Review
This is the patch like I'd do it. Instead for(i = 0; m_pop3ConData->msg_info[i].msgnum != msg_num && i <= m_pop3ConData->number_of_messages; i++) ; m_pop3ConData->msg_info[i].uidl = PL_strdup(uidl); we could simply do m_pop3ConData->msg_info[m_listpos - 1].uidl = PL_strdup(uidl); Required for this is that we can be sure the order and number in the UIDL and XTND responses is the same as in the LIST response. And I don't if we really can.
Attached patch patch v2 (deleted) — Splinter Review
Ups, in the first patch I forgot the changed header file. Two additional changes are in this patch: 1. If LIST, XTND or UIDL lists fewer entries than STAT reported m_pop3ConData->number_of_messages is limited to the new maximum. 2. m_pop3ConData->msg_info[m_listpos - 1].msgnum == msg_num is tried first and taken if true. If not, the loop searching for the right index to msg_num is entered. I think that's best of both worlds (fast & paranoid).
Attachment #143729 - Attachment is obsolete: true
Comment on attachment 143785 [details] [diff] [review] patch v2 looks good to me
Attachment #143785 - Flags: superreview+
Comment on attachment 143785 [details] [diff] [review] patch v2 requesting mscott review.
Attachment #143785 - Flags: review?(mscott)
re 1.7b and 1.7, I'd like to get it in but I'm not sure how risky it is. It's supposed to fix a couple bugs in our handling of pop3 servers doing the wrong thing - I'm not sure how many other users are experiencing this. I can try running with this patch.
Attachment #143785 - Flags: review?(mscott) → review+
I would test it too, if it would be a matter of replacing some .dll, otherwise I have to wait for 1.7... And the server may get fixed by that time (because it does these things only for several days).
Maybe that's another bug, but since it's about getting the right UIDL and could be changed in the same patch. Our XTND XLST response parser is broken. Someone commented out char *eatMessageIdToken = nsCRT::strtok(newStr, " ", &newStr); in GetXtndXlstMsgid() sometime in the past. So if we get a response 1 Message-ID: <3117E4DC.2699@netscape.com> we'll extract "Message-ID:" as UIDL ... Reactivating the aboe line would fix this problem.
Christian, that line was commented out in the very first version of Mozilla, which means it was commented out in the pre-mozilla source code. Unfortunately, that means I can't tell why it was commented out since that history is lost. I'm not saying that means uncommenting it out is wrong; just that it's been this way for 5 years at least.
Ah, ok. I know that XTND XLST is unlikely to be supported by a server and maybe the feature has never been used. But I think if we offer a command, it should work.
David, did you evaluate the risk of this patch yet? If ok, we should ask for approval.
fix checked in, thx, Christian!
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Reopen to change assignee.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: bienvenu → ch.ey
Status: REOPENED → NEW
Status: NEW → RESOLVED
Closed: 21 years ago21 years ago
Resolution: --- → FIXED
Whiteboard: fixed-aviary1.0
Blocking 1.7.3
Flags: blocking1.7.3+
Whiteboard: fixed-aviary1.0 → [sg:fix] fixed-aviary1.0, needed1.7
Is this in 1.7.0? I have tried it and I see no difference to 1.5. I was currently hit the bug.
I don't think this was checked into 1.7...
This has not landed on the 1.7 branch; it should.
Keywords: fixed-aviary1.0
Whiteboard: [sg:fix] fixed-aviary1.0, needed1.7 → [sg:fix] needed1.7
Comment on attachment 143785 [details] [diff] [review] patch v2 a=dveditz for drivers
Attachment #143785 - Flags: approval1.7.3+
Blocks: 245066
Fix checked in to 1.7 and 1.7.2 branches
Keywords: fixed1.7.3
Whiteboard: [sg:fix] needed1.7 → [sg:fix] fixed1.7.2+
Whiteboard: [sg:fix] fixed1.7.2+ → [sg:fix] fixed1.7.3
Product: MailNews → Core
Keywords: fixed1.7.5fixed1.7.3
Whiteboard: [sg:fix] fixed1.7.3 → [sg:fix]
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: