Closed
Bug 801916
Opened 12 years ago
Closed 12 years ago
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
Categories
(MailNews Core :: Backend, defect)
MailNews Core
Backend
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 19.0
People
(Reporter: standard8, Assigned: standard8)
References
Details
(Keywords: intermittent-failure, regression)
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•12 years ago
|
||
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•12 years ago
|
||
Do these tests attempt to save the login information after they prompt for it?
Assignee | ||
Comment 3•12 years ago
|
||
I just confirmed this is a regression from bug 723004. I now need to remember what these tests do.
Blocks: 723004
Keywords: regression
Assignee | ||
Comment 4•12 years ago
|
||
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•12 years ago
|
||
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?
Assignee | ||
Comment 6•12 years ago
|
||
This definitely works for our xpcshell tests. Justin, thoughts?
Attachment #672267 -
Flags: review?(dolske)
Comment 7•12 years ago
|
||
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•12 years ago
|
||
My favorite of these options is (1).
Assignee | ||
Comment 9•12 years ago
|
||
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.
Assignee | ||
Comment 10•12 years ago
|
||
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.
Assignee: nobody → mbanner
Attachment #672267 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #672267 -
Flags: review?(dolske)
Attachment #672960 -
Flags: review?(irving)
Attachment #672960 -
Flags: review?(Pidgeot18)
Assignee | ||
Updated•12 years ago
|
Attachment #672960 -
Flags: review?(neil)
Comment 11•12 years ago
|
||
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.
Assignee | ||
Comment 12•12 years ago
|
||
(In reply to neil@parkwaycc.co.uk 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.
Assignee | ||
Comment 13•12 years ago
|
||
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.
Attachment #672960 -
Attachment is obsolete: true
Attachment #672960 -
Flags: review?(neil)
Attachment #672960 -
Flags: review?(irving)
Attachment #672960 -
Flags: review?(Pidgeot18)
Attachment #673152 -
Flags: review?(neil)
Comment 14•12 years ago
|
||
Comment on attachment 673152 [details] [diff] [review]
SMTP fix v2
Indeed, you appear to get nullptr back.
Attachment #673152 -
Flags: review?(neil) → review+
Assignee | ||
Comment 15•12 years ago
|
||
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.
Assignee | ||
Updated•12 years ago
|
Target Milestone: --- → Thunderbird 19.0
Assignee | ||
Updated•12 years ago
|
Flags: in-testsuite+
Assignee | ||
Comment 16•12 years ago
|
||
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.
Attachment #673206 -
Flags: review?(neil)
Attachment #673206 -
Flags: review?(irving)
Assignee | ||
Comment 17•12 years ago
|
||
Comment on attachment 673206 [details] [diff] [review]
Really fix SMTP
Sorry, somehow I keep thinking NeilAway is :neil.
Attachment #673206 -
Flags: review?(neil) → review?(neil)
Comment 18•12 years ago
|
||
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).
Attachment #673206 -
Flags: review?(neil) → review+
Assignee | ||
Updated•12 years ago
|
Attachment #673206 -
Flags: review?(irving)
Assignee | ||
Comment 19•12 years ago
|
||
Updated•12 years ago
|
Keywords: intermittent-failure
Updated•12 years ago
|
Whiteboard: [tb-orange]
Assignee | ||
Comment 20•12 years ago
|
||
I believe the SMTP parts of this are fixed, the LDAP parts are moved out to bug 833971.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•