Closed Bug 533083 Opened 15 years ago Closed 13 years ago

POP biff on startup: if we have no password, we say hello to server and then quit

Categories

(MailNews Core :: Networking: POP, defect)

Product:
MailNews Core
Component:
Networking: POP
Version:
1.9.1 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 3.3a3

People

(Reporter: BenB, Unassigned)

References

(
URL
)

Details

(Whiteboard: [gs])

Attachments

(1 file, 1 obsolete file)

minimal patch
939 bytes, patch
Bienvenu
: review+
Details | Diff | Splinter Review 
Why are we saying hello to the server and when quit when we don't have a password? Very polite, but pointless, and I think both server operators and privacy-sensitive users (who don't store passwords) would prefer that we don't do that.

Reproduction:
1. Create POP account, do not store password,
   or remove password from existing one
2. Enable Account Settings | Server Settings |
   [x] Check for new messages at startup
   [x] Check for new messages every [   1] minutes
3. Start wireshark to listen on port 110
4. Restart Thunderbird

Actual result:
On startup, we connect to the POP server, it sends OK (greets us), we send "AUTH", which most servers don't understand and respond with -ERR, we send "CAPA", get a response, and then we close the TCP connection.

Afterwards, we make no further requests to the server. (It does work, if we have a password.) So, the periodical biff is disabled, but not the one on startup. I don't know whether the periodical request being disabled is because we now realize we can't login, because we tried and noticed we can't, or because there's code checking in advance whether we have a password.

Expected result:
No network connections.
(In reply to comment #0)
> Afterwards, we make no further requests to the server. (It does work, if we
> have a password.) So, the periodical biff is disabled, but not the one on
> startup. I don't know whether the periodical request being disabled is because
> we now realize we can't login, because we tried and noticed we can't, or
> because there's code checking in advance whether we have a password.

If we don't have a password, we won't prompt for periodic biff requests as they aren't given a msgWindow and we don't prompt. iirc the reason we don't pass them a msgWindow is that otherwise we could be prompting the user for a password when they haven't initiated anything. David B knows more.
Mark is right; we don't want to put up modal password prompts when the user is not initiating the check for new mail. And I believe we pre-flight the periodic biff with a check to see if we have a password. The check for new mail on startup is considered as the user initiating the action; it's unlikely that Thunderbird started up all by itself :-)
The lack of msgWindow in biff case prevents the password prompt, yes. This bug is about no network connection, not no prompts, though. I searched for msgWindow and GetPassword() and can't see in the code that we prevent starting the connection / fetch when there's no msgWindow - all that prevents are prompts, and that's correct.

I do see in the code in SendUsername() and SendPassword() that when GetPassword() returns an error, abort or no password, we exit there. That would precisely explain why the initial fetch starts a connection and then ends after CAPA, because SendUsername is the next step. So, there must be another check for the periodical check that prevents that we even *start* the connection.

Either 1) we remember the POP3_PASSWORD_FAILED (or similar) from the first connection and check that in the next biff rounds, or in the POP3_READ_PASSWORD state the MK_POP3_PASSWORD_UNDEFINED, or 2) the periodical biff has a specific check for empty passwords *before* starting a connection.

Either way, both the biff on startup and the periodical one should check that it has a password before even making a connection.
> The check for new mail on startup is considered as the user initiating
> the action; it's unlikely that Thunderbird started up all by itself :-)

Either we consider it a user action and allow prompts, or we don't make a connection. What we currently do is: We make a network connection, then notice mid-flight that we have no password and don't want to prompt, and exit out. It's an absolutely useless connection without purpose or gain and without user notification.
>Actual result:
>  On startup, we connect to the POP server, it sends OK (greets us), we send
>  "AUTH", which most servers don't understand and respond with -ERR, we send
>  "CAPA", get a response, and then we close the TCP connection.
> [...]
> It's an absolutely useless connection without purpose or gain and without user notification.

"AUTH" is a valid command; my server replies: "-ERR Unknown AUTH method".  
"CAPA" gets a response, but on my server it's: "-ERR Invalid command; valid commands: USER APOP AUTH QUIT"

The "AUTH" command is ONLY issued at startup.  Subsequent attempts to check mail (e.g., when opening a mailbox and are prompted for a password) only result in the CAPA.

