80296 - "GetAllNewMessages" checks on default/selected account only

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Account Configuration, defect, P1)

Description

[DaTTa]

When using "Get All New Messages"  option, it just checks the accounts that have
been already checked.
Maybe it's only checking accouns with known passwords.

Comment 1

[racham]

Yes. The idea is to fetch mail for those accounts which have already been
authenticated (password entered in that session OR entered and stored by
password manager in it's database). If you see problems with the above, please
file a bug. Thanks.

Closing this as invalid. 

Comment 2

[Sheela Ravindran]

change->from esther to sheela

Comment 3

[[e-mail]]

I have my passwords stored in the password manager for the 3 POP accounts and
the IMAP account that I have, and once I open a new session, it doesn't check
any accounts with the "Get All New Messages" option until I manually go to each
account and check it. Then after that is checks the mail using the "Get All New
Messages" option.  
I thought if my passwords were stored, and I clicked the option on startup of a
new session, it should check all the accounts?  Is this wrong, or am I missing
the point?

Adding myself CC

Comment 4

[racham]

OK..if you stored the passwords already and clicked on GetAllNewMessages in a
new session, messages should be fetched for all those accounts without having
you to manullay get messages for each account first.  

Reopening this bug then. Sheela, please verify if this is happening. I will
start investigating on my end. thanks.

bhuvan

Comment 5

[Sheela Ravindran]

buildid:  2001051406 win98
confirming this bug. Get all new messages was working before but not 
now.
-Created multiple acconts in a profile-3pop, 1webmail(imap)
-Having multiple accounts in a profile the first account gets checked and the 
message is downloaded because both biff is on, check mail at startup is on and 
auto download is checked. 
-The remaining accounts when I first logged in clicked on get all new messages 
had no effect on the remaining 3 accounts.  
-clicked on the second account inbox and had the below error in the console
mailbox://3qatest07@nsmail-2/Inbox
Error loading with many headers to download: [Exception... "Component returned f
ailure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIMsgFolder.updateFolder]"
 nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame :: chrom
e://messenger/content/commandglue.js :: ChangeFolderByURI :: line 221"  data: no
]
-Then when clicked on the third account and did get message for that account 
retrieved the message
-same for the 4th account.
-same session I composed another mail message and sent to all the above accounts 
and did get all new messages
-retrieved message for 3 pop accounts and not for the web mail account

There was a problem with get all new messages and it was fixed. I guess this is 
a regression. 
adding keyword and nominating this for nsbeta1




 As reporter mentions I had to click on each account to get the new message.

Comment 6

[scottputterman]

marking nsbeta1+

Comment 7

[scottputterman]

Sheela, I just wanted to verify that in the scenario you wrote about, all of
your accounts had remember password on.

Comment 8

[scottputterman]

moving to 0.9.2.  If we get some time, I'd like to see this in 0.9.1

Comment 9

[scottputterman]

adding PDT+

Comment 10

[Sheela Ravindran]

Changing the summary of the bug. Get all new messages checks on the default
account only the first time you start the mail application.  Or checks any
account selected in the folder pane at the start up of app. When there are
multiple accounts with password saved for all still checks on the first account
only.  So  after manually logging into each one of them will make get all new
messages work for that session.
OR with password saved have to do get mssg on each account to make Get all new
messages work. 

After that Get all new messages will work for all the accounts in the same
session. If you close and come back in again then you have to log into each
account again to make it work.

Comment 11

[racham]

Attachment: patch that I used to test the concept getting the info from password mgr to make things work in mailnews..! It worked. Just attaching for your reference. But it will be better to add an api that returns the password string.

Comment 12

[racham]

