DaTTa Reporter
	Description • 23 years ago

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.

	racham Assignee
	Comment 1 • 23 years ago

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. 

	Sheela Ravindran
	Comment 2 • 23 years ago

change->from esther to sheela

	scb147@bakerpage.com
	Comment 3 • 23 years ago

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

	racham Assignee
	Comment 4 • 23 years ago

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

	Sheela Ravindran
	Comment 5 • 23 years ago

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.

	scottputterman
	Comment 6 • 23 years ago

marking nsbeta1+

	scottputterman
	Comment 7 • 23 years ago

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

	scottputterman
	Comment 8 • 23 years ago

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

	scottputterman
	Comment 9 • 23 years ago

adding PDT+

	Sheela Ravindran
	Comment 10 • 23 years ago

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.

	racham Assignee
	Comment 12 • 23 years ago

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

	racham Assignee
	Comment 15 • 23 years ago

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

	racham Assignee
	Comment 16 • 23 years ago

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

	racham Assignee
	Comment 17 • 23 years ago

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

	scottputterman
	Comment 18 • 23 years ago

moving to 0.9.3

	scottputterman
	Comment 19 • 23 years ago

removing the nsbranch

	scottputterman
	Comment 20 • 23 years ago

moving to 0.9.4

	Scott MacGregor
	Comment 21 • 23 years ago

Mail news triage meeting --> .9.5

	Sheela Ravindran
	Comment 22 • 23 years ago

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

	Scott MacGregor
	Comment 23 • 23 years ago

not an eMojo stopper. nsBranch-.

Triaging for the next release. 

	R.K.Aa.
	Comment 24 • 23 years ago

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

	scottputterman
	Comment 25 • 23 years ago

moving to 0.9.7

	racham Assignee
	Comment 26 • 23 years ago

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

	racham Assignee
	Comment 27 • 23 years ago

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.

	racham Assignee
	Comment 28 • 23 years ago

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).

	racham Assignee
	Comment 30 • 23 years ago

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...

	Scott MacGregor
	Comment 31 • 23 years ago

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;

	racham Assignee
	Comment 32 • 23 years ago

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.

	racham Assignee
	Comment 33 • 23 years ago

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.

	racham Assignee
	Comment 34 • 23 years ago

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.

	(not reading, please use seth@sspitzer.org instead)
	Comment 35 • 23 years ago

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.

	Stephen P. Morse
	Comment 36 • 23 years ago

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.

	Alec Flett
	Comment 37 • 23 years ago

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)

	Stephen P. Morse
	Comment 40 • 23 years ago

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?

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 41 • 23 years ago

> 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.)

	racham Assignee
	Comment 42 • 23 years ago

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.

	Stephen P. Morse
	Comment 43 • 23 years ago

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

	Stephen P. Morse
	Comment 44 • 23 years ago

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

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 45 • 23 years ago

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.

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 46 • 23 years ago

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?

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 47 • 23 years ago

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.

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 48 • 23 years ago

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|?

	(not reading, please use seth@sspitzer.org instead)
	Comment 49 • 23 years ago

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.

	Stephen P. Morse
	Comment 50 • 23 years ago

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

	Alec Flett
	Comment 51 • 23 years ago

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

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 52 • 23 years ago

Yes, that was my question. =)

	Matthias Versen [:Matti]
	Comment 53 • 23 years ago

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

	Shoshannah Forbes
	Comment 54 • 23 years ago

also broken on Mac Os 9.2.2 

	racham Assignee
	Comment 55 • 23 years ago

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

	racham Assignee
	Comment 56 • 23 years ago

* 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.

	Alec Flett
	Comment 57 • 23 years ago

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);

	Alec Flett
	Comment 58 • 23 years ago

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.

	racham Assignee
	Comment 59 • 23 years ago

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.

	racham Assignee
	Comment 61 • 23 years ago

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.

	Alec Flett
	Comment 62 • 23 years ago

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

	Alec Flett
	Comment 63 • 23 years ago

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.. :)

	racham Assignee
	Comment 64 • 23 years ago

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.

	Alec Flett
	Comment 65 • 23 years ago

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.

	racham Assignee
	Comment 66 • 23 years ago

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.

	racham Assignee
	Comment 67 • 23 years ago

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

sr=alecf (per his update)

	Scott MacGregor
	Comment 68 • 23 years ago

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)

	Scott MacGregor
	Comment 69 • 23 years ago

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.

	Stephen P. Morse
	Comment 70 • 23 years ago

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

	racham Assignee
	Comment 71 • 23 years ago

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.  

	Scott MacGregor
	Comment 72 • 23 years ago

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.

	racham Assignee
	Comment 73 • 23 years ago

Thankd everyone for great reviews and feedback.

This is finally done !

Fix checked in. 

Marking Fixed.

	Dawn Endico
	Comment 74 • 23 years ago

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.

	Andrew Lin
	Comment 75 • 23 years ago

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

	esther
	Comment 76 • 23 years ago

changing qa to esther, I will be verifying this

	esther
	Comment 77 • 23 years ago

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.

	esther
	Comment 78 • 23 years ago

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

	esther
	Comment 79 • 23 years ago

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

	esther
	Comment 80 • 23 years ago

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