Hitting Stop Button while downloading messages doesn't send QUIT to server

NEW
Unassigned

Status

16 years ago
3 years ago

People

(Reporter: ch.ey, Unassigned)

Tracking

(Blocks: 3 bugs)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patchlove])

Attachments

(1 obsolete attachment)

(Reporter)

Description

16 years ago
Currently when hitting stop button nsPop3Protocol::Cancel is reached, calls some
other functions, but none sends the QUIT command to the server.

This causes two problems:
1. Some servers keep the season alive until timeout, so no login possible for a
few minutes.
2. According to RFC 1939 only when the client has issued the QUIT command, the
session enters the UPDATE state. Only in this state mails marked for deletion
going to be deleted on the server.

Comment 1

16 years ago
Dup of/related to bug 138632?
(Reporter)

Comment 2

16 years ago
No, unfortunately not. This bug is about stop while downloading mails.
But 138632 is about stop while logging in/connecting.
(Reporter)

Comment 3

16 years ago
Created attachment 124078 [details] [diff] [review]
proposed patch

Either that's a feature (I'd say bad design then) to really abort and have the
post-state the pre-state, or a real bug.

If the latter, here's a patch.
It's only this one line because we can't set next_state = POP3_SEND_QUIT and
wait what happens since after Abort() the connection is lost, no more able to
send any byte.
(Reporter)

Updated

15 years ago
Attachment #124078 - Flags: review?(bienvenu)

Comment 4

15 years ago
This looks potentially OK - You've tested it and it works in the sense that the
quit gets on the wire, as it were? I ask because I think SendData is
asynchronous, and we abort the connection immediately. What happens when we hit
cancel for a connection that never gets established, i.e., trying to connect to
a pop3 server that's down or not a valid host name?

Also, we're not going to try to commit our state in this case, I think - is that
wrong? We're not going through nsPop3Protocol::ProcessProtocolState() in this
case, are we? I think we want to be, if possible...
(Reporter)

Comment 5

15 years ago
Yes, the QUIT gets on the wire (verified with ethereal) and is recognized by the
server.
Hitting Stop for a connection that never gets established doesn't work at all
(see bug 138632).


And right, we're not going through ProcessProtocolState(). I only wanted to
change as little as possible - to be honest, I don't understand what
nsPop3Protocol::Abort() does.

I now tried
NS_IMETHODIMP nsPop3Protocol::Cancel(nsresult status)  // handle stop button
{
  m_pop3ConData->next_state = POP3_WAIT_FOR_RESPONSE;
  m_pop3ConData->next_state_after_response = POP3_SEND_QUIT;
}

and so going over POP3_SEND_QUIT, POP3_QUIT_RESPONSE and POP3_DONE.
It seems to work too (and same data on the wire according to ethereal).

But we'll have a problem if the server doesn't respond to our QUIT for what
reason soever -> we won't close the connection. And that would be far worse than
the current behaviour.

Comment 6

15 years ago
it looks to me like the cancel code wants to get into the POP3_INTERRUPTED state

nsPop3Protocol::Abort makes it so the code that's writing the message into a
mailbox during pop retrieval clean up after an error correctly, e.g., not
leaving the mailbox with a partial message at the end.

We want to do things like having CommitState get called, because that handles
our pop3 uidl state (assuming it's in a correct state after stop is pressed).

I think we need to worry about the cleanup here - there's lots of state just
sitting around. We don't want to lose state, and we don't want to leak memory
when stop is pressed. I'm not sure what the answer is but I think we need to
find out...
(Reporter)

Comment 7

15 years ago
> it looks to me like the cancel code wants to get into the POP3_INTERRUPTED state

Yes, that would be better, it's unused for now. I was inattentive when looking
at the code.

> We want to do things like having CommitState get called, because that handles
> our pop3 uidl state (assuming it's in a correct state after stop is pressed).

Calling POP3_INTERRUPTED what leads to POP3_ERROR_DONE does the same as
nsPop3Protocol::Abort() plus CommitState().

Only the call to AbortMailDelivery() is in an if(m_pop3ConData->msg_closure) in
POP3_ERROR_DONE while it isn't in Abort() - but that looks ok.
Product: MailNews → Core

Comment 8

14 years ago
Comment on attachment 124078 [details] [diff] [review]
proposed patch

cleaning up request queue
Attachment #124078 - Flags: review?(bienvenu) → review-
sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.
Assignee: sspitzer → nobody
Duplicate of this bug: 408724

Comment 11

11 years ago
Still open?  What was the hold-up on David's 2004-11-29 patch?

Comment 12

11 years ago
which version it's going to be included in??
It causes many troubles with various antispam/antivirus programs. After closing connections there's no QUIT and server doesn't delete mails (no UPDATE). So TB is stuck and can't download any mail from server. I think there's need to increase priority of this bug.

Comment 13

10 years ago
David? comment 8 still valid?
QA Contact: esther → networking.pop

Comment 14

10 years ago
Guys, I don't understand the coding problem being discussed here, but FYI the STOP button works fine in old Netscape 3, under identical circumstances (or even simultaneously) and with the same POP3 servers. So it's not like it can't be done. I have other comments over at https://bugzilla.mozilla.org/show_bug.cgi?id=138632

Comment 15

10 years ago
<lonely patch looking for owner>
Blocks: 120795, 138632
Whiteboard: [patchlove]

Updated

10 years ago
Blocks: 246335
(Assignee)

Updated

10 years ago
Product: Core → MailNews Core
Comment on attachment 124078 [details] [diff] [review]
proposed patch

This patch still applies! (but has got a r- with no updates since, so obsoleting)

$ patch --dry-run < ~/Desktop/tbTestPatches/206754.diff 
patching file nsPop3Protocol.cpp
Hunk #1 succeeded at 773 (offset 115 lines).
Attachment #124078 - Attachment is obsolete: true
Christian,

would you consider submitting a patch addressing Bienvenue's comment ?

Comment 18

8 years ago
Before I go open a new bug, would this be the cause of the Stop button not doing anything in  the middle of a large move/copy of messages (over IMAP) from one server/account to another?
Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.
You need to log in before you can comment on or make changes to this bug.