Last Comment Bug 226669 - Thunderbird tries to download nonexisting messages with POP3
: Thunderbird tries to download nonexisting messages with POP3
Status: RESOLVED FIXED
[sg:fix]
: fixed-aviary1.0, fixed1.7.3
Product: MailNews Core
Classification: Components
Component: Networking: POP (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Christian Eyrich
: esther
:
Mentors:
: 230294 (view as bug list)
Depends on:
Blocks: 237131 245066
  Show dependency treegraph
 
Reported: 2003-11-24 12:16 PST by Tim Kosse
Modified: 2009-01-22 10:17 PST (History)
5 users (show)
dveditz: blocking1.7.5+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
example patch (6.19 KB, patch)
2004-03-12 10:55 PST, Christian Eyrich
no flags Details | Diff | Splinter Review
patch v2 (8.53 KB, patch)
2004-03-13 03:03 PST, Christian Eyrich
mscott: review+
mozilla: superreview+
dveditz: approval1.7.5+
Details | Diff | Splinter Review

Description Tim Kosse 2003-11-24 12:16:11 PST
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
Comment 1 Christian Eyrich 2003-11-25 08:04:57 PST
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?
Comment 2 David :Bienvenu 2003-11-25 08:21:26 PST
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.
Comment 3 Christian Eyrich 2003-11-25 08:43:00 PST
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.
Comment 4 Christian Eyrich 2004-03-11 11:34:44 PST
*** Bug 230294 has been marked as a duplicate of this bug. ***
Comment 5 Christian Eyrich 2004-03-11 13:56:19 PST
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.
Comment 6 David :Bienvenu 2004-03-11 14:13:57 PST
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.
Comment 7 Christian Eyrich 2004-03-11 15:28:49 PST
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.
Comment 8 Christian Eyrich 2004-03-12 10:55:42 PST
Created attachment 143729 [details] [diff] [review]
example patch

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.
Comment 9 Christian Eyrich 2004-03-13 03:03:26 PST
Created attachment 143785 [details] [diff] [review]
patch v2

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).
Comment 10 David :Bienvenu 2004-03-13 08:54:53 PST
Comment on attachment 143785 [details] [diff] [review]
patch v2

looks good to me
Comment 11 David :Bienvenu 2004-03-16 07:18:09 PST
Comment on attachment 143785 [details] [diff] [review]
patch v2

requesting mscott review.
Comment 12 David :Bienvenu 2004-03-16 07:24:57 PST
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.
Comment 13 :aceman 2004-03-18 07:15:27 PST
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).
Comment 14 Christian Eyrich 2004-03-20 03:29:49 PST
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.
Comment 15 David :Bienvenu 2004-03-20 08:38:23 PST
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.
Comment 16 Christian Eyrich 2004-03-20 09:36:18 PST
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.
Comment 17 Christian Eyrich 2004-04-01 03:13:21 PST
David, did you evaluate the risk of this patch yet? If ok, we should ask for
approval.
Comment 18 David :Bienvenu 2004-04-16 07:29:30 PDT
fix checked in, thx, Christian!
Comment 19 Christian Eyrich 2004-04-22 09:12:10 PDT
Reopen to change assignee.
Comment 20 Daniel Veditz [:dveditz] 2004-08-17 14:21:21 PDT
Blocking 1.7.3
Comment 21 :aceman 2004-08-21 05:42:56 PDT
Is this in 1.7.0? I have tried it and I see no difference to 1.5. I was
currently hit the bug.
Comment 22 David :Bienvenu 2004-08-21 07:38:53 PDT
I don't think this was checked into 1.7...
Comment 23 Daniel Veditz [:dveditz] 2004-08-21 11:45:02 PDT
This has not landed on the 1.7 branch; it should.
Comment 24 Daniel Veditz [:dveditz] 2004-08-21 11:45:47 PDT
Comment on attachment 143785 [details] [diff] [review]
patch v2

a=dveditz for drivers
Comment 25 Daniel Veditz [:dveditz] 2004-08-27 06:16:58 PDT
Fix checked in to 1.7 and 1.7.2 branches

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