I DO get prompted for a password.  The actual connection, AUTH, and CAPA _all_ occur while the "Password" prompt dialog is still present.  The TCP connection is not closed until the user hits "Cancel" -- and then it is rudely closed (by sending FIN), instead of gracefully closed (using "QUIT").

> Expected result:
> No network connections.

This should be the expected result, UNTIL the user responds to the request for a password.  If the prompt is cancelled, no network connection should occur.  If a password is entered, THEN the network connection (and a correct sequence of commands) should occur.

I observed these results on TB 3.0b3 (Eudora 8.0b7), and TB 3.1a1pre -- both on MacOS 10.5.8.
I lied.  The problem DOES occur for me, but only if "Automatically download new messages" is NOT checked in "Server Settings".  The bogus network connection occurs regardless of the "Automatically download new messages" setting; that setting only affects whether or not the Password prompt appears (and the timing of the FIN on the network connection -- with no password prompt, the FIN is immediate; with the password prompt, the FIN is not sent 'til the user hits Cancel).  I've attached a POP3 log, tcpdump, and ShredderDebug (3.1a1pre) output to Bug 529364.
(In reply to comment #0)
> On startup, we connect to the POP server, it sends OK (greets us), we send
> "AUTH", which most servers don't understand and respond with -ERR, we send
> "CAPA", get a response, and then we close the TCP connection.

POP3 log attaced to bug 529364 by Michael A. Pasek is for the "get a response"=="-ERR to CAPA" case.
> bug 529364 attachment 416321 [details]
Sorry but I couldn't know "TCP connection close" by the POP3 log.
> POP3 log attaced to bug 529364 by Michael A. Pasek is for the "get a response"=="-ERR to CAPA" case.
>> bug 529364 attachment 416321 [details]

The POP3 log is for.....well, I don't know; you asked for it, so I got it.  I assumed that you had a particular piece of code in mind and wanted to verify that -- at the POP3 level -- that code was doing what you thought it should.

> Sorry but I couldn't know "TCP connection close" by the POP3 log.

Logs are useful, but just like "settings"....you should never believe them; The Truth Is On The Wire (the one connecting two computers).    :-)

The tcpdump (Bug 529364, Attachment 416322 [details]) shows the FIN sequence (for case "a" in Comment #27 in Bug 529364).
(In reply to comment #8)
> > Sorry but I couldn't know "TCP connection close" by the POP3 log.
> Logs are useful, but just like "settings"....you should never believe them; 

I wanted to say "I can't read tcpdump, so please look into tcpdump".

> The tcpdump shows the FIN sequence.

Is it evidence of "tcp connection is closed by server after -ERR to CAPA"?
Or evidence of "Tb closed connection after -ERR to CAPA and log of ERROR: 4013"?
As "then we close the TCP connection"(we==Tb) is stated in comment #0, I think you also talking about "connection close by Tb". But I'd like to know difference from "USER/PASS,-ERR,CAPA,connection close by server,wait for CAPA resoponse" case, Bug 429069 and Bug 533006, well.
> Is it evidence of "tcp connection is closed by server after -ERR to CAPA"?
> Or evidence of "Tb closed connection after -ERR to CAPA and log of ERROR: 4013"?

The connection close was initiated by TBird (10.10.50.8 is my laptop).  I believe the connection close is not because of a response from the server, but due to the reason noted in the second paragraph of Comment #3:
> [...] when GetPassword() returns an error, abort or no password, we exit there. That would
> precisely explain why the initial fetch starts a connection and then ends after
> CAPA, because SendUsername is the next step. 

I don't know what error "4013" is, but it may be a way of logging: "I'm trying to log in but I don't have a password!".  I haven't begun digging into the code enough to be able to find where error "4013" is defined (or used).

> As "then we close the TCP connection"(we==Tb) is stated in comment #0, I think
> you also talking about "connection close by Tb". 

Yes, the connection close was initiated by Tb.

> But I'd like to know difference from "USER/PASS,-ERR,CAPA,connection close by server,
> wait for CAPA response" case, Bug 429069 and Bug 533006, well.

See those Bug Reports for further comment.
After some poking around, I found what 4013 is:
nsLocalStrings.h:#define POP3_PASSWORD_UNDEFINED                               4013
(In reply to comment #2)
> Mark is right; we don't want to put up modal password prompts when the user is
> not initiating the check for new mail. 

Under what circumstances would a check for new mail NOT be user-initiated, either explicitly (Get New Messages) or implicitly (Prefs "Check for new messages at startup", "Check for new messages every xx minutes")  ?  

> And I believe we pre-flight the periodic biff with a check to see if we have a password. 

Any idea where that code is ?  This doesn't work as I'd expect; if "Check for new messages at startup" is NOT checked, but "Check for new messages every xx minutes" IS checked, I'm never prompted for a password -- i.e., it's not checking for new messages every xx minutes -- even though the preferences are set to do so.  It would be expected that there would be no further checking if I had been prompted for a password and "Cancel"led that prompt, but with the settings above, it just never checks.

(In reply to comment #3)
> [...] can't see in the code that we prevent starting the connection / fetch when there's 
> no msgWindow [...] I do see in the code in SendUsername() and SendPassword() that 
> when GetPassword() returns an error, abort or no password, we exit there. 

Unfortunately, as this bug points out, that's too late.

> [...] So, there must be another check for the periodical check that prevents that we even 
> *start* the connection.

But where is that check ?  All the code in nsPop3Protocol.cpp that worries about getting a 
password does so when there's already a connection -- as far as I can tell.  I can't see how
it can get into the "SendUserName" and "SendPassword" code without a connection (and
suspect things would go downhill fast if it did).
 
> Either 1) we remember the POP3_PASSWORD_FAILED (or similar) from the first
> connection and check that in the next biff rounds, or in the POP3_READ_PASSWORD
> state the MK_POP3_PASSWORD_UNDEFINED, or 2) the periodical biff has a specific
> check for empty passwords *before* starting a connection.

See comment above about "Check for new messages at startup" and "Check for new messages
every xx minutes".  

POP3_PASSWORD_FAILED and MK_POP3_PASSWORD_UNDEFINED don't, I think, ever get set with 
that set of preferences; I can't see how it would ever get into the READ_PASSWORD_STATE (it 
looks like this only gets set when authentication attempts have failed), or set the flag 
POP3_PASSWORD_FAILED (which precipitates, I think, going into the READ_PASSWORD_STATE).  

> Either way, both the biff on startup and the periodical one should check that
> it has a password before even making a connection.

Agreed.  And when performing that check (they should be identical), the user should be prompted
for a password UNLESS he/she had cancelled an earlier password prompt (for the startup case, this would never evaluate to true). For the "cancelled" case, the prompt should only occur for exlicit 
mail checks.

I did note something strange in nsPop3Service.cpp.  Depending on (what I believe to be) the 
setting of "Automatically download new messages", the "POP3 URL" could be constructed as:
  pop3://some.server.name:port?check

Where is the URL checked for the existence of the query string, and what's done differently depending on it's presence/absence ?  (I can try to figure out the latter if given the former).

I'd like to play around with this, but trying to check for the existence of a password is not as simple
as I thought it should be.  It doesn't appear that the password is associated with the POP server 
defined in the account settings, but rather with the "sink" -- and I haven't yet figured out where
or when the "sink" gets associated with the server.....
Say "explicitly user-initiated", if that makes it clearer for you. But the distinction is: can it happen when the user is not sitting at the computer? - If so, that's not user-initiated.

Here's the pre-flighting code I mentioned - http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgBiffManager.cpp#323
(In reply to comment #13)
> Say "explicitly user-initiated", if that makes it clearer for you. 

> But the distinction is: can it happen when the user is not sitting at the computer? -
> If so, that's not user-initiated.

Why not ?  I can set up a scheduled task to check for mail -- and forget that I don't have a
password saved for account "X".  Why shouldn't I be prompted for the password (even though
I'm not there) ?  After all, Tbird doesn't have any way to know whether I'm "sitting at the computer"
or not.  Similarly, I could start Tbird and allow a password prompt to come up, then just walk away.
There's effectively no difference, between the two; the prompt is there with no one to answer it.

UI differences aside....

> Here's the pre-flighting code I mentioned -
> http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgBiffManager.cpp#323

Thanks.  That makes it a little bit clearer, although "Password is Required" is a bit of a misnomer, since it doesn't actually check for the presence of the password, only whether the user has actually been authenticated.  However, "GetPasswordwithoutUI" might be useful (I haven't looked into it thoroughly yet).  This info oughta keep me busy for a couple of days.....
> Why shouldn't I be prompted for the password [for the periodical check]

Because it'd prompt you every 3 minutes, disturbing your other work on the computer. I think that part makes sense.

---

bienvenu, are you saying that we *should* ask for the password, if [x] Check for new mail at startup is checked and we don't have a password in the pw mgr? If so, we just need to make sure the nsPop3Protocol code gets passed the proper msgWindow, I think.

If the prompt is *not* wanted, we should put a "pre-flight" - a check for password availability before we open a network connection - in the nsPop3Protocol.cpp code.
FWIW, I think Michael has a point that we don't do what we promise when
[x] Check for new messages at startup
[ ] Check for new messages every [   1] minutes
and no stored password (probably intentionally by user).
So, although I dislike the thought a bit, I think we have to put up a password prompt for the first automatic "at startup" mail fetch, as bienvenu suggested.
(In reply to comment #15)
> > Why shouldn't I be prompted for the password [for the periodical check]
> 
> Because it'd prompt you every 3 minutes, disturbing your other work on the
> computer. I think that part makes sense.

Not if the additional consideration in comment #12 was factored in:
> [...] UNLESS he/she had cancelled an earlier password prompt (for the
> startup case, this would never evaluate to true). For the "cancelled" case, the
> prompt should only occur for explicit mail checks.

In other words, if I cancelled a password prompt, don't prompt me for 'biff', but prompt me if I "Get New Messages".

I thought I'd suggested it here, but it was probably in one of the related bug reports:  There could
be an additional option on the password prompt:  "Cancel and  don't ask me again this session".  This
would allow multi-account users to leave their Preferences settings alone, but still "pick and choose" which servers they want to check for messages while skipping some even for "manual" checks -- handy for "road warriors" who need to access their accounts differently from different locations.
--
> bienvenu, are you saying that we *should* ask for the password, if [x] Check
> for new mail at startup is checked and we don't have a password in the pw mgr?

That's what happens now, IF you have "Automatically download new messages" enabled; otherwise you just get the bogus connection and no password prompt (and, therefore, no dog-barking-at-mailman, aka 'biff').  The only problem is that the password prompt occurs AFTER the network connection has already been established and "AUTH" and "CAPA" have been sent; if the user clicks "Cancel", the unwanted connection has still occurred (and is ended with FIN instead of QUIT).

> If so, we just need to make sure the nsPop3Protocol code gets passed the proper
> msgWindow, I think.

I'd have to go back and pore over the code again, but I think not having a window is -- effectively -- used as a "flag" to trigger/inhibit some actions (like asking for a password or making a connection).  I think the lack of a window (and not having "Automatically download new messages" enabled) is what causes the behaviour noted in Bug Report 529364.  The other problem is that -- from what I can tell -- the current code never asks for the password until there's actually a network connection (in the SEND_USER_NAME state where SendUsername is invoked).

> If the prompt is *not* wanted, we should put a "pre-flight" - a check for
> password availability before we open a network connection - in the
> nsPop3Protocol.cpp code.

Other than possibly for the biff case (ideally, only _after_ a cancelled password prompt), I can't think of a case where the prompt would not be wanted.  The problem with trying to do a "pre-flight" is that the network connection occurs in 'Initialize', and you can never get to SEND_USER_NAME state (to ask for the password) without first establishing the connection.   I've not given up yet, though.....
I've made some changes in nsPop3Protocol (the only protocol I've test with, thus far) that APPEAR to work (for the few cases I've tested) and will prompt for a new password if none exists -- BEFORE attempting to make a network connection.  It's probably not the correct style and all, but I'll either figure that out (or let someone else "prettify" it).  

The problem noted in 529364 still occurs, though, if "Automatically download" is not enabled (even if "Check for new messages at startup" is enabled) --  until you do a manual check.  Maybe I'll take a whack at that after I more completely test (with as many preference/sequence permutations as I can envision) the changes I've already made.
With some additional testing, I think I've got the desired behaviour for both this and 529364 (assuming that the initial report of the problem occurring WITH "Automatically download new messages) is incorrect.  Change summary:
  nsPop3IncomingServer.cpp 
    - pass "aMsgWindow" to "CheckForNewMail" in "PerformBiff"
  nsPop3Protocol.cpp
    - check to see if a password is needed in "Initialize"
    - if password is needed, get "mLocalBundle" (see NOTE1, below), then invoke "GetPassword" to 
      get the password (see NOTE2, below)
    - Only get "mLocalBundle" at end of "Initialize" if one wasn't acquired to get the password 
      (see NOTE1, below)
    - Add a check for the presence of "m_nsIPop3Sink" in "GetPassword"

I've tested this with variations of settings for "Check for new messages at startup", "Automatically download new messages", and "Check for messages every xx minutes".  In all cases, the bogus network connection is eliminated; no connection will be made without having a password.  

If "Check for new messages at startup" is enabled, the password prompt occurs at startup. If a valid password is entered, the check is made (if "Automatically download" is set, any downloads occur), and periodic 'biff' occurs thereafter (if "Check for new messages every xx minutes" is set).  If the password prompt is cancelled, or "OK" is clicked without entering anything, no attempted network connection is done, and no periodic 'biff' occurs thereafter.  The periodic 'biff' can be started by manually checking for messages (Command-T) and entering a valid password.

If "Check for new messages at startup" is not enabled, no prompt occurs and no periodic 'biff' occurs.  The periodic 'biff' can be started by manually checking for messages (as above).

NOTE1: I don't know what the "mLocalBundle" -- that the existing code gets at the end of "Initialize" -- is used for.  I just know that "GetPassword" needed one, so I copied the code from the end of "Initialize" to get one, then added an 'if' statement to SKIP getting another one (at the end of "Initialize") if one had already been acquired.  I don't know whether the proper thing to do would have been to dispose of the one acquired for the password and then just get another one at the end of "Initialize", or whether it's OK to "recycle" the "mLocalBundle" (for whatever it gets used for by the invoker of "Initialize".

NOTE2: I made up an error code to be returned to the invoker if the user is prompted for a password and no password is entered ("Cancel", or "OK" with no data).  I didn't call "Error", because:
  a) that appears to need some data structure I didn't have (EXC_BAD_ACCESS in Xcode 
     debugger) -- and I couldn't figure out what data structure it needed, even when 
     "stepping into" the function.
  b) The error that would need to be returned (POP3_PASSWORD_UNDEFINED) is NOT a failure 
     (bit 31 is not set).  I used NS_ERROR_GENERATE_FAILURE, but the closest thing I could find 
     to an "appropriate" module ID was "NS_ERROR_MODULE_MAILNEWS".

