Closed Bug 44146 Opened 25 years ago Closed 25 years ago

improperly parented modal dialogs in wallet.cpp

Categories

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

Product:
Core
Component:
Networking: Cookies
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: danm.moz, Assigned: morse)

References

Details

(Whiteboard: [nsbeta2+] ETA 7/17)

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.
Blocks: 25684
This should be nsbeta2+ since it blocks bug 25684 which is already nsbeta2+.
Status: NEW → ASSIGNED
Keywords: nsbeta2
Target Milestone: --- → M18
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.
Putting on [nsbeta2+] radar for beta2 fix.
Whiteboard: [nsbeta2+]
Whiteboard: [nsbeta2+] → [nsbeta2+] ETA 7/17
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.
Fixes mentioned above have been checked in. So only the enumerated cases above remain as problems.
Have fixes in hand for Wallet_3ButtonConfirm dialogs. Awaiting review.
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.
Fix in hand for the legal disclaimer dialog. Waiting code review from dveditz.
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.
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.
Fix checked in. All dialogs in wallet/single-signon now have parent windows. Closing out this bug report (at last!@).
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
verified: WinNT 2000072108 Linux 2000072020 Mac OS8.6 2000072108
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.