801916 - Permanent orange: TEST-UNEXPECTED-FAIL | test_bug155172.js, test_smtpPassword.js, and others failing post-nsresult changes - SMTP & NNTP passwords prompted rather than obtained from password manager

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Mark Banner (:standard8)]

We're getting a number of tests failing on Windows after the first two patches on bug 801383 have landed.

I think the test failures are indicating that we're attempting to get the SMTP password from the user (via an alert) rather than the password manager. I think this is the case for NNTP as well.

e.g.

TEST-UNEXPECTED-FAIL | c:\talos-slave\test\build\xpcshell\tests\mailnews\compose\test\unit\test_smtpPasswordFailure1.js | test failed (with xpcshell return code: 0), see following log:
>>>>>>>
### XPCOM_MEM_LEAK_LOG defined -- logging leaks to c:\users\cltbld\appdata\local\temp\tmptyfayc\runxpcshelltests_leaks.log

TEST-INFO | (xpcshell/head.js) | test 1 pending
WARNING: NS_ENSURE_TRUE(!(accountList.IsEmpty())) failed: file e:/builds/moz2_slave/tb-try-c-cen-w32-dbg/build/mailnews/base/src/nsMsgAccountManager.cpp, line 1418
Directory request for: MailD that we (mailDirService.js) are not handling, leaving it to another handler.
Directory request for: MFCaF that we (mailDirService.js) are not handling, leaving it to another handler.
WARNING: No valid default account found, just using first (FIXME): file e:/builds/moz2_slave/tb-try-c-cen-w32-dbg/build/mailnews/base/src/nsMsgAccountManager.cpp, line 845
Directory request for: DefRt that we (mailDirService.js) are not handling, leaving it to another handler.
Send
WARNING: NS_ENSURE_TRUE(aCallbacks) failed: file e:/builds/moz2_slave/tb-try-c-cen-w32-dbg/build/mailnews/compose/src/nsSmtpUrl.cpp, line 836
WARNING: NS_ENSURE_TRUE(aUrlListener) failed: file e:/builds/moz2_slave/tb-try-c-cen-w32-dbg/build/mailnews/base/util/nsMsgMailNewsUrl.cpp, line 113
WARNING: NS_ENSURE_TRUE(m_callbacks) failed: file e:/builds/moz2_slave/tb-try-c-cen-w32-dbg/build/mailnews/compose/src/nsSmtpUrl.cpp, line 845
WARNING: This method is lossy. Use GetCanonicalPath !: file e:/builds/moz2_slave/tb-try-c-cen-w32-dbg/build/mozilla/xpcom/io/nsLocalFileWin.cpp, line 3262

TEST-UNEXPECTED-FAIL | ../../../resources/alertTestUtils.js | promptPasswordPS unexpectedly called: Enter your password for testsmtp on localhost:
 - See following stack:
JS frame :: c:\talos-slave\test\build\xpcshell\head.js :: do_throw :: line 451
JS frame :: ../../../resources/alertTestUtils.js :: <TOP_LEVEL> :: line 214
JS frame :: resource://gre/components/nsLoginManagerPrompter.js :: <TOP_LEVEL> :: line 455
native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

https://tbpl.mozilla.org/php/getParsedLog.php?id=16125991&tree=Thunderbird-Trunk

Comment 1

[Mark Banner (:standard8)]

This could be fallout from http://hg.mozilla.org/mozilla-central/rev/5091701dcc45, bug 723004 - it seems to have changed the default to private browsing if no window is supplied, I suspect that could be the case, but really can't confirm until we resolve bug 801916 enough to get a build I can play around with.

Comment 2

[(no longer active)]

Do these tests attempt to save the login information after they prompt for it?

Comment 3

[Mark Banner (:standard8)]

I just confirmed this is a regression from bug 723004. I now need to remember what these tests do.

Comment 4

[Mark Banner (:standard8)]

I now remember what's going on here:

http://hg.mozilla.org/mozilla-central/annotate/cd3270dc35cc/toolkit/components/passwordmgr/nsLoginManagerPrompter.js#l425

