44169 - improperly parented modal dialog in nsPluginHostImpl.cpp

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P4)

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.
  One such place is in nsPluginHostImpl.cpp, DisplayNoDefaultPluginDialog().
  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

[av (gone)]

cc'ing to Ed Burns.

Comment 2

[edburns]

I'll take this.

I think this isn't a bug because the user should have the default plugin
installed, which prevents this dialog from coming up.  The default plugin ships
with the product.  I'm glad it's annoying because it puts pressure on getting
the default plugin there for all platforms.

Comment 3

[edburns]

accept

Comment 4

[edburns]

Dan, since this particular instance of "modal windows don't behave nicely" is
only something that will come up if the user removes the default plugin from
their directory, can we assign this to a more pertinent instance of the
problem?  

Comment 5

[Dan M]

  I can't think of a more pertinent area; the struggle is to get the right window 
to be the parent for the modal dialog. People familiar with the specifics of the 
code the dialog comes from have the strongest chance of doing that.
  On the other hand, the serious underlying problem we're actually after is to 
prevent the crashes, window ordering problems and other entertaining things that 
happen when the user interacts with the modal dialog and its parent in unexpected 
ways.
  I'm not familiar with this particular bit of code. But it sounds like it may be 
a case where a modal dialog legitimately belongs to no parent. If that's true, 
and especially if the user can't make bad things happening by doing unexpected 
things, then I'd close this bug as invalid.

Comment 6

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

[edburns]

Moving to P4, since the "no default plugin window" doesn't show up normally.

Comment 8

[karnaze (gone)]

Moving to m1.0.

Comment 9

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 10

[Asa Dotzler [:asa]]

don't move bugs that are in the 1.0 dependency tree. sorry.

Comment 11

[Denis Antrushin]

nsPluginHostImpl now uses nsIPrompt it got from
nsIWindowWatcher:
  nsCOMPtr<nsIPrompt> prompt;
  nsCOMPtr<nsIWindowWatcher>  wwatch(do_GetService("@mozilla.org/embedcomp/window-watcher;1"));
  if (wwatch)
    wwatch->GetNewPrompter(0, getter_AddRefs(prompt));

danm, is it correct? If so, we can close this bug

Comment 12

[Denis Antrushin]

Reassign to default onwer to get attention :-)

Comment 13

[rubydoo123]

I pinged danm to take a look comment #11 and to say whether he agrees or not.

Comment 14

[Dan M]

Yes, it's pretty much fixed. The nsNetSupportDialog code has been replaced with
the PromptService, which is the important thing. It would be better if a parent
DOM window had been explicitly supplied, but it should function alright with a
null parent. (Does the plugin have access to a suitable parent window? If so,
things would be happiest if it were used to parent the prompt window.)

Comment 15

[rubydoo123]

spreading the wealth of bugs, assigning to serge.

Comment 16

[Denis Antrushin]

I didn't find any way to get DOMWindow.
I only able to get nsIDocument, nsIDOMElement and probably nsIDOMDocument
(using reinterpret casts :-( ) 
Is it possibile to get DOMWindow from any of these interfaces?

Comment 17

[serge (gone)]

Attachment: patch v1

possible fix, danm could you review, please?

Comment 18

[Dan M]

Yes, that's the stuff. But where does aOwner come from? Does it need null checks?

Comment 19

[serge (gone)]

woops, my bad, that's the problem when you're trying to get the patches on the 
same tree for different bugs;(
the right patch is following.

Comment 20

[Denis Antrushin]

Attachment: patch

Does getting plugin owner from an active running plugin (if any) is correct
way?

Comment 21

[serge (gone)]

yes, getting plugin owner from an active running plugin doesn't look good:(
we'll bring up a modal dialog against wrong parent window, the window which has 
an active plugin.

Comment 22

[serge (gone)]

maybe this will help
nsresult nsPluginHostImpl:: GetPrompt(nsIPrompt **aPrompt)
{
  nsCOMPtr<nsIPrompt> prompt;
  nsCOMPtr<nsIWindowWatcher> 
    wwatch(do_GetService("@mozilla.org/embedcomp/window-watcher;1"));
  if (!wwatch)
    return NS_ERROR_FAILURE;

  nsCOMPtr<nsIDOMWindow> domWindow;
  wwatch->GetWindowByName(NS_LITERAL_STRING("_content").get(), nsnull, 
    getter_AddRefs(domWindow));
  wwatch->GetNewPrompter(domWindow, getter_AddRefs(prompt));

  NS_IF_ADDREF(*aPrompt = prompt);
  return NS_OK;
}

Comment 23

[serge (gone)]

it doesn't help:( 
investigating...

Comment 24

[serge (gone)]

Attachment: test case

default plugin sould be removed to call into DisplayNoDefaultPluginDialog()

Comment 25

[rubydoo123]

added ADT3

Comment 26

[rubydoo123]

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

Comment 27

[serge (gone)]

Attachment: patch v1.2

Thanks Dedis, this is an expansion of your code.

Comment 28

[serge (gone)]

av, could you take a look on the patch v1.2, please?

Comment 29

[Denis Antrushin]

Only few lines looks familiar to me, all credits is yours, serge :-)

Comment 30

[Denis Antrushin]

See also somewhat related bug #51679 (Call to SetCookieString needs to pass in
an nsIPrompt)