Sorry..the attachment posted above (id:37911) is meant for bug 84834. I do have
a patch which fixed the problem in mailnews (that's certainly more code). Will
post that just as backup. But as I mentioned earlier, the beter approach is to
make nsIPasswordMgr to provide this service (of getting password).

bhuvan

Comment 13

[racham]

Attachment: Patch contingent to nsPassword's GetPassword() routine (suggested in bug 84834)

Comment 14

[racham]

Attachment: latest patch

Comment 15

[racham]

When tested with encrypted scenario, I was asked for master password 3 times (1
account or 2 accounts). I asked Steve Morse about when I can expect such a thing
as it doesn't look right to me.

bhuvan

Comment 16

[racham]

Posting my email session with Steve Morse :

[My query]
Bhuvan Racham wrote:

>
> Hi Steve,
>
> Here is scenario that is kind of puzzling for me.
>
> I built mozilla with the flag (BULD_PSM2) you suggested.
>
> When I tried to enyumerate thru the password manager to find out the
> password for given server, I have been master password being asked 3
> times (all 3 standard modal dialogs which come up when we ned to enter
> master password dialog). When can I expect these multiple dialogs. I
> think something is wrong here. I am attaching the code that I have which
> is called for every mail account you have. Surprisingly, I got the
> master password dialog 3 times both for 1 and 2 account cases.
>
> bhuvan

[Steve Morse's response]
What you are describing sounds like a psm/modal-dialog problem.  You
should only get that dialog once and it should be modal.  But from what
you are saying, I presume that you are getting three dialogs all at the
same time, before you even had a chance to respond to any of them.  Is
that correct?

There are known problem with modal dialogs not being modal (which is why
I copied danm on this), and also there are things that the caller of a
modal dialog is supposed to do to make sure his dialog is modal (that's
why I copied thayes on this).  These are the people you need to be
addressing your question to.

As a work-around, is there something that you can do before calling the
password enumerator that will force the master-password dialog to come
up?  In that case it should come up only once.  Then when you go to the
enumerator, the database has already been unlocked and you should get no
further dialogs.

-- Steve

Comment 17

[racham]

Steve,

2 dialogs come up immediately overalpped. Once I fill one after another, I see
the third (last) one coming up. I have to fill that to have gates opened for
password database. By the dialogs are first shown when the enumerator's
GetNext() is first called (see latest patch ID : 38362).

Your work around seemed reasonable. But, I will have to see whether I can create
such an opportunity here. Because we don't want to throw any dialog up there
unless otherwise required, it might be little trickier to identify and bring up
the dialog. That's an API to trigger masterpassword, without taking enumeration
path, would have been good. thanks.

bhuvan

Comment 18

[scottputterman]

moving to 0.9.3

Comment 19

[scottputterman]

removing the nsbranch

Comment 20

[scottputterman]

moving to 0.9.4

Comment 21

[Scott MacGregor]

Mail news triage meeting --> .9.5

Comment 22

[Sheela Ravindran]

Nominating this for the next release. There in no point of having 
GetAllNewMessages if it does not work for all accounts. 

Comment 23

[Scott MacGregor]

not an eMojo stopper. nsBranch-.

Triaging for the next release. 

Comment 24

[R.K.Aa.]

*** Bug 92374 has been marked as a duplicate of this bug. ***

Comment 25

[scottputterman]

moving to 0.9.7

Comment 26

[racham]

Adding dependency on 60150 [Modal dialogs (alert, confirm, prompt) do not halt
code execution].

Comment 27

[racham]

Fix to 60150 might solve this problem rightway. But I want to ask to Steve if it
is possible for him to do the enumeration (& subsequent checks) I am doing on
mailnews end, on his end i.e., password manager component. I have requested so
keeping fact that PasswordManager seem to be running on thread that does not
suffer from this modal dialog problems, example being the viewStored Passwords
menu item which enumerates thru passwords list. thanks.

Comment 28

[racham]

I tried moving the enumeration code into PasswordManager itself. That didn' help
either. So, the only real way to fix this is to fix the modal dialog problem
(bug 60150).

Comment 29

[racham]

Attachment: Fresh patch, v 1.0 - Gets messages for all authenticated accounts (in both 'master password' and 'no master password' cases)

Comment 30

[racham]

The latest patch posted (id=65186) fixes all the problems that we faced in the
past. 

So, the original problem was that when user clicked on GetAllNewMessages, it got
messagse only for those accounts which have been explicitely
authenticated/accessed   (in case of accounts whose passwords are already
remembered) in that session. The ideal situation is that it should just get
messages for all those accounts whose passwords are stored (without having the
need of them being accessed in that session) plus those accounts which have been
explicitely authenticated (i.e., via password dialog).

Current patch, when asked to get messages for all authenticated accounts, 
enumerates through password manager database and extracts the passwords of
accounts which have been remembered and enables the process of getting new
messages for all those accounts. Also, it keeps the original behavior of getting
new messages for all those accounts which have been explicitely authenticated.

Incase user has a master password, the password willbe asked just once, unlike
several times as seen in the past. A small fix that removed a redundant call
enabled the proper behior. However, the core bug where the dialogs does not stop
of code execution still exists (as it can be reproduced by adding back that
redundant call). But, it is not a blocker of this bug anymore.

Thanks to Steve Morse for implementing the interfaces needed for this patch.
Thanks to DanM and Scott Putterman for their insights.

Now onto the review-seeking process...

Comment 31

[Scott MacGregor]

Comment on attachment 65186 [details] [diff] [review]
Fresh patch, v 1.0 - Gets messages for all authenticated accounts (in both 'master password' and 'no master password' cases)

r=mscott

Code looks good Bhuvan. Couple comments:

1) can we rename enumerateForPassword to something more generic like....well
actually i can't think of a good name for that routine now that I think about
it. You don't want to say enumerateWalletForPassword since that implies a
certain implementation. Hmmm....

2) can we optimize the while loop which iterates over the password entries by
pulling the following variable declaration up out of the loop?
nsCOMPtr<nsIPassword> passwordElem;

