557622 - Hook up IMAP to msgAsyncPrompt service and make its password prompts serial again

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Mark Banner (:standard8)]

Spinning out the separate work from bug 338549. We need to hook up IMAP to the msgAsyncPrompt service so that the prompts will occur in serial fashion.

Attachment 435000 [details] [diff] has a first version for this bug.

Comment 1

[David :Bienvenu]

this patch breaks the retry logic when a bad password is entered - we keep sending the bad password. I suspect when BenB did his auth work, he might have broken this patch...or made the merge hard.

Comment 2

[David :Bienvenu]

Attachment: fix new password entry

this gets the patch working as well as it did before. There's still an issue where if you retry a password, and there was a second password request waiting its turn, you press retry on the first password, but get the second password dialog. To fix this, I think we'd need to make the async password prompter know about retry, so that it doesn't put up the queued request. 

I would think pop3 would have this same issue. I need to refresh my memory about how the async password service works...

Comment 3

[David :Bienvenu]

How can we write meaningful imap tests for this, given that we can only have a single fake imap server? The async prompter code can have its own tests, but testing it with the imap code would seem to require multiple imap servers.

Comment 4

[David :Bienvenu]

I think nsIMsgAsyncPromptListener's onPromptStart needs to return a tri-state value, ok, cancel, enter new password. This would put the prompter code in a mode where it's waiting for a prompt request from the same key, instead of just handling the next prompt request in the queue. Standard8, I'm happy to have a quick try at doing that. I'll put up a patch in a little bit.

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #3)
> How can we write meaningful imap tests for this, given that we can only have a
> single fake imap server? The async prompter code can have its own tests, but
> testing it with the imap code would seem to require multiple imap servers.

The best way I can think would be to have multiple fake servers running in separate processes. This kinda implies mozmill which we haven't got hooked up yet.

Comment 6

[Mark Banner (:standard8)]