Comment 31

[av (gone)]

1. Does this patch include some other fixes not related to this bug? Looks like
big-pdf-problem fix is also here.

2. Is there any reason that you created a new method and new macro rather than
replace old ones?

In all other aspects look good to me.

Comment 32

[serge (gone)]

thanks av.
1. yea, my fault, I'll remove unrelated code.
2. risk to change nsIPluginHost was my only concern, 
but this is not a final patch, I'll investigate the risk and update the patch.

Comment 33

[av (gone)]

Comment on attachment 78807 [details] [diff] [review]
patch v1.2

With #1 addressed r=av. I would prefer the method be replaced, but will be
happy with current version too.

Comment 34

[serge (gone)]

Attachment: patch v1.3 addressed av's comments 1 & 2

I've checked it out, there is no risk to add one more parameter to
nsIPluginHost::HandleBadPlugin()
av could you r= on this? thanks.

Comment 35

[av (gone)]

Comment on attachment 79560 [details] [diff] [review]
patch v1.3 addressed av's comments 1 & 2

r=av

Comment 36

[serge (gone)]

Patrick, could you sr= for this, please?

Comment 37

[Patrick C. Beard]

Comment on attachment 79560 [details] [diff] [review]
patch v1.3 addressed av's comments 1 & 2

Why comment out the code at label EXIT_DNDPD:, just delete it.
nsPluginHostImpl:: GetPrompt() always returns NS_OK, why not have it return an
error if the nsIPrompt out parameter can't be allocated?


+      nsCOMPtr<nsIPrompt> prompt;
+      GetPrompt(aOwner, getter_AddRefs(prompt));
+      DisplayNoDefaultPluginDialog(aMimeType, prompt);

What happens here is prompt is NULL?

Comment 38

[serge (gone)]

Attachment: patch v1.4 with Patrick's concerns addressed

1. label EXIT_DNDPD is deleted;
2. nsPluginHostImpl:: GetPrompt() returns correct error code;
3.
>+	nsCOMPtr<nsIPrompt> prompt;
>+	GetPrompt(aOwner, getter_AddRefs(prompt));
>+	DisplayNoDefaultPluginDialog(aMimeType, prompt);
>
>What happens here is prompt is NULL?

