101769 - Stop loading plugins on startup

Status: VERIFIED FIXED

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

Description

[Suresh Duddi (gone)]

Linux and mac load all plugins on startup. We should store the plugin info from
registry and read it from there unless the plugin has changed (filetime check)

Comment 1

[Suresh Duddi (gone)]

This is a startup perf issue.

Comment 2

[Erik Hensema]

It's also very annoying. The Macromedia Flash plugin seems to initialise the
sound driver which fails on my system because KDE's artsd is running. Flash
blocks until I kill off artsd.

Strange thing: the same plugin does work on Navigator 4.7.

Comment 3

[Patrick C. Beard]

I tried to fix this in the Mac code a while back, but there is a bug which
prevents a subsequent attempt to load a Mac plugin from working.

Comment 4

[Suresh Duddi (gone)]

Attachment: Cache plugin info so we dont need to load plugins on startup

Comment 5

[Suresh Duddi (gone)]

Issues with patch:
- We can optimize the two create of registry objects happening.
- What happens when we do reload plugins ? I guess we should do a sync our
  cache after reload.
- cache doesn't affect xpcom plugins (plugins detected by LoadXPCOMPlugisn()).
  I guess that is ok since they dont cause a load of plugin; Plus Patrick tells
  me that we should OBSOLETE and possibly even rip it out.
- Still need to add code to detect timestamp change on plugin dll and disable
  cache of that plugin
- Cache use filename as a string. Is that ok for the mac ?

av, if you think this is heading in the right direction, we can talk and resolve
all the above.

Comment 6

[av (gone)]

On reload plugins we should probably do the complete rescan of the plugin 
folder. 

Another idea: could be benefitial for Windows too, why not to accept this 
approach for all platforms?

Comment 7

[Suresh Duddi (gone)]

Oh Yeah! The patch does it for all platforms.

Comment 8

[Suresh Duddi (gone)]

Ccing conrad for any possible embedding issues.

Comment 9

[Conrad Carlen (not reading bugmail)]

This should be fine for embedding apps. They will have the registry referred to
by nsIRegistry::ApplicationRegistry. If they do not pass a directory service
provider to NS_InitEmbedding, it will be the same registry as used by mozilla/NS.

As far as using strings for the paths to the cached plugins, Mac users are prone
to change the name of their drives at any time and then the hard path is then
invalid. Also, paths aren't unique on the Mac. There is a pretty easy way around
that though. You can use the PersistentDescriptor methods of nsIFile (nsFileSpec
has something like this as well). It will work XP but do something better on the
Mac. 

Comment 10

[Suresh Duddi (gone)]

Taking over bug since I am working on this one.

Comment 11

[Suresh Duddi (gone)]

Attachment: v2.0 caching plugininfo in registry

Comment 12

[Suresh Duddi (gone)]

Ok this new patch fixes all issues above + implements versioning of the
nsplugins info in the registry and adds useful prlog stmts.

Conrad: This patch still doesn't address using the persistentfiledescriptor.
Plugins code uses a mismash of nsIFile and nsfilespec. Didnt know how to get
persistentfiledescriptor from an nsIFile. Anyone ?

r=/sr= please

Testing: I copied the libflashplayer.so into my plugins directory and tested
that it doesnt get loaded after caching, gets loaded if I touch it and saw flash
work in all cases in my browser. Did about:plugins to see the plugins info.
av: This patch is final enough that you can try it on your windows
buildconrad/bread: any help with mac ?

Comment 13

[Conrad Carlen (not reading bugmail)]

From nsIFileSpec:
http://lxr.mozilla.org/mozilla/source/xpcom/io/nsIFileSpec.idl#70
and from nsIFile, QI to an nsILocalFile and then:
http://lxr.mozilla.org/mozilla/source/xpcom/io/nsILocalFile.idl#107

The data returned by each class is interchangeable with the other.

And, for a good essay on not using paths to specify files,
http://lxr.mozilla.org/mozilla/source/xpcom/io/nsIFile.idl#28 ;-)

Comment 14

[Suresh Duddi (gone)]

Thanks Conrad. I looked into doing the persistentDescriptor changes too. But
those changes are a little bit more pervasive (apis needs to change). I think
the right thing to do is to get this in and do that next along with conversion
to nsIFile from nsFileSpec.

Comment 15

[Conrad Carlen (not reading bugmail)]

Sounds fine. Since you're using registry APIs, you can just define a new key
that gets stored when the change to persistent descs gets made. If the new key
isn't there, we use the old key/value. I don't think it will be hard to have a
migration path, so go ahead and use paths for now.

Comment 16

[Suresh Duddi (gone)]

We can even do better than that. There is versioning of the registry hierarchy
in the patch. If version numbers dont match, the registry hierarchy is thrown
and recreated.

So when we do the persistentDescriptor I would just not store "filename" anymore
and change the key to "persistentDescriptor" and bump the version number
kPluginsInfoVersion

Comment 17

[Suresh Duddi (gone)]

Attachment: v2.0 attaching correct patch this time.

Comment 18

[Conrad Carlen (not reading bugmail)]

Storing both would allow forward/backward compatibility.

Comment 19

[Suresh Duddi (gone)]

After I tested it on windows: 
- CachePluginsInfo() never happens because of premature returns in NS_WIN only code.
- Unwanted plugins need to be cached and not loaded
- Unwanted plugins cause mSyncCachedPlugins to be set everytime

New patch in the works.

Comment 20

[Suresh Duddi (gone)]

Attachment: Final patch after windows and unix testing

Comment 21

[Suresh Duddi (gone)]

This latest patch covers all issues including versioning, filestamp check,
dealing with unwanted plugins.

Works on windows and unix. Checking it on the mac.

Comment 22

[Suresh Duddi (gone)]

Attachment: Patch after win,unix,mac testing

Comment 23

[Suresh Duddi (gone)]

The mac was using mFullPath and mFileName while other platforms were using only
mFileName. This is all ugly and converting to nsIFile and using persitent
descriptor would fix it. Filed bug 102700

So finally this works on all platforms.

Ccing waterson. sr= ?
av r= ?

Comment 24

[av (gone)]

dp, I am looking at it. Still need some time to fully understand it.

Comment 25

[Suresh Duddi (gone)]

Let me know once you have reviewed it.

Comment 26

[av (gone)]

Comment on attachment 51699 [details] [diff] [review]
Patch after win,unix,mac testing

nice clean up, so far no problems with the patch, 
r=av, will file issues as they arise.

Comment 27

[Chris Waterson]

Don't need to check |if (foo)| to |delete foo;|, |delete| handles null pointers:

+      }
+      else {
+        if (pluginTag)
         delete pluginTag;


Linked list removal is less painful if you maintain a link pointer (in
RemovedCachedPluginsInfo):

  nsPluginTag *tag, **link;
  for (link = &mCachedPlugins, tag = *link;
       tag != nsnull; 
       link = &tag->mNext, tag = *link) {
    if (/* found the element to remove */) {
      *link = tag->mNext;
      return tag;
    }
  }

Otherwise, looks good. sr=waterson

Comment 28

[Brendan Eich [:brendan]]

You can consolidate the iterator-pointer assignment via

  nsPluginTag *tag, **link;
  for (link = &mCachedPlugins; (tag = *link) != nsnull; link = &tag->mNext) {
    if (/* found the element to remove */) {
      *link = tag->mNext;
      return tag;
    }
  }

and also fit the for-loop control on one line.  Bonus!

/be

Comment 29

[Suresh Duddi (gone)]

Thanks guys. Brenda, I will stick with the multiline for loop as I have a
preference for not having assignments as sideeffects.

Comment 30

[Suresh Duddi (gone)]

Thats the second time i missed the 'n' out of brendan. I swear that wasnt
intentional. Now, got to figure out who brenda is ...

Comment 31

[Suresh Duddi (gone)]

Fixed check in 0.9.6

Comment 32

[Brendan Eich [:brendan]]

(don't remind me of Beverley Hills 90210!)

side effects are all the for loop control is about, but choose your poison.  The
redudancy isn't the only objection to assigning in the init and update parts --
it's also visually busier, and the comma operators don't help.  Parting shot
from Brenda :-)

/be

Comment 33

[Daniel Mario Vega (Restore MNG Support --> #18574)]

Buil ID: 2001101510

I installed this build on Linux, I started and closed mozilla a few times so
plugins get registered. Then I started mozilla (my start page is a blank page).
And without browsing any other page, I do on command line "cat
/proc/<MozillaPid>/maps" and I still see all the plugins in memory. ( npmidi.so,
libflashplayer.so, nppdf.so, rpnp.so, javaplugin_oji.so and libnullplugin.so ).

Am I doing anything wrong ?

Comment 34

[Daniel Mario Vega (Restore MNG Support --> #18574)]

I know. My problem is obvious. I have an old build. I downloaded today a nightly
build ( mozilla-gcc295-i686-pc-linux-gnu.tar.gz ) but it seems to be rather old.
Sorry for the spam. :-(

Comment 35

[Peter Lubczynski]

I think the branch would like this as well.

Comment 36

[Conrad Carlen (not reading bugmail)]

I have the patch for bug 77231 almost merged into 0.9.4. I think it should be
able to go into the branch itself.

Comment 37

[Peter Lubczynski]

Conrad, be sure you get the patch in bug 101769 which is really part of this bug.

Comment 38

[Peter Lubczynski]

This patch really won't help Windows at all and in fact has caused a few
regressions so I don't think anymore it should NOT be on the embedding branch.

Comment 39

[av (gone)]

I agree with Peter. There is no full understanding of some of the regressions. 
See, e.g., bug 112508.

Comment 40

[shrirang khanzode]

fixed a while ago. verif!