(In reply to comment #4)
> I think nsIMsgAsyncPromptListener's onPromptStart needs to return a tri-state
> value, ok, cancel, enter new password. This would put the prompter code in a
> mode where it's waiting for a prompt request from the same key, instead of just
> handling the next prompt request in the queue. Standard8, I'm happy to have a
> quick try at doing that. I'll put up a patch in a little bit.

I'm a bit concerned about that sort of solution, as it isn't really going to be compatible with core code if/when we can move across to that. Although we may be able to get a change in there.

The other question would be what happens if it doesn't get a prompt request for the same key... is it going to just keep stacking them up?

Comment 7

[David :Bienvenu]

(In reply to comment #6)

> I'm a bit concerned about that sort of solution, as it isn't really going to be
> compatible with core code if/when we can move across to that. Although we may
> be able to get a change in there.

If we went to some sort of non-modal password approach, we could in theory have multiple password prompts up at the same time, e.g., stacked sliding alerts. But I don't think the situation the way it is now is acceptable, and I don't see a way of fixing it without changing the interface.

> 
> The other question would be what happens if it doesn't get a prompt request for
> the same key... is it going to just keep stacking them up?

If the listener promises to reprompt, but doesn't, it would effectively block new password prompts. I was planning on adding a comment to the idl to that effect. Maybe the nsIMsgAsyncPromptListener could do the re-prompting itself, by just re-running the current prompt, which would avoid that issue.

Comment 8

[Mark Banner (:standard8)]

Ok, let's go with that for now, and we can sort out core later ;-)

Comment 9

[David :Bienvenu]

Actually, my suggestion won't work - we don't know that the user wants to retry until the password has actually failed, and we've prompted the user, so it can't be a return value from onPrompt, unless we delay the onPrompt callback until after that whole process is done. That would mean the user couldn't get multiple logons going on at the same time, which I think would be bad.

A couple other possibilities - we could have a parameter to queueAsyncAuthPrompt that forces the new retry prompt to run immediately, so that it would pop up on top of any existing password prompt, which isn't perfect, but is less confusing to the user. Or the retry code could simply not use the queueAsyncAuthPrompt at all, but that would allow other password prompts to come up...so I think I'll try adding a param to queueAsyncAuthPrompt.

Comment 10

[David :Bienvenu]

Attachment: proposed fix

this implements my idea for allowing the client code to implement reprompting by jumping the password prompt queue for that case.

Comment 11

[Mark Banner (:standard8)]

I've not looked at the patch yet, but I'm getting errors like:

../../../mozilla/dist/include/nsMsgIncomingServer.h: In member function ‘virtual nsresult nsImapIncomingServer::AsyncGetPassword(nsIImapProtocol*, PRBool, nsACString_internal&)’:
../../../mozilla/dist/include/nsMsgIncomingServer.h:111: error: ‘nsCString nsMsgIncomingServer::m_password’ is private
/Users/moztest/comm/main/src/mailnews/imap/src/nsImapIncomingServer.cpp:2113: error: within this context
../../../mozilla/dist/include/nsMsgIncomingServer.h:111: error: ‘nsCString nsMsgIncomingServer::m_password’ is private
/Users/moztest/comm/main/src/mailnews/imap/src/nsImapIncomingServer.cpp:2129: error: within this context
../../../mozilla/dist/include/nsMsgIncomingServer.h:111: error: ‘nsCString nsMsgIncomingServer::m_password’ is private

when trying to build it.

Comment 12

[David :Bienvenu]

Attachment: include nsMsgIncomingServer.hd in diffs

sorry about that; this should build...

Comment 13

[Mark Banner (:standard8)]

Did you look at xpcshell tests? I'm seeding a failure:

TEST-UNEXPECTED-FAIL | /Users/moztest/comm/main/tb/mozilla/_tests/xpcshell/test_imap/unit/test_imapPasswordFailure.js | test failed (with xpcshell return code: 
...
TEST-UNEXPECTED-FAIL | /Users/moztest/comm/main/tb/mozilla/_tests/xpcshell/test_imap/unit/test_imapPasswordFailure.js | false == true - See following stack:
JS frame :: /Users/moztest/comm/main/src/mozilla/testing/xpcshell/head.js :: do_throw :: line 257
JS frame :: /Users/moztest/comm/main/src/mozilla/testing/xpcshell/head.js :: do_check_eq :: line 287
JS frame :: /Users/moztest/comm/main/src/mozilla/testing/xpcshell/head.js :: do_check_true :: line 299
JS frame :: /Users/moztest/comm/main/tb/mozilla/_tests/xpcshell/test_imap/unit/test_imapPasswordFailure.js :: run_test :: line 153
JS frame :: /Users/moztest/comm/main/src/mozilla/testing/xpcshell/head.js :: _execute_test :: line 151
JS frame :: -e :: <TOP_LEVEL> :: line 1

Comment 14

[Mark Banner (:standard8)]

I can't run the patch and get prompts, I'm getting: 

Error: runnablePrompter:run: TypeError: prompter.first.onPromptStart is not a function

Source File: file:///Users/moztest/comm/main/tb/mozilla/dist/ShredderDebug.app/Contents/MacOS/components/msgAsyncPrompter.js
Line: 61

Did you miss including the msgAsyncPrompter.js changes?

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 438978 [details] [diff] [review]
include nsMsgIncomingServer.hd in diffs

>@@ -110,13 +110,14 @@ interface nsIImapIncomingServer : nsISup

>+  ACString PromptPassword(in nsIMsgWindow aWindow);

nit: needs documentation
 
>+NS_IMETHODIMP
>+nsImapIncomingServer::AsyncGetPassword(nsIImapProtocol *aProtocol,
>+                                       PRBool aNewPasswordRequested,
>+                                       nsACString &aPassword)
>+{
>+  nsresult rv;
>+  if (m_password.IsEmpty())
>+  {

nit: rv can be declared inside the if statement.

>+NS_IMETHODIMP
>+nsImapProtocol::OnPromptAuthAvailable()
>+{
>+  NS_NOTREACHED("Did not expect to get imap protocol queuing up auth "
>+                "connections");
>+  return NS_OK;
>+}

Would it, in theory be possible that a user had two windows and tried access two folders on the same imap server that we could hit this?

My only other concern is that the addition of:

+   * @param aPromptImmediately If the user is retrying a failed password, we
+   *                           need to prompt immediately, even if there is a
+   *                           prompt up.

won't actually work on Mac. I'm not 100% sure, but I think the way modal dialogs there will stop that happening, and therefore Mac would still have the issue of confusion over failed passwords. Generally, I think we'll probably get away with it, but may be worth having a bug open on it so that we know it is an issue.

Cancelling reviews as I think the patch needs update for the missing changes - they probably also will fix the unit test as well.

Comment 16

[David :Bienvenu]

Attachment: include js file in diffs.

this also addresses the nit. Yes, the unit tests have been passing fine for me on windows but have not tried the mac. I can do that in a bit...

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 439237 [details] [diff] [review]
include js file in diffs.

In general this is looking good. I'm not totally sure I like the effect of sticking up the prompt immediately if we fail, but I'm not sure that's just a side effect of these changes as we'd get the retry/enter/cancel dialog coming up in front anyway.

I think we can't do much about that until we drop modal dialogs for these things and this is much better than what we have now.

A few comments:

>+   * @param aPromptImmediately If the user is retrying a failed password, we
>+   *                           need to prompt immediately, even if there is a
>+   *                           prompt up.

I think we should note that this doesn't necessarily happen on Mac, but will happen in front of any other queued prompts.

>+function do_timeout_function(aDelayInMS, aFunc, aFuncThis, aFuncArgs) {

I think this function got pasted in either the wrong place, or just incorrectly got pasted...

>+  ACString PromptPassword(in nsIMsgWindow aWindow);

nit: please add a little bit of documentation for this. Might be worth adding that it may stick up modal prompts.

>+NS_IMETHODIMP
>+nsImapProtocol::OnPromptAuthAvailable()
>+{
>+  NS_NOTREACHED("Did not expect to get imap protocol queuing up auth "
>+                "connections");
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsImapProtocol::OnPromptCanceled()
>+{
>+  // A prompt was cancelled, so notify the imap thread.
>   return NS_OK;
> }

We can reach both of these so we need to implement them. Test case: In separate tabs, open up different folders of the same server. Restart Thunderbird and enter the password once. The assertions are then shown, and the other folders don't finish loading.

Comment 18

[David :Bienvenu]

Attachment: patch addressing comments

This addresses the review comments. I think you can still get into some odd situations, especially with canceling, but overall it's better than what we have now.

Comment 19

[David :Bienvenu]

Attachment: fix cancel handling

This should fix canceling, by propagating the rv from GetPassword over to the imap thread.

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 440082 [details] [diff] [review]
fix cancel handling

Yep, that's much better. Like you said before, there's some strange effects with password cancelling, but I think we can work on those later and take what we've got as the big improvement for 3.1.

Comment 21

[David :Bienvenu]

fixed for 3.1 b2.