I've checked prompt in 
+void DisplayNoDefaultPluginDialog(const char *mimeType, nsIPrompt *prompt)
 {
   nsresult rv;
+  nsCOMPtr<nsIPref> prefs(do_GetService(kPrefServiceCID));
+
+  if (!prefs || !prompt)
+    return;
---
Please review.

Comment 39

[Peter Lubczynski]

adding forgotten nsbeta1+ to match topembed+

Comment 40

[serge (gone)]

Comment on attachment 80640 [details] [diff] [review]
patch v1.4 with Patrick's concerns addressed

carry over r=av

Comment 41

[Denis Antrushin]

removing myself from cc:

Comment 42

[Patrick C. Beard]

Comment on attachment 80640 [details] [diff] [review]
patch v1.4 with Patrick's concerns addressed

Below

@@ -296,32 +307,17 @@

you are clobbering the rv values that are returned from the calls to
GetStringFromName, for titleUni and messageUni. There should be NS_FAILED(rv)
checks for those too. Same problem near

@@ -6116,28 +6143,16 @@

Below

@@ -6050,10 +6064,13 @@

+  rv = cookieService->SetCookieString(uriIn, prompt, cookie,0);

What happens if GetPrompt() fails before the call to SetCookieString()? If you
pass nsnull as you were before, does it crash? With those fixes, sr=beard.

Comment 43

[rubydoo123]

moving to 1.0.1

Comment 44

[Jaime Rodriguez, Jr.]

Removing "+" from [ADT2 RTM]. Pls do not add information to impact markings in
the Status Whiteboard, unless directed to do so by the ADT. Correct ADT markings
are [ADT1 RTM], [ADT2 RTM], [ADT3 RTM], any others will effect our tracking. thanks!

Pls land this on the trunk, add ETA's, and have QA verify the bug before
nominating as adt1.0.0 (see
http://maya.mcom.com/MachV/project_wide/bug_triage/MachV_Nomination.html#adt for
more information).

Comment 45

[serge (gone)]

Attachment: patch v1.5 no clobbering return val from GetStringFromName()

>What happens if GetPrompt() fails before the call to SetCookieString()? If you

>pass nsnull as you were before, does it crash?
I do not know the answer, I haven't find any appropriate test case to get at
that point:( At least we pass null from beginning and I've seen no bugs related
to this code.

Comment 46

[serge (gone)]

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

Comment 47

[av (gone)]

Comment on attachment 85535 [details] [diff] [review]
patch v1.5 no clobbering return val from GetStringFromName()

Patrick, nsnull is currently hardcoded in this call and as serge said there is
no known problems associated with that. r=av

Comment 48

[rubydoo123]

mass move PL RTM 

Comment 49

[Jaime Rodriguez, Jr.]

Mass removing adt1.0.0, and adding adt1.0.1 because, we are now on 1.0.1.

Comment 50

[Patrick C. Beard]

Comment on attachment 85535 [details] [diff] [review]
patch v1.5 no clobbering return val from GetStringFromName()

Please remove this empty block:


+    if (NS_SUCCEEDED(rv)) {
+    } 

Other than that, sr=beard

Comment 51

[serge (gone)]

on the trunk
/mozilla/modules/plugin/base/src/ns4xPlugin.cpp,v  <--  ns4xPlugin.cpp
new revision: 1.88; previous revision: 1.87
mozilla/modules/plugin/base/src/ns4xPluginInstance.cpp,v  <--  ns4xPlug
inInstance.cpp
new revision: 1.92; previous revision: 1.91
mozilla/modules/plugin/base/src/nsIPluginHost.h,v  <--  nsIPluginHost.h
new revision: 1.31; previous revision: 1.30
mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp,v  <--  nsPluginHostImpl.cpp
new revision: 1.392; previous revision: 1.391
mozilla/modules/plugin/base/src/nsPluginHostImpl.h,v  <--  nsPluginHostImpl.h
new revision: 1.79; previous revision: 1.78

Comment 52

[Jaime Rodriguez, Jr.]

shrir - can you pls verify this one on the trunk? thanks!

Comment 53

[serge (gone)]

shrir: on win32 delete npnul32.dll from plugins folder that you can use the test
case. http://bugzilla.mozilla.org/attachment.cgi?id=75516&action=view .
w/o the patch browser can crash on closing no default plugin dialog.

Comment 54

[shrirang khanzode]

Thx,serge. Verif on 0607 trunk, looks good.

Comment 55

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) approval for checkin on the 1.0 branch. pls check
this in asap, then add the keyword "fixed1.0.1".

Comment 56

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 57

[serge (gone)]

checked in on MOZILLA_1_0_BRANCH

Comment 58

[shrirang khanzode]

verified on 0723 branch, testcase does not crash