In promptPassword, the way wallet used to work, and the way it continues to work now, is that when it is called, it looks through for existing logins. If one is found, it'll return early and save prompting for the actual password in the case it has already been saved.

However, with the change to return true from the _inPrivateBrowsing function when we don't have a window, this breaks the xpcshell tests and our LDAP code (in compose window).

I'm not quite sure how ldap is succeeding for the compose windows case, but the code is here:

http://hg.mozilla.org/comm-central/annotate/eac22e3d7ba3/mailnews/addrbook/src/nsAbLDAPListenerBase.cpp#l150

Comment 5

[(no longer active)]

Hmm, perhaps we can modify that check to be something like:

if (hostname && (!this._window || !this._inPrivateBrowsing))

?  That should fix this test failure and probably doesn't matter for Firefox.  Justin, what do you think?

Comment 6

[Mark Banner (:standard8)]

Attachment: Fix as suggested by Ehsan

This definitely works for our xpcshell tests. Justin, thoughts?

Comment 7

[Justin Dolske [:Dolske]]

I'm not super concerned about this, since these are basically old APIs that we kept^H^H^H^Hreimplemented exclusively for mailnews. :)

But... With this change, anything that calls promptPassword without a window will automatically get a password returned (assuming one already exists) without any user interaction. That would probably be... surprising... in "private browsing" mode. [Also, as a minor point, the checkbox can now default to saving an entered password -- but more trivial to fix.]

So, a few options:

1) Fix the tests and/or code to correctly pass around a window. That should be fun with xpcshell. :/ I assume this _is_ a testing-only issue? And that smtp/nntp/ldap all continue to work as-expected for end users? Or do prompts sometimes break for them too?

2) Add a knudge-pref for these tests (with a suitable scary name) that makes _inPrivateBrowsing (or just promptPassword?) work as the tests expect.

3) Say "screw it", take this patch as-is, and be mindful that this is a potential PB footgun. Auditing existing callers to make sure they're mindful of doing the right thing wrt PB would be a good idea.

4) Not sure if it actually helps the xpcshell case, but we _could_ try making _inPrivateBrowsing more clever for the special-case of "this._window is null, but there's only 1 app window so go check that". The downside is that this is a bit too magical, and means simply having a 2nd window open can change behavior in an unexpected-to-the-user way. So I hesitate to even suggest it. :)

Comment 8

[(no longer active)]

My favorite of these options is (1).

Comment 9

[Mark Banner (:standard8)]

I think I'm happy with 1, and was heading towards it anyway. Let me see if I can get time to look at it tomorrow.

Comment 10

[Mark Banner (:standard8)]

Attachment: SMTP fix

This should fix the SMTP issues. It is based on what the incoming servers do. Still need to make sure we fix LDAP, but I can do that as a follow-up. We might also need to review cases like chat and calendar to check they are ok.

The compose unit tests pass with this change. I'll double-check the UI when I get up in the morning. I'll take the review of whoever gets to it first.

Comment 11

[[e-mail]]

Comment on attachment 672960 [details] [diff] [review]
SMTP fix

>+  if (numLogins > 0)
[For below]

>+    nsCString serverCUsername;
>+    rv = GetUsername(serverCUsername);
>+    NS_ConvertUTF8toUTF16 serverUsername(serverCUsername);
GetUsernamePasswordWithUI seems to think that the username is ASCII. Which is right?

>+        m_password = NS_LossyConvertUTF16toASCII(password);
LossyCopyUTF16toASCII

>+    NS_FREE_XPCOM_ISUPPORTS_POINTER_ARRAY(numLogins, logins);
>+  }
When there are zero logins, do you get a zero-length or a null array?


>   if (!m_password.IsEmpty())
>     return GetPassword(aPassword);
> 
>-  NS_ENSURE_ARG_POINTER(aDialog);
>-
>   nsCString serverUri;
>   nsresult rv = GetServerURI(serverUri);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  // We need to get a password, but see if we can get it from the password
>+  // manager without requiring a prompt.
>+  rv = GetPasswordWithoutUI(NS_ConvertUTF8toUTF16(serverUri));
>+  if (rv == NS_ERROR_ABORT)
>+    return NS_MSG_PASSWORD_PROMPT_CANCELLED;
>+
>+  if (!m_password.IsEmpty())
>+    return GetPassword(aPassword);
>+
>+  NS_ENSURE_ARG_POINTER(aDialog);
So, you moved the aDialogCheck, but duplicated the m_password check? I didn't look at the other servers but I'm guessing you only need the first version.

