44145 - improperly parented modal dialog in psm-glue

Status: VERIFIED FIXED

(Core :: Security, 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 three such places in nsFSDR.cpp: Wallet_Confirm, wallet_GetString and 
wallet_getDoubleString. These seem to all track back to nsFSecretDecoderRing 
functions, which of course take no nsIPrompt parameter and appear to be called 
from hundreds of places. Woohoo.
  There's one more use in nsPSMUICallbacks.cpp, PromptUserCallback(); another 
function that ends up effectively being called in a zillion places throughout 
psm-glue.
  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.
  I'm not quite sure who to assign this to; dougt, I think. But thayes gets first 
bat since Doug is on leave at the moment.

Comment 1

[Stephen P. Morse]

nsFSDR is a tempory module that was to be used until the real nsSDR module was 
ready for prime time (the F means False).  Terry switched in the real module a 
couple of weeks ago.  So if his new module stays in the build, we will not have 
to worry about the problems in nsFSDR.  However, if for some reason we find that 
we can't release the beta with the real nsSDR, we might have to use nsFSDR as a 
fallback in the beta2 release.

So, optimistically, the only case we need to be concerned about is 
PromptUserCallback in nsPSMUICallbacks.cpp.

Comment 2

[Stephen P. Morse]

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

Comment 3

[Jay Patel [:jay]]

Putting on [nsbeta2-] radar. Not critical to beta2. 

Comment 4

[Doug Turner (:dougt)]

Because of the use of nsCommonDialogs, or the use of the nsIPrompt service, this 
component can not be used for embedding.  Adding the embedding keyword.



How To Bring Up A Modal Dialog: 

You need to know what window you want to have modality against.  This will be a 
nsIDOMWindow.  Using a magic "hidden" window breaks modality and embedding 
application may not have this hack.  One you have the parent DOMWindow, you 
simply: 
  

nsCOMPtr<nsIPrompt> prompter; 
aDOMWinow->GetPrompt(getter_AddRefs(prompter)); 
if (prompter) 
    prompter-> 
  

Any other way that you try to bring up a dialog may not work in an embedding 
application. To reiterate, do not use the nsICommonDialogs interface -or- a
nsIPrompt service if you want your component in a non-seamonkey app. 
  

I don't have a DOMWindow?! 

If you don't have a DOMWindow, you need to get one.  Just about every place I 
saw that used the hidden window, could have used the real parent window with
some work.  There really is no place in the code where we should be displaying a 
modal dialog without knowing what parent window we are modal against.  If you 
find a case where you don't have a top level window, lets talk. 

Comment 5

[Stephen P. Morse]

The problem is more than just not being embedable.  The use of the hidden window 
will lead to a crash if you close down the rest of the browser before closing 
these modal dialogs.

I'd like to help out here.  I just looked over the code in nsPSMUICallbacks.cpp.  
Specifically in the routine DisplayURI which is where the dialog gets put up.  
The code is:

  rv = appShell->GetHiddenWindowAndJSContext( getter_AddRefs( hiddenWindow ),
                                                    &jsContext );
  ...
  // open the window
  nsIDOMWindow	*newWindow;
  hiddenWindow->Open(jsContext, argv, 3, &newWindow);

I'm already experienced at finding parent windows, having fixed all the places 
in wallet.cpp and singsign.cpp where such was needed.  But I don't see how to 
get a parent window in this case.  OK, doug, I'm taking you up on your offer.  
"LET'S TALK."

Comment 6

[Stephen P. Morse]

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

Comment 7

[Doug Turner (:dougt)]

I fixed this for beta3. (your welcome terry :-))

Comment 8

[Terry Hayes]

Thank you Doug.  So where does the parent window come from (or how do you 
discover it?)

Comment 9

[John Unruh]

Verified per dougt's comments.