61388 - javascript:navigator.plugins.refresh(true) does not work

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[av (gone)]

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

Comment 1

[av (gone)]

Attachment: QuickTime Plugin

Comment 2

[av (gone)]

Adding ekrock.

Comment 3

[Phil Schwartau]

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

Comment 4

[Phil Schwartau]

Attachment: An HTML file showing your plugins; may be of some use -

Comment 5

[av (gone)]

Reassingning to myself.

Comment 6

[av (gone)]

Attachment: First step -- to hook plugin stuff to DOM refreshing

Comment 7

[av (gone)]

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

Comment 8

[Johnny Stenback (:jst)]

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.

Comment 9

[av (gone)]

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

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

Comment 10

[av (gone)]

Mmm... I meant 'destructor' indeed.

Comment 11

[Johnny Stenback (:jst)]

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.

Comment 12

[vidur (gone)]

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

Comment 13

[av (gone)]

Attachment: corrected attachment

Comment 14

[R.K.Aa.]

(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

Comment 15

[av (gone)]

Attachment: still partial but already functional fix

Comment 16

[av (gone)]

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.

Comment 17

[av (gone)]

Attachment: forgot to include a header file to the previous patch

Comment 18

[av (gone)]

Attachment: making DOM refresh its own list

Comment 19

[av (gone)]

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?

Comment 20

[sean echevarria]

Attachment: update to the patch av provided

Comment 21

[sean echevarria]

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

Comment 22

[sean echevarria]

Attachment: patch per my last comment

Comment 23

[av (gone)]

Attachment: making DOM reload page when asked

Comment 24

[av (gone)]

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.

Comment 25

[av (gone)]

Attachment: the whole thing

Comment 26

[av (gone)]

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.

Comment 27

[Peter Lubczynski]

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.

Comment 28

[av (gone)]

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.

Comment 29

[av (gone)]

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.

Comment 30

[vidur (gone)]

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.

Comment 31

[av (gone)]

Attachment: Updated DOM part fix as per vidur's comment.

Comment 32

[av (gone)]

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

Comment 33

[av (gone)]

Attachment: Some Johnny's suggestions incorporated to the latest patch

Comment 34

[Johnny Stenback (:jst)]

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

Comment 35

[av (gone)]

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

Comment 36

[av (gone)]

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.

Comment 37

[av (gone)]

Attachment: Changes to the plugin module code reflecting my previous comment

Comment 38

[Peter Lubczynski]

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.

Comment 39

[av (gone)]

The new patch should be free of this weirdness.

Comment 40

[Peter Lubczynski]

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.

Comment 41

[av (gone)]

Reproducable on NT. Debugging...

Comment 42

[Peter Lubczynski]

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?

Comment 43

[av (gone)]

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?

Comment 44

[Peter Lubczynski]

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.

Comment 45

[Peter Lubczynski]

Attachment: patch to fix crash on Mac

Comment 46

[Peter Lubczynski]

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?

Comment 47

[av (gone)]

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.

Comment 48

[Peter Lubczynski]

Attachment: better patch (ignore previous) which prevents crash in another spot too

Comment 49

[Chris Waterson]

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.

Comment 50

[Peter Lubczynski]

r=peterl

Comment 51

[Peter Lubczynski]

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?

Comment 52

[Peter Lubczynski]

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?

Comment 53

[av (gone)]

It works fine to me on NT4.0.

Comment 54

[av (gone)]

Attachment: final candidate

Comment 55

[av (gone)]

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.

Comment 56

[av (gone)]

The fix checked in. Marking fixed.