Comment 12

[Mark Banner (:standard8)]

(In reply to [e-mail] from comment #11)
> >+    nsCString serverCUsername;
> >+    rv = GetUsername(serverCUsername);
> >+    NS_ConvertUTF8toUTF16 serverUsername(serverCUsername);
> GetUsernamePasswordWithUI seems to think that the username is ASCII. Which
> is right?

We'll go for ASCII as that's all the nsISmtpServer interface allows to js.

> >+    NS_FREE_XPCOM_ISUPPORTS_POINTER_ARRAY(numLogins, logins);
> >+  }
> When there are zero logins, do you get a zero-length or a null array?

I suspect this is a null array, although it seems to be hidden in the depths of xpcom.

> >   if (!m_password.IsEmpty())
> >     return GetPassword(aPassword);
> > 
> >-  NS_ENSURE_ARG_POINTER(aDialog);
> >-
> >   nsCString serverUri;
> >   nsresult rv = GetServerURI(serverUri);
> >   NS_ENSURE_SUCCESS(rv, rv);
> > 
> >+  // We need to get a password, but see if we can get it from the password
> >+  // manager without requiring a prompt.
> >+  rv = GetPasswordWithoutUI(NS_ConvertUTF8toUTF16(serverUri));
> >+  if (rv == NS_ERROR_ABORT)
> >+    return NS_MSG_PASSWORD_PROMPT_CANCELLED;
> >+
> >+  if (!m_password.IsEmpty())
> >+    return GetPassword(aPassword);
> >+
> >+  NS_ENSURE_ARG_POINTER(aDialog);
> So, you moved the aDialogCheck, but duplicated the m_password check? I
> didn't look at the other servers but I'm guessing you only need the first
> version.

Yes, we don't want to touch the login manager if we've already got a password, as this could cause a master password prompt. The second check is necessary so that once we've examined the login manager, we don't need to go on to prompt if we've got a password already.

Comment 13

[Mark Banner (:standard8)]

Attachment: SMTP fix v2

This is updated with the comments. I've also re-run the tests and manually check the cases of sending email and send in background and SMTP all works fine.

Comment 14

[[e-mail]]

Comment on attachment 673152 [details] [diff] [review]
SMTP fix v2

Indeed, you appear to get nullptr back.

Comment 15

[Mark Banner (:standard8)]

Checked in:

https://hg.mozilla.org/comm-central/rev/1504248507fa

I'll try and have a look later today and see what else we need to fix.

Comment 16

[Mark Banner (:standard8)]

Attachment: Really fix SMTP

Ok, so in producing the last fix I'd forgotten I still had the previous patch applied to m-c.

Thankfully, all is not lost, I'd forgotten that FindLogins wants the URI without the username, whereas PromptPassword wants it with the username...

This is therefore adding another function so we can choose to have the URI with or without the username, and then using that in the right places.

The tests really do pass this time around.

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 673206 [details] [diff] [review]
Really fix SMTP

Sorry, somehow I keep thinking NeilAway is :neil.

Comment 18

[[e-mail]]

Comment on attachment 673206 [details] [diff] [review]
Really fix SMTP

>+  nsString serverUri(NS_ConvertASCIItoUTF16(GetServerURIInternal(false)));
NS_ConvertASCIItoUTF16 serverUri(GetServerURIInternal(false));

>+nsCString
>+nsSmtpServer::GetServerURIInternal(const bool aIncludeUsername)
>+{
>+  nsAutoCString uri(NS_LITERAL_CSTRING("smtp://"));
nsCString (because of nvro).

Comment 19

[Mark Banner (:standard8)]

https://hg.mozilla.org/comm-central/rev/c539482fbd90

Comment 20

[Mark Banner (:standard8)]

I believe the SMTP parts of this are fixed, the LDAP parts are moved out to bug 833971.