92301 - data in password manager dialog vanishes

Status: VERIFIED FIXED

(SeaMonkey :: Passwords & Permissions, defect)

Description

[Henrik Gemal]

to reproduce:
1) open two browser windows
2) in the first window select Tasks -> Privacy and Security -> Password 
Manager -> View Stored Password
3) While having the stored password dialog still showed, switch to the other 
browser window and select Tasks -> Privacy and Security -> Password Manager -> 
View Stored Password

capow: no passwords are showed. The console says:

Error: window.arguments has no properties
Source File: chrome://communicator/content/wallet/SignonViewer.js
Line: 87

Comment 1

[Stephen P. Morse]

Truly an interesting bug.

Major issue here is the loss of data in the open password-manager window -- the 
js error is minor by comparison.  Changing summary line to reflect that.

Comment 2

[Henrik Gemal]

the passwords are not really lost. If you just press Cancel in the all the 
dialogs and the start the Password Manager again the passwords are still 
there....

Comment 3

[Terri Preston]

Hmm, did you notice that it also brings up the form manager tabs as well? (no 
info in there either)  Different bug morse?

Comment 4

[Stephen P. Morse]

Yes, I was aware of the fact that it doesn't vanish completely.  But it does 
vanish from the screen and that's what my wording implied.

Yes I realize the problem with the form manager sites as well.  I understand the 
problem and have a fix which I'll post in a few minutes.

Comment 5

[Stephen P. Morse]

Attachment: corrects this problem and other related ones

Comment 6

[Stephen P. Morse]

The problem is being caused by both browser windows attempting to use the same 
dialog window for the password-manager dialog.  And that's because they both 
have the same name in the call to openDialog.

The fix is to use no name at all for that parameter, or to use the special name 
_blank.  I noticed other uses of openDialog that I had and some used no name 
whereas others used _blank.  So, for consistency, I made them all use _blank in 
the patch that I just posted.

Furthermore, the cookie tutorial gets opened in a modal window when there is no 
need for it to be modal.  This causes several instances of it to appear if 
opened in more than one browser window.  I fixed this up as well -- this time 
adding an explicit name for the parameter in openDialog.

cc'ing vishy and blake for reviews.

Comment 7

[Viswanath Ramachandran]

r=vishy

Comment 8

[Blake Ross]

Still, why is this the correct fix? It seems like there's a problem on the
dialog side as well.  It may just be that that window.arguments error is causing
execution to stop, meaning the list won't be populated.  I don't understand why
you should be allowed to open more than one of these windows, and it seems like
this fix is going to allow that (?)

Comment 9

[Stephen P. Morse]

> It may just be that that window.arguments error is causing execution to
> stop, meaning the list won't be populated.

The arguments error doesn't cause execution to stop but it does cause the 
Startup() method to exit without initializing the date to either password 
manager (as would be the case if argument was "S") or form manager (argument of 
"W").  And in that case the list doesn't get populated as you said, and the 
users sees this as the list vanishing before his eyes.  So I think we are saying 
the same thing, and it is completely understandable why the list goes blank.

> I don't understand whyyou should be allowed to open more than one of these 
> windows, and it seems like this fix is going to allow that (?)

Yes it will allow that.  But don't forget that the dialog in question here is 
modal.  So if we allow two browser windows to open up a common modal dialog, 
what does that mean?  It can't be modal to two browser windows simultaneously.  
That's why I chose to open a separate dialog for each browser window.

> Still, why is this the correct fix?

What do you propose that the correct fix should be?

Comment 10

[Henrik Gemal]

being able to have two instances of the same dialog is gonna create confusen. 
Stuff like: "When I delete a site in the first dialog it still shows up the 
second dialog" ....

Comment 11

[Stephen P. Morse]

What's the alternative?  Having all browser windows freeze when the modal dialog 
is opened?

I'll admit that the solution presented here is not optimal but I don't know how 
else to handle it.  However I think that it's not something that we need to be 
overly concerned with since it's a rare occurence that the user would open such 
a dialog in more than one browser window simultaneously.

Even opening the dialog in one browser window and continuing to process in the 
second window will cause some confusion because the dialog does not get updated 
as he does things like add sites from the second browser window.  And, worse 
yet, the user can open the pref panel (which has recently become non-modal), 
from it open the password-manager dialog which is now modal to the pref panel, 
and while it is open he can continue to browse in the browser window.

If the examples I just cited are of concern to you (and they probably should 
be), then open a separate bug report to track them.  But I would considered them 
less important than the symptoms described in this report (contents of a dialog 
vanish in front of the user) and that is what I am trying to fix with the 
current patch.

Comment 12

[Blake Ross]

I think ultimately the correct fix is for the dialog not to be modal, and for
each entry point to just focus the dialog if it's already open.  sr=blake
because this is better than nothing, and it's what the cookie manager seems to
do currently.

Comment 13

[Stephen P. Morse]

Fix checked in

Comment 14

[Terri Preston]

Verified fixed on w98 build 2001-09-17-05-0.9.4, (On linux build 
2001-09-17-05-0.9.4, you can't use the drop down menus while this dialog is open 
so I believe it works on linux) (Also on Mac OS X build 2001-09-17-04-0.9.4 I 
can't switch windows while this dialog is open)  I believe these are all 
workable situations, therefore marking verified