I'll attach the 'hg diff' output here; pointers, suggestions, comments -- and MILD criticisms  ;-)  -- will be appreciated.  Other testers/verifiers appreciated (or suggestions for test conditions that might exercuse boundary cases).

Lastly, for the case where "Check for new messages at startup" is NOT enabled, but "Check for new messages every xx minutes" IS enabled, I was afraid I'd have to deal with Ben's concern from comment #15:
> Because it'd prompt you every 3 minutes, disturbing your other work on the
> computer. I think that part makes sense.

Fortunately, because of the "pre-flight" check done by the periodic 'biff' code (David's comment #13), the changes did NOT cause a prompt every "xx" minutes if no check was done at startup or no valid password was entered for the startup check (or "Check for new messages at startup" was NOT enabled).
I dunno whether any "flags" should have been set when submitting this attachment.
'hg diff' output for changes made to nsPop3IncomingServer.cpp, nsPop3Protocol.cpp.
(In reply to comment #20)
> Created an attachment (id=417378) [details]
> Possible modifications to correct this bug
> 
> I dunno whether any "flags" should have been set when submitting this
> attachment.
> 'hg diff' output for changes made to nsPop3IncomingServer.cpp,
> nsPop3Protocol.cpp.

Michael you need to request reviews if you want your fix to get into the tree see https://developer.mozilla.org/en/comm-central#Requirements.
Assignee: nobody → mike001
(In reply to comment #21)
> Michael you need to request reviews if you want your fix to get into the tree
> see https://developer.mozilla.org/en/comm-central#Requirements.

I'm sure I don't want my fix -- as is -- to get into the tree, although I would like a review because I'm still trying to get a handle on the overall code structure/design, and understand exactly how the code works and how the test process works.  There are portions of the possible modifications that need modification and/or clarification by someone who understands the code better than I do (see NOTEs in comment #19).

I've look over the "Requirements" page (and sub-pages) at the URL to which comment #21 referred; although there's lots of information about NEEDING reviews/unit tests/etc., I couldn't find any information about HOW to request a review (or super-review).  I finally found that information here:
   https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch#Getting_Reviews

I'm also not so sure I should be the "Assignee" (should I change the status from "NEW" to "ASSIGNED" ?), but I'll take questions to (and, hopefully, get answers from) the dev-apps-thunderbird list.
Comment on attachment 417378 [details] [diff] [review]
Possible modifications to correct this bug

Initial -- need comments/finalization
Attachment #417378 - Flags: review?(bienvenu)
Attachment #417378 - Attachment is patch: true
NOTE: changes (attachment 417378 [details] [diff] [review]) were done to a clone repository captured 2009/12/05; hg identity:
  ae769efac6ed+ tip

Didn't "update" nightly because I wanted a "known quantity" for test purposes (and because I knew the changes would not be applied in their current form anyway).
Woah.....I am not worthy.

Took a look at Bug 511832 (don't ask why), and followed a twisted trail from there.  I think my naive attempt to fix this problem can be deferred to BenB; he can just add a couple of lines to the MOUNTAIN of work he's done for Bug 525238.
> Woah.....I am not worthy.
> MOUNTAIN of work

heh! Don't compare that. Many of my fixes are small, too.

Indeed, I found and filed this bug as part of bug 525238, but I intentionally filed it as separate bug, because it can be nicely separated in code and logically. And your pointer in comment 6 to "[x] Download automatically" was very useful. I'll integrate any small fix that's done here into my patch. I also think you're on the right track.

Starting in a new project, or starting to contribute at all, can be totally intimidating at first. Just keep going, you'll figure it out at some point. bienvenu doesn't respond at the moment, because he's swamped with the 3.0.0 backslash and serious bugs that need to get get out as 3.0.1, but he'll respond later (or if not, ping him nicely). Or I'll help you getting it into the right shape, once I have time after my work.
(In reply to comment #26)
> [...] 
> Indeed, I found and filed this bug as part of bug 525238, but I intentionally
> filed it as separate bug, because it can be nicely separated in code and
> logically. 

Well, maybe.  The changes I made do one thing VERY differently than the existing code, and that is that they do the request for password outside the SendAuth code (in the "state machine") -- and therefore, before any connection is made.  Those changes would have to accomodate a non-password based authentication mechanism, like NTLM/GSSAPI; however, I think they'd work if "GetPasswordPromptRequired" returned a "false" indication (i.e., "no prompt required").


> And your pointer in comment 6 to "[x] Download automatically" was
> very useful. I'll integrate any small fix that's done here into my patch. I
> also think you're on the right track.

Actually, I got that pointer from  comment #10 in Bug 529364.

> ]...]
> bienvenu doesn't respond at the moment, because he's swamped with the 3.0.0
> backslash and serious bugs that need to get get out as 3.0.1,  [...]

I figured he'd be a busy person...

> Or I'll help you getting it into the right shape, once I have time after my work.

I think I should wait until the changes for 525238 go in, since this patch hits some of the same code.
> the request for password ... before any connection is made.
> Those changes would have to accomodate a non-password based
> authentication mechanism, like NTLM/GSSAPI

Ah, I see. Yes, that'd be a good reason to hold this bug on bug 525238, because that would allow you to know whether Kerberos is intended or not and therefore a whether a password required or not. (I do all this work for the same reason: I *actually* want to fix bug 524698.)

> I think I should wait until the changes for 525238 go in

OK.
Comment on attachment 417378 [details] [diff] [review]
Possible modifications to correct this bug

thx for working on this.

-    rv = pop3Service->CheckForNewMail(nsnull, urlListener, inbox, this, nsnull);
-    // it's important to pass in null for the msg window if we are performing biff
-        // this makes sure that we don't show any kind of UI during biff.
+    rv = pop3Service->CheckForNewMail(aMsgWindow, urlListener, inbox, this, nsnull);

This means that biff will put up alerts if we can't connect to the server. We don't want to do that.

I'm not crazy about prompting for the password before we know if we can connect to the server or not, but we could live with that if we had to.

I'll try to look at this a little bit more later...
(In reply to comment #29)
> -    rv = pop3Service->CheckForNewMail(nsnull, urlListener, inbox, this,
> nsnull);
> -    // it's important to pass in null for the msg window if we are performing
> biff
> -        // this makes sure that we don't show any kind of UI during biff.
> +    rv = pop3Service->CheckForNewMail(aMsgWindow, urlListener, inbox, this,
> nsnull);
> 
> This means that biff will put up alerts if we can't connect to the server. We
> don't want to do that.

Actually, I think we do (at least, I would expect it to).  If I haven't saved a password, I expect to be prompted for a password when I want to initiate a connection, and I expect to be told about errors if that connection cannot be made (or if my password is rejected, etc.).  

The only difference between the two cases (passing a message window or null) is whether "Automatically download new messages" is checked/not-checked; that shouldn't have any effect on whether there's UI interaction.

> I'm not crazy about prompting for the password before we know if we can connect
> to the server or not, but we could live with that if we had to.

It's probably preferable to prompt for the password before attempting to connect to the server; better to have everything you need before starting out on a journey, eh ?  Besides, that's the reason for this bug -- we make a connection and then cannot complete it properly.

> I'll try to look at this a little bit more later...

Not to worry; see comment #'s 27 & 28.
> > This means that biff will put up alerts if we can't connect to the server. We
> > don't want to do that.
>
> Actually, I think we do

There are situations when you configure a mail server that's not always reachable. Typical example is a corporate mail account. May also happen for some ISPs whose mail servers are only reachable inside the ISP network. This and other situations are common. Esp. if you have several accounts, some (maybe several) of which are not always reachable. Having an alert come up every time for every account would be *extremely* annoying. (Note that it would come up even if you have your password saved, but even if not, no reason to punish those users who choose not to.)

So, I agree with bienvenu: On startup, we could maybe ask for the password, but must not alert for errors.

> I'm not crazy about prompting for the password before we know if
> we can connect to the server or not, but we could live with that
> if we had to.

Good point. I had not considered the case that we might ask for a password for a server that's not reachable.

> Besides, that's the reason for this bug -- we make a
> connection and then cannot complete it properly.

Well, the bug does not say we should ask for the password. It merely says the connection is useless.
In fact, when I filed the bug, I expected the connection simply not to be made at all and wait for manual mail check. Given the case of non-reachable servers, I am tending towards that more, but it's not my call.
(In reply to comment #31)
> [...configuration scenarios...] Having an alert come up every time
> for every account would be *extremely* annoying. (Note that it would come up
> even if you have your password saved, but even if not, no reason to punish
> those users who choose not to.)

Why would the same alert that occurs for a password-saving user be "punishment" for a user that does not save passwords ?  Currently, if "Check for new messages at startup" is enabled AND "Automatically download new messages" is enabled, you'll get an alert if the connection fails.

> So, I agree with bienvenu: On startup, we could maybe ask for the password, but
> must not alert for errors.

Either we don't ask for the password (and, therefore, don't attempt a connection), OR we ask for a password, attempt the connection, and let the user know if it failed (and the reason for the failure).  

The former case currently occurs if "Automatically download new messages" is NOT enabled (but we do make the bogus connection); this is Bug 529364.  The latter case currently occurs if "Automatically download new messages" IS enabled (the bogus connection will still occur if the user cancels the password prompt).  As I noted in comment #31, the setting of "Automatically download new messages" should have no effect on UI interaction (from my perspective as a user). If I disable the "filter" that lets a connection through my firewall, the latter case (with a valid password entered at the prompt) will result in the alert:
  Alert
  Connection to server chindi.michael-pasek.net timed out.

> > I'm not crazy about prompting for the password before we know if
> > we can connect to the server or not, but we could live with that
> > if we had to.
> 
> Good point. I had not considered the case that we might ask for a password for
> a server that's not reachable.

The server may be unreachable, your password might have expired, the server may have
"internal problems", etc. -- none of that can be known before the connection is attempted.
I don't know how it would be possible to prevent a useless connection (this bug) without 
having all the necessary pieces (e.g., password) in place before initiating the connection.

> Well, the bug does not say we should ask for the password. It merely says the
> connection is useless.

The reason it's useless is because there's no password -- Catch-22.
I just found Bug 123440.  If, after 8 years, a suitable way can't be found to resolve the dialog/alert dilemma, there's no way to fix this that won't result in one of those annoying (but sadly, necessary) dialog boxes and/or alerts.  Assigning this back to "nobody".
Assignee: mike001 → nobody
Michael - while there's disagreement on popping up a message on errors, the other half of your patch is still needed and, it seems, not disagreed upon.

In TB2, opening TB would prompt me for my POP passwords if I had "check for new mail on startup" selected.

In TB3, opening TB doesn't. Which means I must choose 'get mail' for each inbox I have (3), which is clearly silly.

So even if you don't prompt for connection errors, can we at least return to the TB2 behavior of prompting for passwords on startup?
Hm... 3.0.5 is out and this still hasn't been fixed. That's why I ended up here. I strongly support the argument given by Phil Dibowitz in the previous comment (comment #34). I find the discussion on this bug is pretty academic. Fact is, this worked correctly in TB2, and it does not work at all in TB3, so why not simply fix this so it works like it did before? I think personal considerations like "I'm not crazy about prompting for the password before we know if we can connect to the server" and others are not relevant. It's not even a question of reconsidering a previous functionality that has been dropped (or accidentally destroyed) for some reason. It simply does not make sense to have an option in TB3 "check for mail at startup", if we are "saying hello to the server and when quit when we don't have a password" as stated initially  (Description). If I want to check for mail at startup (and I'm sure there are many users out there who want this functionality, without saving their passwords), then I need to ask the user for his password to check for mail. And if the connection with the server is dropped when the password is not available, I obviously have to prompt for the password before that happens. Otherwise, the condition "check for e-mail" is not accomplished. Pretty obvious. Come on guys, don't get entangled in useless discussions. Just please fix this bug (put the code back to what it was before). See also bug 529364.
The "proposed fix" attached here would have fixed this bug and Bug 529364 (with some "cleanup" for comments and some code I didn't understand -- but does work).  With the changes, TB _will_ prompt for a password before making a connection, and _will_ notify of errors if any occur during that connection; those results appear to be at odds with the desires of the powers that be.  Also, since others were about to commit changes that affected the same area of code (see Comment 25 & Comment 26), it was my hope that those doing the other changes would take a look at what I'd done and incorporate the proposed changes for this bug into the changes for Bug 525238 -- that didn't happen.  Since fixing Bug 525238 involved fixes for an authentication method that does NOT have passwords, the changes proposed for fixing this bug would have to take a "password-less" auth method into account.
URL: http://gsfn.us/t/19prn
Whiteboard: [gs]
Attached patch minimal patch — — Splinter Review 
This minimal fix should fix it - the biff manager always passes in nsnull for the msgWindow, so it's ok to pass along the msg window to the pop3 code.

It's unclear to me what all the rest of the changes in your patch were for, but I'm happy to land this minimal change.
Attachment #417378 - Attachment is obsolete: true
Attachment #417378 - Flags: review?(bienvenu)
I don't think this will apply correctly after the changes for 525238.  At any rate, the changes don't accommodate a "password-less" authentication method (e.g., Kerberos).
(In reply to comment #38)
> I don't think this will apply correctly after the changes for 525238.  At any
> rate, the changes don't accommodate a "password-less" authentication method
> (e.g., Kerberos).

I'm sorry, did you mean that your changes would handle a password-less authentication method, if they applied correctly?
(In reply to comment #39)
> I'm sorry, did you mean that your changes would handle a password-less
> authentication method, if they applied correctly?

Ooops.  No, I meant that my changes wouldn't have handled a password-less auth method (I didn't notice that you'd attached a new patch 'til after I'd made my comment).
Comment on attachment 484787 [details] [diff] [review]
minimal patch

I've extracted just one line of Michael's patch - I'll r+ it and land it...
Attachment #484787 - Flags: review+
basic issue fixed on trunk - http://hg.mozilla.org/comm-central/rev/33fc203adc4f
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: