av (gone) Assignee
	Description • 24 years ago

1. Make sure you don't have npqtplugin.dll (or any other plugin) in the Mozilla
Plugins folder
2. Type about:plugins in the URL bar, see that QuickTime time plugin is not listed
3. Copy the plugin over to the Plugins folder
4. Type the above commans in the URL bar
5. Type about:plugins in the URL bar

Expected result: QT plugin should be now listed
Actual result: QT plugin is still not there

	av (gone) Assignee
	Comment 2 • 24 years ago

Adding ekrock.

	Phil Schwartau
	Comment 3 • 24 years ago

Browser, not engine. Reassigning to DOM Level 0 for further triage - 

	av (gone) Assignee
	Comment 5 • 24 years ago

Reassingning to myself.

	av (gone) Assignee
	Comment 7 • 24 years ago

Johhny, would you please review this patch? This is only nsPluginArray::Refresh 
implementation.

	Johnny Stenback (:jst)
	Comment 8 • 24 years ago

Andrew, you call GetService(), shouldn't there be a matching ReleaseService()?
Other than that it looks good so far. r=jst for that part.

I tested what 4.x does if someone calls plugins.refresh(true) and it turns out
that only the page where the function is executed is reloaded, just in case you
were wondering.

	av (gone) Assignee
	Comment 9 • 24 years ago

> Andrew, you call GetService(), shouldn't there be a matching ReleaseService()?

Probably not. It is assigned to a member and released in the constructor.

	av (gone) Assignee
	Comment 10 • 24 years ago

Mmm... I meant 'destructor' indeed.

	Johnny Stenback (:jst)
	Comment 11 • 24 years ago

Oh, I see, I didn't look at the existing code a lot so I didn't realize that,
duh! The patch looks fine to me.

	vidur (gone)
	Comment 12 • 24 years ago

It seems like a null check for mPluginHost is enough since we get the service in
the constructor of this class. Otherwise sr=vidur.

	R.K.Aa.
	Comment 14 • 24 years ago

(making notes in public..)
following cycle: harpoon Linux dep leak up 1396B > 5108B

Two other checkins in cycle?
-sspitzer 12/21/2000 15:34 for bug 56074. Files:
/cvsroot/mozilla/modules/libpref/src/init/mailnews.js 3.82
/cvsroot/mozilla/mailnews/import/src/nsImportMail.cpp 1.27
/cvsroot/mozilla/mailnews/base/util/ nsMsgIncomingServer.cpp 1.94
/cvsroot/mozilla/mailnews/base/public/nsIMsgIncomingServer.idl 1.47
/cvsroot/mozilla/mailnews/base/prefs/resources/ content/ AccountManager.js 1.59

-attanasi "turn Style Context FastCache back on" No bug. Files:
/cvsroot/mozilla/layout/base/src/nsStyleContext.cpp 3.145
/cvsroot/mozilla/layout/base/src/nsStyleSet.cpp 3.78

	av (gone) Assignee
	Comment 16 • 24 years ago

Guys, please have a look and review this patch. I would like to get something 
checked in before I proceed any further and make it final.

This patch is functional meaning that after installing a plugin, issuing this 
javascript refresh command and returning to the page with the plugin it will 
work. Removing plugins works too. There are still things to do but I beleive 
they are more or less independent and can be just added. Those (what I know) 
are:
   - after refresh about:plugins does not show the refreshed list so I need to 
figure out how to hook this up
   - javascript:navigator.plugins.refresh(...) ingnores arguments and does not 
force page reload if it is 1. I think I have some understanding of this one.

	av (gone) Assignee
	Comment 19 • 24 years ago

Attachment 21887 [details] [diff] is a small and independent change in the DOM code. It addresses 
first issue in my 2001-01-02 14:41 comments. It can be checked in separately.

Johnny would you please review?

	sean echevarria
	Comment 21 • 24 years ago

Andrei: I've attached an updated version of your last patch.  There was a pre-
existing bug (bug 62590) in PluginArrayImpl::Refresh that caused a leak of the 
plugin host whenever you typed about:plugins - the kPluginManagerCID service is 
init'd in the PluginArrayImpl ctor.  hmmm - maybe my updated patch should just 
conditionalize the call to GetService rather than always ReleaseService before 
GetService...

	av (gone) Assignee
	Comment 24 • 24 years ago

This patch addresses both issues cited in my 2001-01-02 14:41 comments. It also 
incorporated latest seans suggestions, so it is a compilation of 21887, 21907, 
21908, and looks like this is everything we need to do in the DOM part.

	av (gone) Assignee
	Comment 26 • 24 years ago

Just if somebody needs everything in one attachment. This patch (22213) combines 
21602, 21705 (plugin part) and 22210 (DOM part) and thus is the complete patch 
to fix this bug.

Please review and superreview. Looks like we need to fix this asap.

	Peter Lubczynski
	Comment 27 • 24 years ago

The patch worked nicely in all the testcases I tried on the Mac except for one 
which I was able to cause a crash. I didn't even get a stack trace and it's an 
edge case anyway:

1) Open two windows with the same plugin, lets say Flash.
2) Remove the Shockwave-Flash plugin
3) Issue the javascript:navigator.plugins.refresh(true)
4) Crash!

Have you tried this on Windows? I think PluginSaftey may catch it but I can't 
test right now as my system state is set up for testing another patch.

	av (gone) Assignee
	Comment 28 • 24 years ago

I see this crash on Windows. There is no need to have two windows open. And it 
happens with any plugin after you remove it. I too cannot see any sensible stack 
trace, and safety wrapping does not help also. The only place I can blame now is 
stopping/destroying mechanism in nsPluginHost::ReloadPlugins. Any help here 
would be appreciated. I'll try Purify.

	av (gone) Assignee
	Comment 29 • 24 years ago

Although removing a plugin might seem not that important, this will probably 
cause problems when the user updates a plugin to a newer version. We should test 
this too.

	vidur (gone)
	Comment 30 • 24 years ago

Comments on av's last patch: the references from NavigatorImpl and PluginArray
to the docshell should be explicitly marked as weak references. The use of
mDocShell in PluginArray::Refresh() should be enclosed in a null check for
mDocShell. Other than that, sr=vidur.

	av (gone) Assignee
	Comment 32 • 24 years ago

Johnny, would you give me an 'a=' on 23002

	Johnny Stenback (:jst)
	Comment 34 • 24 years ago

Last patch looks good to me, r&a=jst

	av (gone) Assignee
	Comment 35 • 24 years ago

Attachment 23222 [details] [diff] checked into the trunk.

	av (gone) Assignee
	Comment 36 • 24 years ago

After investigating the crash referred to in my and peter's comments on 
2001-01-10 I found the following. If we have a page with a plugin and do refresh 
after we removed or updated the plugin dll we crash because we unload the dll 
library from the memory (when destroying the plugin list in ~nsPluginTag), while 
OS still tries to access it during page tear up, likely it sends messages to the 
window which is no longer associated with anything. Removing UnloadLibrary line 
helps to avoid the crash but imposes another problem: it does not pick up the 
fact that we updated the dll and shows the old one when we return to the page.

The solution is to postpone UnloadLibrary for running plugin dlls up till the 
moment when it is needed again. I introduced a flag mCanUnloadLibrary in 
nsPluginTag and do it or not in the destructor according to its value. The flag 
is set for running plugins when the refresh command comes. At the same moment we 
construct a special list of unloaded but due to be unloaded libraries which list 
lives until we hit any page with any plugin again. At this point the list, if 
any, is cleared and new dlls loaded as needed.

	Peter Lubczynski
	Comment 38 • 24 years ago

Haven't tried out the new patch yet, but with the currently applied patch, on 
Linux, it looks like about:plugins lists plugins twice after a 
navigator.plugins.refresh is sent. 

To reproduce on Linux, go to about:plugins
Issue a javascript:navigator.plugins.refresh(true)
Then notice twice as many entries

This could stem from another bug as before you issue the refresh command, there 
are already two copies of the default plugin, both pointing to the same share 
library file. 

This isn't very serious, but it's weird that it happens. 

Time to try out the new patch.

	av (gone) Assignee
	Comment 39 • 24 years ago

The new patch should be free of this weirdness.

	Peter Lubczynski
	Comment 40 • 24 years ago

Ok, I'll try the patch out on Linux in just a sec, but the Mac still has some 
problems. The following situation causes a crash. I'll use Flash as an example:

1) Visit a page that needs flash
2) You see the default plugin and
3) Install the plugin by dragging it to the plugins folder
4) Open a second browser window to the same page. Leave the first open, in the 
back.
5) Default plugin still visible
6) Do a javascript:navigator.plugin.refresh(true)
7) CRASH --> unmapped memory exception.

This is probably a very rare case, but I'll continue to do some more testing and 
post my results.

	av (gone) Assignee
	Comment 41 • 24 years ago

Reproducable on NT. Debugging...

	Peter Lubczynski
	Comment 42 • 24 years ago

I applied the new patch on my Mac and I still have the same problem as I 
mentioned in my 2001-01-10 15:17 comment.

I've actually made it a more general case in that the page that you do the 
javascript:navigator.plugins.refresh(true) doesn't have to have any plugin at 
all, there just needs to be one open in the background, in another window. 

Perhpas another way to fix this would be to check if there are any other 
DocShells (or WebShells) that are running this plugin and if so, don't 
unload/reload that plugin.

It's a rare case, but it is a crash. cc:ing Brian Nesse for his input. Brian, 
how did Nav 4.x handle this special case?

	av (gone) Assignee
	Comment 43 • 24 years ago

Please ignore my previous comment. It does _not_ crash on Windows. I was just 
using the wrong tree...

Peter, how it currently works -- we call nsINavigation::Reload method, and 
nsINavigation object is per DocShell, so it is not supposed to reload the second 
window. But something there still happens: I see the default plugin disappear 
but the newly installed plugin does not show up. Hitting Reload button brings it 
up. As to checking other DocShells, I was told that there is no available global 
way to do this. 

Would you try to debug it on Mac to see why it crashes?

	Peter Lubczynski
	Comment 44 • 24 years ago

It looks like the timer is firing on the Mac which is causing another event to 
try to be passed to the plugin through HandleEvent which then causes the crash 
because the plugin is shutdown.

I've seen similar behavior in other instances. HandleEvent needs more bullet-
proofing.

	Peter Lubczynski
	Comment 46 • 24 years ago

It turns out that it wasn't just timer events that were lingering after the 
plugin was shutdown, but others as well like PAINT/UPDATE. It turns out that 
fNPP.pdata is null when the plugin is shutdown so I have HandleEvent check and 
return of it's null which seems to have fixed the crash. I'm can't reproduce any 
of the crashes I was able to before. Works pretty good on Mac, still need some 
testing on other platforms. 

Andrei, do you forsee any side-effects in checking for a null fNPP.pdata like 
this?

	av (gone) Assignee
	Comment 47 • 24 years ago

Thanks, Peter. I am trying to verify it works on Unix now, and if it does will 
post the updated patch for (hopefully) final review.

	Chris Waterson
	Comment 49 • 24 years ago

Looks like there might be some nested C-style comments in
nsPluginHostImpl::AddInstanceToActiveList(); you should fix that. sr=waterson,
if peterl is ok with it.

	Peter Lubczynski
	Comment 50 • 24 years ago

r=peterl

	Peter Lubczynski
	Comment 51 • 24 years ago

I've found some more weird behavior with this bug, perhpas I don't have all the 
patches installed but maybe you can try it just in case:

1) Copy out your qt* (Quicktime) plugins to another folder. Leave the default 
plugin in.
2) Start Mozilla and browse to:
http://www.apple.com/trailers/fox/cast_away/trailerqt_320.html
3) Click on the icon and you should go to Apple's page.
4) Close that window (Apple's) and manually copy the qt* plugins back.
5) Now, notice the default plugin says to click after installing. Do that and 
nothing happens. Manually issue javascript:navigator.plugins.refresh(true) and 
also nothing happens. Still the default plugin.
6) Visit another page with Quicktime (let's say http://quicktime.apple.com) and 
it works.

This happened on Win2000 with a pretty fresh pull. Does the same happen for you?

	Peter Lubczynski
	Comment 52 • 24 years ago

Ahh...do a "Super Reload" and it works. Looks like it's comming from the cache. 
Can the Reload call force a trip back to the server?

	av (gone) Assignee
	Comment 53 • 24 years ago

It works fine to me on NT4.0.

	av (gone) Assignee
	Comment 55 • 24 years ago

This attachment is corrected for Chris' comments 23708 plus 24032 plus couple of 
tabs fixes. I am going to check it into the trunk today if nobody stops me.

	av (gone) Assignee
	Comment 56 • 24 years ago

The fix checked in. Marking fixed.