44146 - improperly parented modal dialogs in wallet.cpp

Status: VERIFIED FIXED

(Core :: Networking: Cookies, defect, P3)

Description

[Dan M]

nsNetSupportDialog should only be used as a backup plan if no other nsIPrompt 
interface is available. It has been used in dozens of places because of its 
seductive convenience. But it's flawed, creating modal windows that don't behave 
correctly; the cause of various blanket bugs like 25684 and 39439 (both currently 
considered nsbeta2+).
  This problem can only be fixed by trying much harder to find a proper window to 
be the modal dialog's parent. nsNetSupportDialog should be relegated to providing 
backup when herculean efforts to locate the actual parent window fail for some 
reason.
  As an example, the cookie service has been taught to use a proper parent window 
for its dialog by laboriously storing a reference to that window's nsIPrompt in 
nsHTTPChannel, from which it can be extracted and passed around while processing 
notification events, punting to nsNetSupportDialog only when no other choice is 
available. That same sort of thing needs to be done in many more places.
  There are six such places in wallet.cpp, all similar: Wallet_Confirm, 
Wallet_ConfirmYN, Wallet_3ButtonConfirm, Wallet_Alert, Wallet_CheckConfirmYN and 
WLLT_OnSubmit. The mysterious case of Wallet_Confirm at least is already covered 
by bug 44145. The others are probably not so lucky.
  Happy recipients of this bug would spread the happiness most widely if they 
would start using a good nsIPrompt window. The "modal windows don't behave 
nicely" bugs are being made dependent on this bug and its siblings, and will 
eventually be closed as "as fixed as they're going to get" once these have all 
been considered.

Comment 1

[Stephen P. Morse]

This should be nsbeta2+ since it blocks bug 25684 which is already nsbeta2+.

Comment 2

[Stephen P. Morse]

In 4.x it was easy for an app to put up a dialog.  It simply called FE_Confirm 
or FE_Alert and didn't have to worry about parent windows.  Under the current 
architecture that is no longer the case and it appears that the app has to do a 
lot of soul-searching in order to put up simple dialogs.  That's a big burden to 
put on the app writer and will certainly hamper our ability to quickly implement 
new apps.

With that being said, let me see what I can do about the particular instances in 
wallet and single signon of dialogs with no parent windows.

Reference To Bug 44145
----------------------

Wallet_Confirm as used in wallet.cpp and signsign.cpp have nothing to do with 
the Wallet_Confirm that bug 44145 are referring to.  So ignore the comment above 
to that bug.

Wallet_Confirm
--------------

Their is one call to Wallet_Confirm in wallet.cpp.  That is in WLLT_DeleteAll 
and is coming from the menu clicks in taskOverlay menu.  I'm not sure how to get 
a parent window for that.

Wallet_ConfirmYN
----------------

Their is one call to Wallet_ConfirmYN and that is coming from si_OkToSave in 
singsign.cpp.  Fortunately that is bracketed by #ifdef DefaultIsOff, a case that 
no longer occurs (the default for single signon has long since been agreed upon 
to be on).  So this call is never made.

Wallet_3ButtonConfirm
---------------------

Wallet_3ButtonConfirm is called once in wallet.cpp and once in signsign.cpp.  In 
both cases they occur just after an analysis has been made of the form being 
displayed and it is determined that it a candidate to be saved.  The dialog then 
asks the user if the information on the form should be saved.  What window 
should I use as a parent for those dialogs?

Wallet_Alert
------------

There are seven calls to Wallet_Alert.

One is in Wallet_OnSubmit is bracketed by #ifndef autocapture and since this 
will always be defined we don't need to worry about it.

One is in EncryptString which is called indirectly from Wallet_Encrypt which is 
a public upi and can be called from anywhere.  I'm not sure what to do about 
that one?

Five others (WLLT_ExpirePassword, WLLT_ChangePassword, wallet_ReencryptAll, 
WLLT_Prefill, WLLT_RequestToCapture) are called from menu clicks in the 
tasksOverlay menu so once I know how to handle Wallet_Confirm (see above), I'll 
know what to do about these five as well.

Wallet_CheckConfirmYN
---------------------

This is no longer being called so we can forget about it.

Wallet_OnSubmit
---------------

Wallet_OnSubmit is not a separate dialog -- it does put up a Wallet_Alert dialog 
and this is covered in the Wallet_Alert section.

Comment 3

[leger]

Putting on [nsbeta2+] radar for beta2 fix.

Comment 4

[Stephen P. Morse]

Just as a note to myself, I now have fixes in hand for almost all the dialog 
occurences.  The ones I still need to investigate are:

Wallet_3ButtonConfirm
---------------------

Wallet_3ButtonConfirm is called once in wallet.cpp and once in signsign.cpp.  In 
both cases they occur just after an analysis has been made of the form being 
displayed and it is determined that it a candidate to be saved.  The dialog then 
asks the user if the information on the form should be saved.

Wallet_Alert
------------

There are two remaining calls to Wallet_Alert.

One is in EncryptString which is called indirectly from Wallet_Encrypt which is 
a public upi and can be called from anywhere.  The purpose of this is to put up 
the legal disclaimer the very first time that data obscuring is being done.

One is in wallet_ReencryptAll.  It is in a callback routine that gets 
executed whenever the pref is changed from encryption to obscurring or vice 
versa.  The dialog occurs only if a failure was reported from the PSM module.

Comment 5

[Stephen P. Morse]

Fixes mentioned above have been checked in.  So only the enumerated cases above 
remain as problems.

Comment 6

[Stephen P. Morse]

Have fixes in hand for Wallet_3ButtonConfirm dialogs.  Awaiting review.

Comment 7

[Stephen P. Morse]

Above fix checked in.  Dialogs still not fixed are the two that use 
Wallet_Alert, namely:

One in EncryptString which is called indirectly from Wallet_Encrypt which is 
a public upi and can be called from anywhere.  The purpose of this is to put up 
the legal disclaimer the very first time that data obscuring is being done.

One in wallet_ReencryptAll.  It is in a callback routine that gets 
executed whenever the pref is changed from encryption to obscurring or vice 
versa.  The dialog occurs only if a failure was reported from the PSM module.

Comment 8

[Stephen P. Morse]

Fix in hand for the legal disclaimer dialog.  Waiting code review from dveditz.

Comment 9

[Stephen P. Morse]

Fix mentioned above checked in.

Now there is only one silly dialog remaining.  It pops up if the user specifies 
that he wants to change the encoding from obscurred to encrypted or vice versa 
and there was a failure reported back from the psm module.  In that case the 
dialog says "Data not converted."  This occurs on a callback routine that is 
invoked when the encryption pref is changed.  So it is not clear to me how to 
get a parent window in that case.

The dialog call is currently commented out (with //??? so I can locate it 
easily) in wallet.cpp.  I'll do a little more investigating to see if I can fix 
this last dialog easily.

Comment 10

[Stephen P. Morse]

Have fix for last dialogs in hand.  It's been reviewed and will get checked in 
as soon as the tree opens today.  I'll close out this report as soon as I get 
the fix checked in.

Comment 11

[Stephen P. Morse]

Fix checked in.  All dialogs in wallet/single-signon now have parent windows.  
Closing out this bug report (at last!@).

Comment 12

[Tom Everingham]

verified:
WinNT 2000072108
Linux 2000072020
Mac OS8.6 2000072108