60064 - Crashes on "Plugin Downloader" dialog.

Status: VERIFIED FIXED

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

Description

[rpallath]

Invoke Browser.
Prompts for choosing Profile. Create a new Profile
Choose the same and invoke the browser.
Visit www.comdex.com
It will invoke a "Plugin Downloader Popup"
Click on the "OK" button twice/thrice/...
It will crash the browser.

Subsequent invokation however will go thru' smoothly.
No crashes but assertions wil be thrown.

Seems like this crash can be produced very consistently with  invokation of a 
new profile.
On second invokation on checking the .mozilla/<profile> dir. Found that the
only file that changes is localstore.rdf.
Wondering if it has anything  to do  with that.


Tried it on Solaris 2.7 (sparc).
The following was used for this build:
	1106 Commercial Build 
		[tag: -r Netscape_20000922_BRANCH -D "10/26/2000 10:00:00"
		 mozilla source patch: 57725.diffs
		 PSM: 1.3 & NSS 3.1 (NSS_3_1_RTM tag)
		 PSM jedimind trick: libpsmshim.so
		 Cool/AIM: sparc drop 08282000 ,intel drop 10092000
		 ns source patch for run-mozilla.sh ]
        1106 Commercial Blackwood Build
        	[tag:  -r JAVADEV_PR3_20001002 ]
	1024 Commercial Build gtk libs
	Java Plugin 1.3.0_01 FCS bits

Comment 1

[rpallath]

Updated Status Whiteboard

Comment 2

[Rich Burridge]

Attachment: Diffs to fix bug 60064 (from Sergei V. Lunegov).

Comment 3

[Rich Burridge]

Evaluation and fix from Sergei V. Lunegov <[e-mail]>

I found out reason of bug #60064:
on www.comdex.com there are 3 tags which point to flash movies
and for every tag new default plugin and new window
are created. 
Every created window is
a modal one. So instead of 3 windows
we can see only one. 

And for every tag method "makeWidget" is invoked 
in file modules/plugin/default/unix/nullplugin.c
Then invokation of appropriate methods is binded
by method gtk_signal_connect. Since for Cancel button
second argument is Gtk_Widget everything is fine.
And for 'OK' button second argument is PluginInstance.
I'm not expert in GTK and I think
the reason is we same button (the window is modal)
with various sources.

My suggestion is:
in function "void makeWidget(PluginInstance *This)"
(file modules/plugin/default/unix/nullplugin.c)
to set field 'exists' of plugin instance 'This' to 'TRUE'.
And in method "NPError NPP_SetWindow(NPP instance, NPWindow* window)"
(file modules/plugin/default/unix/npshell.c) to add
checking: "if(This->exists != TRUE)" 
and only if this statement is true to invoke function "makeWidget(This)".

Diffs for appropriate files are in attachment.

This fix also covers bug #60060

Comment 4

[Scott MacGregor]

I'd need av to approve this fix. I'm not at all familiar with this part of the
plugins code. Do we need to bring in a linux guru to review it as well?
blizzard/pavlov? 

Comment 5

[Rich Burridge]

mscott: I've added blizzard and pavlov to the cc:. Chris/Stuart, could 
you cast your eyes over the evaluation/diffs and comment please? Thanks.

Comment 6

[Christopher Blizzard (:blizzard)]

What happens if there are two plugins with different mime types on a single
page?  Does this patch handle that case?

Comment 7

[Rich Burridge]

Adding reply from Sergei, to blizzard's question. My feeling is we should
just let 60064 fix the crasher, and open another bug for the other problem.
Comments please.

Sergei V. Lunegov wrote:
"I thought about this problem.
This patch doesn't handle case when there
are several mime-types which require various plugins on one
page. Problem concerns only non-installed plugins.
If page contains several mime-types which require plugins
and these plugins are installed -- everything is OK.

The window which contains message with suggestion
to download plugin is a modal one. If page
contains several mime-types (and appropriate plugins
are not installed) only one window can be showed at every 
time moment. I mean since window is a modal
then it is impossible to show several such windows simultaneously.
This patch prevents browser from crashing
and allows to download first (order of tags in html page)
plugin
but it doesn't solve problem with several mime-types.

I didn't investigate this problems with other browsers (MS-IE)
and I don't know how do they act.

On my opinion there are 2 possibilities:
1) to file new bug with synopsys :"Mozilla doesn't allow to download
several plugins .." and to make refernce to bug 60064
2) to fix this problem in frames of bug 60064".
. 

Comment 8

[Rich Burridge]

Oops. I should learn to read all mt email before responding to things.
Here's a followup comment from Sergei:

"I sent all of you message about several mime-types on one page.

I was absolutely incorrect. In previous message were only my theories.

After I sent this message I decided to check up all my considerations

The result is: Mozilla works correct (!!!)
when several mime-types are on one page.
Browser shows several windows: one window for every mime-type
I just forgot that for every another mime-type
the new instance of PluginInstance is created.

In attachment there is testcase for this situation
Mozilla should show 2 dialog windows with suggestion
to download appropriate plugins
With patches for bug #60064 Mozilla doesn't crash."

Comment 9

[Rich Burridge]

Attachment: HTML page requesting two uninstalled plugins.

Comment 10

[Christopher Blizzard (:blizzard)]

OK  sr=blizzard

Comment 11

[av (gone)]

If 'a' is needed, a=av.

Comment 12

[av (gone)]

From Sergei Lunegov:

I made add-on fix.
This fix contains only one line -- 'This->exists = FALSE;' this line was added 
to function : 

NPError
NPP_New(NPMIMEType pluginType,
    NPP instance,
    uint16 mode,
    int16 argc,
    char* argn[],
    char* argv[],
    NPSavedData* saved)

I added this line to initialize field 'This->exists'. Earlier field This->exist' 
initialized in method NPP_SetWindow and this wasn't correct because this field 
might be changed in other parts of program and the changed value should be kept. 
Now the initialization of field 'This->exists' is made during creating of 
instance of structure PluginInstance (during creating 'This').

Comment 13

[av (gone)]

Here would be the diffs assuming that the previous patch has already been 
applied:

Index: npshell.c
===================================================================
RCS file: /cvsroot/mozilla/modules/plugin/default/unix/npshell.c,v
retrieving revision 1.8
diff -u -r1.8 npshell.c
--- npshell.c   2000/11/30 21:32:07     1.8
+++ npshell.c   2000/12/08 21:49:39
@@ -124,6 +124,7 @@
     This->type = dupMimeType(pluginType);
     This->instance = instance;
     This->pluginsPageUrl = NULL;
+    This->exists = FALSE;

     /* Parse argument list passed to plugin instance */
     /* We are interested in these arguments

Comment 14

[av (gone)]

r=av. Also note that the previous patch (id=10617) is already in.

Comment 15

[Rich Burridge]

Thanks Andrew. Can you check into the trunk with this, or do we
need a super-reviewer? If the latter, who do you recommend?

Comment 16

[Christopher Blizzard (:blizzard)]

sr=blizzard

Comment 17

[av (gone)]

Checked in.

Comment 18

[Margaret Chan]

Checked in fixes to the OEM branch.  a=jdunn

Comment 19

[edburns]

If this fix is checked in, why is the bug still NEW?

Comment 20

[Rich Burridge]

An oversight?

Comment 21

[drapeau]

edburns: thank you very much for catching the "new" vs. "fix checked in" status.
 I wish somebody were this vigilant with all Bugzilla bugs; if so, maybe the
total bug count would be less than 11000+.

Comment 22

[shrirang khanzode]

no longer crashes with linux trunk 0201 build. Marking VERIF.