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.
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.
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
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.
Do these tests attempt to save the login information after they prompt for it?
I just confirmed this is a regression from bug 723004. I now need to remember what these tests do.
I now remember what's going on here:
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:
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?
Created attachment 672267 [details] [diff] [review]
Fix as suggested by Ehsan
This definitely works for our xpcshell tests. Justin, thoughts?
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. :)
My favorite of these options is (1).
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.
Created attachment 672960 [details] [diff] [review]
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 on attachment 672960 [details] [diff] [review]
>+ if (numLogins > 0)
>+ 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);
>+ 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);
> 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);
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.
(In reply to firstname.lastname@example.org 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
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.
Created attachment 673152 [details] [diff] [review]
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 on attachment 673152 [details] [diff] [review]
SMTP fix v2
Indeed, you appear to get nullptr back.
I'll try and have a look later today and see what else we need to fix.
Created attachment 673206 [details] [diff] [review]
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 on attachment 673206 [details] [diff] [review]
Really fix SMTP
Sorry, somehow I keep thinking NeilAway is :neil.
Comment on attachment 673206 [details] [diff] [review]
Really fix SMTP
>+ nsString serverUri(NS_ConvertASCIItoUTF16(GetServerURIInternal(false)));
>+nsSmtpServer::GetServerURIInternal(const bool aIncludeUsername)
>+ nsAutoCString uri(NS_LITERAL_CSTRING("smtp://"));
nsCString (because of nvro).
I believe the SMTP parts of this are fixed, the LDAP parts are moved out to bug 833971.