Comment 32

[racham]

Scott, thanks for review and your comments. 
Will post a patch with comments incorporated soon. For now, moving the milestone
to 099. Adding patch keyword.

Comment 33

[racham]

How about changing the routine to say GetPasswordWithoutUI which will return the
password (empty or filled) -> [interface : string GetPasswordWithoutUI()] OR
GetPasswordIfAuthenticated() similarly returning a password string ? 

If GetPasswordWithoutUI ever existed in the past (Scott remembers such a routine
being there..), does anyone know why it was taken out..? thanks.

Comment 34

[racham]

Attachment: patch, v1.1 - Scott's comments are incorporated. Changed the routine signature to return a (password) string if authenticated

Scott, 

I have changed the routine name to getPasswordIfAuthenticated(). Let me know 
if you thought of any better name later..If you want, you can go through the
patch again. The only major difference is the signature of the routine that
gives us the password. thanks.

Comment 35

[(not reading, please use [e-mail] instead)]

the nsIPasswordManager interface is not frozen, and it seems like your code to 
find a nsIPassword is generic, and would be wanted by other people.

see what morse says about doing this:

nsIPassword find(string host, wstring username, wstring password);

The implemenation will allow for null (or empty strings), and treat them like 
wild cards, like we do with FindServer() in the mailnews code.

Then, the implemenation can enumerate all the nsIPasswords looking for the 
first "match" and return it.

this would allow the mailnews code to pass in a host, but no username or 
password, to see if there was a nsIPassword object, and from it we would see if 
there was a password.

this would be generic enough to allow other people to lookup give just 
username, or just password, to find the nsIPassword, in order to find the other 
fields.

Comment 36

[Stephen P. Morse]

There is a problem with the attached patch in that it does an include of 
nsIPassword.h.  That will create a dependency on a module in the extensions 
directory.  See bug 113540, as well as brendan's recent e-mail on being able to 
build without the extensions directory.

That said, it seems like seth's suggestion of putting this routine into password 
manager is not only a possible solution, it is the only solution.  If you'll 
generate the patch for doing so, I'll gladly review it.

And seth, while we're at it, I'm still waiting for your review on bug 113540.

Comment 37

[Alec Flett]

another issue with this patch - you're using AssignWithConversion to convert
UCS2 to ASCII - the GetPassword() routine is clearly designed to deal with
unicode, so you need to figure out what charset (likely utf8) the password is
in, and do the conversion there.

If you can guarantee that no conversion is actually required, you should be
using NS_LossyConvertASCIItoUCS2(). AssignWithConversion() is going away (there
will be a more formal announcement coming up)

Comment 38

[racham]

Attachment: patch, v1.2 - mailnews diffs - removed compile dependencies on extensions and password is obtained querying passwordmanager via generic findPassword interface suggested

Comment 39

[racham]

Attachment: patch, v1.2 - password mgr diffs - Added an interface that allows any caller to get a passowrd object attributes by passing either some or none input parameters. Empty string "" is treated as a match.

Comment 40

[Stephen P. Morse]

Comments on patch v1.2 for password mgr diffs

1. Why are these three lines not consistent?

 PRBool checkHostname = !(nsCRT::strcmp(*hostname, ""));
 PRBool checkUsername = !(nsCRT::strcmp(*username,NS_LITERAL_STRING("").get()));
 PRBool checkPassword = !(nsCRT::strcmp(*password,NS_LITERAL_STRING("").get()));

