143178 - Some plugin installers don't refresh plugins

Status: VERIFIED FIXED

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

Description

[av (gone)]

Sometimes after a new plugin is installed it is not immediately picked up
because the plugin list is not refreshed if installer doesn't call
plugins.refresh. We can still cure the situation if we call refresh in our code
on the pages with unknown plugins.

Comment 1

[av (gone)]

I guess the following steps would reproduce the problem

1. go to a page which requires a plugin you don't have, see the default plugin
2. cancel any get-plugin windows, if they pop up
3. install the needed plugin dll by hand, just copying it to the plugins folder
4. visit the page again (or reload it)5. see the default plugin again, not the
freshly installed one

Comment 2

[rubydoo123]

This is a must fix for our embedding clients. With this fix in, users will not
encounter the scenario where they have installed a plug-in and are then
instructed to reinstall the same plug-in.

Comment 3

[Peter Lubczynski]

I think what we need here is that when we hit the first default plugin of a
page, that before showing it, we make sure our "plugin cache" is up-to-date. 

Isn't plugins.refresh() smart enough today not to refresh if nothing has changed?

Comment 4

[av (gone)]

Yes it is. I'll post a patch shortly.

Comment 5

[av (gone)]

Attachment: patch v.1

I tested this patch with Flash in both embedded and full page modes.

Comment 6

[TaylorToddK]

A patch for this problem would be extremely invaluable.  Since mozilla does not 
have a way to be aware of vendors installers that deliver plug-ins during a 
browser session, a solution to this problem is needed.  This will allow the 
browser to have an improved on-demand behavior, and sounds like it would allow 
the user to simply refresh the page to activate installed plug-ins even inside 
of the default plug-in experience.

Comment 7

[av (gone)]

Comment on attachment 83001 [details] [diff] [review]
patch v.1

Peter just generated a fountain of ideas how to make this patch much better.
Marking 'needs-work'.

Comment 8

[av (gone)]

Attachment: patch v.2

Comment 9

[av (gone)]

This patch (like the previous patch) refreshes the plugin list if unknown
content is encountered and tries to launch a plugin again before making a
decision to show the default plugin (PFS) or pop up Save/Open dialog box (in
case of full-page plugin). The improvements are:

1. full-page related change partially moved from nsDocShell.cpp to
nsPluginDocLoader.cpp, unfortunately I could not comletely avoid touching
nsDocShell.cpp

2. reloading the plugins does not cause shutting down running plugins

3. embed case is smarter now, if there are more than one <embed> tags with
unknown plugin reload will be done only once, no need to do it on every <embed>
tag on the page

4. |ReloadPlugins| logic slightly changed (to make #2 possible) -- it will now
look for the changes in plugins and will not do anything if not needed, for both
reloadpages=0 and reloadpages=1. Before it was 'smart' only for reloadpages=1

Please review.

Comment 10

[av (gone)]

Attachment: patch v.3

made using our plugin reload code less confusing and fixed major problem with
refreshing components (nice catch, Peter). Please review.

Comment 11

[av (gone)]

Attachment: patch v.4

one line change, should not really matter, just looks right -- moved
|LoadPlugins| so that it gets called _after_
|nsIComponentRegistrar->AutoRegister|, not before.

Comment 12

[av (gone)]

Attachment: patch v.5

AutoRegistrar code moved to nsPluginArray::refresh, so it does not hit the
performance too much (on envoking the default plugin every time), it still can
be called by navigator.plugins.refresh as it was used to; also peterl's change
for bug 144058 is included. Please review.

Comment 13

[Peter Lubczynski]

Comment on attachment 83417 [details] [diff] [review]
patch v.5

r=peterl

Comment 14

[Patrick C. Beard]

Comment on attachment 83417 [details] [diff] [review]
patch v.5

Nit, don't go changing spacing in other people's files:

-    nsCOMPtr<nsIDocumentLoaderFactory>
-	 docLoaderFactory(do_CreateInstance(contractId.get()));
+    nsCOMPtr<nsIDocumentLoaderFactory>
docLoaderFactory(do_CreateInstance(contractId.get()));

Here's a problem, you took out a NULL check after calling do_GetService:

   nsCOMPtr<nsIPluginHost> pluginHost = do_GetService(kPluginManagerCID);
-  if(! pluginHost)
+  if (NS_FAILED(pluginHost->IsPluginEnabledForType(aContentType))) {

I'm wondering how caching the "current" nsIDocument will affect destruction of
the document. Consider using a weak reference if the class supports it.

I'm concerned also that if there are multiple documents being loaded, that your
caching strategy won't work, since there's only 1 plugin host. You may have to
keep a hash table of the currently loading documents to keep from thrashing
reloading plugins.

Comment 15

[av (gone)]

Attachment: patch v.6

Patrick's comments addressed except the last one. I could not find a good way
to determine if we are currently loading another page with plugins. Adding
document observer at the point we try to instantiate the plugin instance does
not help much as |EndLoad| event have been already fired and we are not going
to get it.

Patrick, if you think this can go without it please sr. Otherwise, I will need
an idea how to do this best.

Comment 16

[Patrick C. Beard]

Comment on attachment 83607 [details] [diff] [review]
patch v.6

Sorry, I should have been more explicit about the style of weak reference to
use. It isn't safe to keep a weak reference like this, because the memory may
get recycled. You should instead use an nsIWeakReference (search for other uses
of nsWeakPtr) in the tree. Other than this, looks fine.

Comment 17

[av (gone)]

Attachment: patch v.7

Comment 18

[Patrick C. Beard]

Comment on attachment 83702 [details] [diff] [review]
patch v.7

sr=beard

Comment 19

[Peter Lubczynski]

Comment on attachment 83702 [details] [diff] [review]
patch v.7

r=peterl

Comment 20

[Peter Lubczynski]

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

Comment 21

[av (gone)]

The fix is on trunk. Marking fixed.

Comment 22

[scottputterman]

shrir, could you verify this on the trunk?

Comment 23

[shrirang khanzode]

yes, this is fixed on the trunk. just verified with flash and qktime on 0521 
trunk.

Comment 24

[scottputterman]

adding adt1.0.0+ for checkin to the 1.0 branch.  Please get drivers approval
before checking in.

Comment 25

[av (gone)]

I applied the patch to my local branch build and conducted some extensive
testing to make sure no regression will be intrduced by the patch. The following
has been done on Win2000/XP and Win98 so far:
               moz smoke tests    moz basic tests   special vendor tests
Acrobat                x                 x                    x
Flash                  x                 x                    x
MediaPlayer            x                 x                    n/a
QuickTime              x                 x                    n/a
RealPlayer             x                 x                    n/a
ViewPoint              x                 x                    x

Evetything worked as expected.

Comment 26

[scottputterman]

changing to adt1.0.1+ for checkin to the 1.0 branch for the Mozilla1.0.1
milestone.  Please get drivers approval before checking in.

Comment 27

[chris hofmann]

Comment on attachment 83702 [details] [diff] [review]
patch v.7

a=chofmann for 1.0.1

Comment 28

[av (gone)]

It is in the branch now, trying to add a keyword...

Comment 29

[shrirang khanzode]

this is fixed...verified it on 0625 brnch on win mac linux.