2. Your code is confusing because you call the variable checkHostname whereas it 
really means that you don't need to check the hostname because it is a wildcard.  
I would suggest restructuring it as follows to make it easier to understand:

  PRBool hostnameOK =
            !(nsCRT::strcmp(*hostname, "")) ||
            !(nsCRT::strcasecmp(*hostname, thisHostname.get());
  PRBool usernameOK = ...
  PRBool passwordOK = ...
  if (hostnameOK && usernameOK && passwordOK) {

3. The following code looks wrong to me

+          if (!(nsCRT::strlen(*hostname))) {
+            nsMemory::Free(*hostname);
+            *hostname = nsCRT::strdup(thisHostname.get());
+          }

You are freeing the string if it has a length of 0.  If so, why do you need to 
free it?

Comment 41

[Mike Shaver (:shaver emeritus)]

> You are freeing the string if it has a length of 0.  If so, why do you need to 
> free it?

A string with a length of zero[*] still represents an allocation of at least one
byte, for the NUL terminator.

[*] But we don't need to compute the length of the string in order to determine
that it's empty: 

  if (!*hostname)

tests emptiness without walking the whole string.

This is probably a better system than calling through nsCRT to explicitly
compare against an empty string (possibly runtime-allocated, if
NS_LITERAL_STRING doesn't expand elegantly), for the
checkHostname/Username/Password.  (I expect they're inconsistent because
hostname is char * while username and password are PRUnichar *, but I haven't
read the code in detail.)

Comment 42

[racham]

Shaver is right.

Those strings were released as they came as input params with some memory
allocated. Releasing the memory before assigning new value.

Inconsistency in the usage of NS_LITERAL_STRING as hostname is char* and
username and password are PRUnichar*.

I will polish the code per Steve's second comment to make it more readable and
clear.

Also, regd Shaver's point about making check for emptyness using

if (!*hostname)

is fine for char* (i.e., for hostname). But for PRUnichar*, I needed to check
length as check on say *username [like if (!*username)] will not fail if the
string is allocated but empty and hence will not get a chance to extract that
attribute of password element.

Comment 43

[Stephen P. Morse]

OK, just make the change suggested in my second item and remove the strlen ass 
suggested by shaver.  With that, r=morse

Comment 44

[Stephen P. Morse]

Clarification: remove the strlen only for hostname as per shaver's suggestion.

Comment 45

[Mike Shaver (:shaver emeritus)]

I don't understand this at all:

> if (!*hostname)
> 
> is fine for char* (i.e., for hostname). But for PRUnichar*, I needed to check
> length as check on say *username [like if (!*username)] will not fail if the
> string is allocated but empty and hence will not get a chance to extract that
> attribute of password element.

*username will be false iff the first character (PRUnichar) of username is 0. 
That will be the case iff the string is allocated but empty.  Why would
|PRUnichar *| behave differently than |char *| in this regard?

But the code I'm asking about isn't checking the length, just comparing it to
the empty string:

 PRBool checkUsername = !(nsCRT::strcmp(*username,NS_LITERAL_STRING("").get()));
 PRBool checkPassword = !(nsCRT::strcmp(*password,NS_LITERAL_STRING("").get()));

Maybe I need to read the whole patch to find the case you're talking about
(another free-if-allocated-but-empty stanza), but that one is pretty clear.

I'll review the whole patch now.

Comment 46

[Mike Shaver (:shaver emeritus)]

Comment on attachment 66617 [details] [diff] [review]
patch, v1.2 - password mgr diffs - Added an interface that allows any caller to get a passowrd object attributes by passing either some or none input parameters. Empty string "" is treated as a match.

>+// Take hostname/username/password as input parameter(s) and return 

Why "parameter(s)"?  C++ requires us to pass all of them for every call.

But why would you ever need to call FindPassword if you have a password
to pass in?  Shouldn't |password| be the IDL return value, while |hostname|
and |username| are |inout| params?

>+    PRBool hasMoreElements = PR_FALSE;
>+    enumerator->HasMoreElements(&hasMoreElements);
>+    // Emumerate through password elements
>+    while (hasMoreElements) {

Are you intentionally ignoring the return value of |HasMoreElements| here?
(Or does nsISimpleEnumerator::HasMoreElements guarantee that the boolean
out param won't be mutated in case of failure?)

>+        // Check if any of the params match wild card entry, i.e., ""
>+        PRBool checkHostname = !(nsCRT::strcmp(*hostname, ""));
>+        PRBool checkUsername = !(nsCRT::strcmp(*username, NS_LITERAL_STRING("").get()));
>+        PRBool checkPassword = !(nsCRT::strcmp(*password, NS_LITERAL_STRING("").get()));

As I said in my previous comment: it's better to check emptiness via a 
cheap test of hostname[0]/username[0]/password[0] than to potentially
allocate an empty wise string at runtime and then perform a string
comparison.

And a check of hostname[0] etc. will allow you to merge that logic
with the below:

>+        // Check to see if there is much either with wild card entry or
>+        // with the actual value passed in.
>+        if ((checkHostname || !(nsCRT::strcasecmp(*hostname, thisHostname.get()))) &&
>+            (checkUsername || !(nsCRT::strcmp(*username, thisUsername.get()))) && 
>+            (checkPassword || !(nsCRT::strcmp(*password, thisPassword.get())))) 

if ((!hostname[0] || !nsCRT::strcasecmp(*hostname, thisHostname.get()) &&&
    (...))

Alternatively, since you never mutate the input parameters until a match
is found, hoist the invariant emptiness checks out of the loop, and set
check{Hostname,Username,Password} only once each.

(I agree with Steven about the backwards naming of |checkHostname| etc.,
though: either rename them to |emptyHostname| or some such, or invert the
value they store.  Renaming will make for cleaner logic in the match stanza,
I think.)

>+        {
>+          if (!(nsCRT::strlen(*hostname))) {
>+            nsMemory::Free(*hostname);
>+            *hostname = nsCRT::strdup(thisHostname.get());
>+          }
>+          if (!(nsCRT::strlen(*username))) {
>+            nsMemory::Free(*username);
>+            *username = nsCRT::strdup(thisUsername.get());
>+          }
>+          if (!(nsCRT::strlen(*password))) {
>+            nsMemory::Free(*password);
>+            *password = nsCRT::strdup(thisPassword.get());
>+          }

Again: don't compute string length just to test emptiness:

  if (!hostname[0])
  if (!username[0])
  if (!password[0])

But see question above about why we might have an initial value for
password in any case.

>Index: nsIPasswordManager.idl
>===================================================================
>RCS file: /cvsroot/mozilla/netwerk/base/public/nsIPasswordManager.idl,v
>retrieving revision 1.3
>diff -u -w -r1.3 nsIPasswordManager.idl
>--- nsIPasswordManager.idl	30 Oct 2001 22:06:54 -0000	1.3
>+++ nsIPasswordManager.idl	26 Jan 2002 21:22:29 -0000
>@@ -36,6 +36,7 @@
>   void addUser(in string host, in wstring user, in wstring pwd);
>   void removeUser(in string host, in wstring user);
>   void removeReject(in string host);
>+  void findPassword(inout string hostname, inout wstring username, inout wstring password);

Better, if we never need to pass in a password:

wstring findPassword(inout string hostname, inout wstring username);

But what of encoding?  The necko interfaces claim their hostname
|strings| to be UTF-8, IIRC, so is this UTF-8 as well?	If so,
nsCRT::strcasecmp is the wrong thing for comparing the caller-provided
hostname with the stored one; you'll need to inflate both to UCS2 and
then compare.

Steven: does the password manager store its hostnames in UTF8 or ASCII?

Comment 47

[Mike Shaver (:shaver emeritus)]

Comment on attachment 66616 [details] [diff] [review]
patch, v1.2 - mailnews diffs - removed compile dependencies on extensions and password is obtained querying passwordmanager via  generic findPassword interface suggested

>+  /* if the password is avialable, get it */
>+  string getPasswordIfAuthenticated();

Spelling: "available".

>+// If the password for the server is avialable either via authentication in the current

(Spelling again.)

>+      // Get the current server URI
>+      nsXPIDLCString currServerUri;
>+      rv = GetServerURI(getter_Copies(currServerUri));
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      char* hostName;
>+      hostName = nsCRT::strdup(currServerUri.get());

Is the server URI just the hostname?  I'd have thought it
included protocol and other information.  If that's really
what the password manager expects, both the parameters and
argument should be renamed.

Comment 48

[Mike Shaver (:shaver emeritus)]

I missed something important before (glad I came back to double check!): why do
findPassword and the password manager use |wstring| for passwords, while
getPasswordIfAuthenticated uses |string|?

Comment 49

[(not reading, please use [e-mail] instead)]

to help answer shaver's question, see:

http://lxr.mozilla.org/mozilla/source/extensions/wallet/public/nsIPassword.idl#30

 30 interface nsIPassword : nsISupports {
 31     readonly attribute string host;
 32     readonly attribute wstring user;
 33     readonly attribute wstring password;
 34 };
 
As for why that's a wstring, I haven't unwound it all yet.

Comment 50

[Stephen P. Morse]

Using wstring for username and password allows you to have foreign characters in 
those items.

Comment 51

[Alec Flett]

I think the real question is, why is the OTHER code using "string" instead of
"wstring" - everyone should be using wstring for both username and password

Comment 52

[Mike Shaver (:shaver emeritus)]

Yes, that was my question. =)

Comment 53

[Matthias Versen [:Matti]]

*** Bug 123082 has been marked as a duplicate of this bug. ***

Comment 54

[Shoshannah Forbes]

also broken on Mac Os 9.2.2 

Comment 55

[racham]

Attachment: patch, v1.3 - password manager diffs - updated with feedback from Morse and Shaver

Morse & Shaver, Thanks for reviews. Here is an updated patch. 

* Restructured hostURI, username and password check logic
* Input params are checked with if (!*hostURI), if (!*username), etc., syntax 
* Modified comments 
* Enumerator obtained from PasswordMgr always returns NS_OK and the boolean
value that gets filled in represents the correct status. No need to check for
rv there.
* Changed routine name to findPasswordEntry to indicate that one can obtain the
whole password object interms of attributes by passing in known attribute
values

Comment 56

[racham]

Attachment: patch, 1.3 - mailnews diffs - updated patch

* Fixed spelling mistakes
* Parameter and argument are renamed from hostname to hostURI to reflect the
data it represents
* Password Manager uses wstring for password. But IMAP and POP3 protocol
services on our code base use  char* for password when transact with servers
LOGIN command incase of IMAP and PASS in case of POP3. So, as a caller, I am
getting the wstring value and converting it to string as needed for protocols.

Comment 57

[Alec Flett]

Comment on attachment 68276 [details] [diff] [review]
patch, v1.3 - password manager diffs - updated with feedback from Morse and Shaver

>+
>+  // Takes hostname, username and password as input parameters and returns 
>+  // set of filled-in hostname, username and password for the first
>+  // password element match. Empty string is treated as a wild
>+  // card entry and will be considered as a match for any of the input
>+  // parameters. 
>+  void findPasswordEntry(inout string hostURI, inout wstring username, inout wstring password);
>+


good documentation! :)

> }
>+
>+NS_IMETHODIMP
>+nsPasswordManager::FindPasswordEntry(char **hostURI, PRUnichar **username, PRUnichar **password)
>+{
>+  NS_ENSURE_ARG_POINTER(hostURI);
>+  NS_ENSURE_ARG_POINTER(username);
>+  NS_ENSURE_ARG_POINTER(password);
>+
>+  nsresult rv;
>+  nsCOMPtr<nsIPassword> passwordElem;
>+    // Emumerate through password elements
>+    while (hasMoreElements) {
>+      rv = enumerator->GetNext(getter_AddRefs(passwordElem));

This compiles? Does this do an implicit QueryInterface()? I'm really curious!
If this compiles but doesn't do an implicit QI, I think nsCOMPtr is broken.
anyone? shaver?

>+
>+        // Check if any of the params are empty and treat them as matches or if they match
>+        // with current password element attribute values.
>+        PRBool hostURIOK = !(*hostURI && nsCRT::strcasecmp(*hostURI, thisHostURI.get()));
>+        PRBool usernameOK = !(*username && nsCRT::strcmp(*username, thisUsername.get()));
>+        PRBool passwordOK = !(*password && nsCRT::strcmp(*password, thisPassword.get()));
>+

Huh?  did you mean
PRBool usernameOK = (*username && !nsCRT::strcasecmp(*username,
thisHostURI.get()));

Otherwise this logic is just wierd. Does this code even work?

you could really make this more readable by going through the standard string
methods:
PRBool usernameOK = (*username && thisUsername.Equals(*username));

For Equals() with case-insensitivity, you should use
nsCaseInsensitive[C]StringComparator()
as the 2nd parameter to Equals.

>+        // If a password match is found based on given input params, 
>+        // fill in those params which are passed in as empty strings.
>+        if (hostURIOK && usernameOK && passwordOK)
>+        {
>+          if (!*hostURI) {
>+            nsMemory::Free(*hostURI);
>+            *hostURI = nsCRT::strdup(thisHostURI.get());
>+          }

Once again, has this even been tested?	You only want to free null pointers?
Did you mean:
if (*hostURI)
  nsMemory::Free(*hostURI);
*hostUri = nsCRT::strdup(thisHostURI.get());?

once again, you should use the standard string routines:

if (*hostURI)
  nsMemory::Free(*hostURI);
*hostURI = ToNewCString(thisHostURI);

Comment 58

[Alec Flett]

Comment on attachment 68282 [details] [diff] [review]
patch, 1.3 - mailnews diffs - updated patch

>Index: base/public/nsIMsgIncomingServer.idl
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/base/public/nsIMsgIncomingServer.idl,v
>retrieving revision 1.74
>diff -u -w -r1.74 nsIMsgIncomingServer.idl
>--- base/public/nsIMsgIncomingServer.idl	5 Feb 2002 01:34:20 -0000	1.74
>+++ base/public/nsIMsgIncomingServer.idl	7 Feb 2002 06:53:02 -0000
>@@ -324,6 +324,9 @@
> 
>   long getIntAttribute(in string name);
>   void setIntAttribute(in string name, in long value);
>+
>+  /* if the password is available, get it */
>+  string getPasswordIfAuthenticated();


this function returns an allocated string, but the only time you ever use it 
is to check if ANY string is returned:

>                            currentServer.type].getService(Components.interfaces.nsIMsgProtocolInfo);
>-      if (protocolinfo.canGetMessages && currentServer.password) {
>+      if (protocolinfo.canGetMessages && currentServer.getPasswordIfAuthenticated()) {
>         // Get new messages now
>         GetMessagesForInboxOnServer(currentServer);

instead, you should create a "readonly attribute boolean authenticated;"



>+      // Obtain the server URI which is in the format <protocol>://<userid>@<hostname>.
>+      // Password manager uses the same format when it stores the password on user's request.
>+      char* hostURI;
>+      hostURI = nsCRT::strdup(currServerUri.get());
>+
>+      nsXPIDLString userName;
>+      nsXPIDLString password;
>+
>+      // Get password entry corresponding to the host URI we are passing in.
>+      rv = passwordMgr->FindPasswordEntry(&hostURI, getter_Copies(userName), getter_Copies(password));

I think there's a better way to use hostURI... try this:

nsXPIDLString hostURI;
hostURI.Adopt(ToNewCString(currServerUri));

then I think you can use getter_Copies and the string will be freed
appropriately.

>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      // If a match is found, password element is filled in. Convert the
>+      // obtained password and store it for the session.
>+      if (!password.IsEmpty()) {
>+        SetPassword(NS_LossyConvertUCS2toASCII(password).get());

"Lossy" throws a big red flag to me, and it should have thrown one to you
too... you're clearly
mangling a value that is UCS2. You need to figure out what charset this string
is in and convert it
appropriately. My first guess would be that it's in UTF8.

I happen to know that this is simply a bad conversion, but in the future when
you MUST
write code that uses NS_LossyConvertUCS2toASCII(), you MUST document why it is
safe
to do a Lossy conversion.

Comment 59

[racham]

Attachment: patch, v1.4 - password mgr diffs - Incorporated Alec's suggestions

I did compile and test all these patches (each version). Thanks for your
suggestions. Here is an updated version of password manager diffs incorporating
your suggestions.

+	 // Check if any of the params are null (set by getter_Copies as
+	 // preparation for output parameters) and treat them wild card
+	 // entry matches or if they match with current password element 
+	 // attribute values.
+	 PRBool hostURIOK  = !*hostURI	|| thisHostURI.Equals(*hostURI);
+	 PRBool usernameOK = !*username || thisUsername.Equals(*username);
+	 PRBool passwordOK = !*password || thisPassword.Equals(*password);

When caller needs to get one of the values, say password, it does getter_Copies
for password in it's code which will come in with value null. So, we treat that
as a match and then try to match the actual for the other 2 variables viz.,
hostURI and username. password will be treated as match via !*password and the
other two have to match via Equal() routine. Once all these values are matched,
we just fill in those variables which need to be filled in the following block.


+	 // If a password match is found based on given input params, 
+	 // fill in those params which are passed in as empty strings.
+	 if (hostURIOK && usernameOK && passwordOK)
+	 {
+	   if (!*hostURI) {
+	     *hostURI  = ToNewCString(thisHostURI);
+	   }
+	   if (!*username) {
+	     *username = ToNewUnicode(thisUsername);
+	   }
+	   if (!*password) {
+	     *password = ToNewUnicode(thisPassword);
+	   }
+	   break; 
+	 }

This interface is designed to allow caller to pass any of the known attribute
values of password entry and get the rest.

Comment 60

[racham]

Attachment: patch, v1.4 - password mgr diffs - please ignore the last attachment, comments added there are still applicable

Comment 61

[racham]

Attachment: patch, v1.4 - mailnews diffs - updated patch with Alec's suggestions

1. Changed interface to an attribute as suggested i.e., readonly attribute
boolean isAuthenticated.

2. hostURI needed to be char* as we need to pass the value of hostURI we are
looking for. getter_Copies can't be used on this one as the value will be null
by the it reaches the password manager routine.

3. password is stored in UTF8. So, using NS_ConvertUCS2toUTF8 for conversion.

Please continue to add your feedback and comments. Will modify these patches as
needed. Thanks.

Comment 62

[Alec Flett]

Comment on attachment 68482 [details] [diff] [review]
patch, v1.4 - password mgr diffs - please ignore the last attachment, comments added there are still applicable

an excellent patch - ample comments, well laid out, easy to read and understand
:) Just wanted to point that out!
sr=alecf

Comment 63

[Alec Flett]

Comment on attachment 68483 [details] [diff] [review]
patch, v1.4 - mailnews diffs - updated patch with Alec's suggestions

what happened to using Adopt()?

If Adopt() didn't work for some reason (explain that here) you should at least
move the nsMemory::Free() before the if (!password.IsEmpty()) so that if
SetPassword() fails, you still free the string.

close.. :)

Comment 64

[racham]

From one of the previous upadtes :
---
nsXPIDLString hostURI;
hostURI.Adopt(ToNewCString(currServerUri));

then I think you can use getter_Copies and the string will be freed
appropriately.
---

There is no problem with Adopt(). But the problem is with getter_Copies(). I 
can't use getter_Copies due to the fact that the variable associated with it 
will be prepared as output param and any data in there will be wiped out by the 
time it reaches PasswordMgr's FindPasswordEntry routine. In the inout context, 
hostURI is serving as an input param and it's value shall not be disturbed. 

I will move the code Free() up as you suggested.

Thanks for all your reviews and comments.

Comment 65

[Alec Flett]

Comment on attachment 68483 [details] [diff] [review]
patch, v1.4 - mailnews diffs - updated patch with Alec's suggestions

great, thanks for the info. I'll just sr=alecf this patch, with the Free()
moved earlier.

Comment 66

[racham]

Attachment: patch, v1.5 - mailnews diffs - updated patch (moving nsMemory::Free call up)

Alec, thanks for a great review and your continuous feedback.
Also, thanks to Morse and Shaver.

Poting a new mailnews patch after moving Free() call as suggested.

Comment 67

[racham]

Comment on attachment 68629 [details] [diff] [review]
patch, v1.5 - mailnews diffs - updated patch (moving nsMemory::Free call up)

sr=alecf (per his update)

Comment 68

[Scott MacGregor]

Comment on attachment 68629 [details] [diff] [review]
patch, v1.5 - mailnews diffs - updated patch (moving nsMemory::Free call up)

we already have a method on the incoming server called:
GetServerRequiresPasswordForBiff
which I believe biff uses to determine whether it can run  biff on that server
or not. Instead of adding this new method, why don't we add your code for that
method to GetServerRequiresPasswordForBiff. That is, if the server (or the
password manager) already has a password then return false for this routine. 

This is how imap already uses this method (i.e. it just checks to see if the
user has been authenticated already and returns false if we are already
authenticated)

Comment 69

[Scott MacGregor]

Comment on attachment 68482 [details] [diff] [review]
patch, v1.4 - password mgr diffs - please ignore the last attachment, comments added there are still applicable

r=mscott however my r= doesn't mean anything here since we still need module
owner approval for this change. You'll want to get the real r= from morse since
he's the module owner.

Comment 70

[Stephen P. Morse]

I thought I gave a review on the password manager part in comment #43.  Anyhow, 
r=morse.

Comment 71

[racham]

Combining with GetServerRequiresPasswordForBiff will work out well except in one
case. It the case where user starts with account central page and has biff set
up to happen after 'x' minutes && stays wihtout logging in for 'x' minutes.
After 'x' minutes, the master password dialog will pop up for performing biff as
the current patch enumerates through password manager database in search of a
password.

In such a situation, today (with current builds on sweetlou), biff simply
doesn't happen until user takes some action that lets the app to get password
for the given server.  

Comment 72

[Scott MacGregor]

Comment on attachment 68629 [details] [diff] [review]
patch, v1.5 - mailnews diffs - updated patch (moving nsMemory::Free call up)

r=mscott we decided to leave 2 separate methods on the class because of the
undesired side effects of merging them together.

Comment 73

[racham]

Thankd everyone for great reviews and feedback.

This is finally done !

Fix checked in. 

Marking Fixed.

Comment 74

[Dawn Endico]

removed the item for this bug from the release notes for 0.9.9 and beyond,
because the bug is fixed now. Let me know if you think the item should be 
re-added.

Comment 75

[Andrew Lin]

*** Bug 129403 has been marked as a duplicate of this bug. ***

Comment 76

[esther]

changing qa to esther, I will be verifying this

Comment 77

[esther]

Using build 20020326 on winxp and running these tests:
Created a new profile with 2 pop, 1 imap and 1 Netscape Web Mail account.
Account settings= Check for new mail at startup = on 
Biff = on at 10 minute intervals
POP preference Automatically Download any new messages = on

First test:
PW not saved
Logged into all accounts
Get All new msgs = OK

Second test:
PW saved = obscure
Launch app, open mail, check for new mail pref worked on all, I saw the new mail
icon but did not open any messages.  Then I sent sent a message to all accounts
from another system waited 30 seconds and did a Get All new messages = OK 

Third test:
PW saved = encrypted
Launch app, open mail, check for new mail pref worked since it prompted for
master password. Gave master password and new messages arrived.  I then sent a
message to all accounts from another system waited 30 seconds and did a Get All
new mail=OK

Forth test: 
PW saved = encrypted for mail but also for browser page
Launch app, log into browser page which is saved encrypted, gave master password
Opened mail, check for new mail perf workded first.  Did not open any msgs,
lever mail accounts collapsed then selected Get All new mail = OK

So far, verified for Windows, still need to test linux and mac.

Comment 78

[esther]

Note: another test I ran:
Fifth test: 
PW saved = check for new mail at start up =OFF, launch app
Opened mail with folders collapsed,clicked Get All new messages =OK

Comment 79

[esther]

Using build 20020327 on linux and the same scenario above all 5 tests passed.
Verified for linux, now testing Mac

Comment 80

[esther]

Using build 20020327 on mac 9.1 and 10.1 running the 5 tests mentioned above
this is fixed. Verified.