Stop loading plugins on startup

VERIFIED FIXED in mozilla0.9.6

Status

()

defect
P2
normal
VERIFIED FIXED
18 years ago
18 years ago

People

(Reporter: dp, Assigned: dp)

Tracking

({perf})

Trunk
mozilla0.9.6
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Assignee

Description

18 years ago
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)
Assignee

Comment 1

18 years ago
This is a startup perf issue.
Blocks: 7251
Keywords: perf

Comment 2

18 years ago
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

18 years ago
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.
Assignee

Comment 5

18 years ago
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

18 years ago
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?
Assignee

Comment 7

18 years ago
Oh Yeah! The patch does it for all platforms.
Assignee

Comment 8

18 years ago
Ccing conrad for any possible embedding issues.
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. 
Assignee

Comment 10

18 years ago
Taking over bug since I am working on this one.
Assignee: av → dp
Assignee

Updated

18 years ago
Attachment #51087 - Attachment is obsolete: true
Assignee

Comment 11

18 years ago
Assignee

Comment 12

18 years ago
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 ?
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 ;-)
Assignee

Comment 14

18 years ago
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.
Assignee

Updated

18 years ago
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla0.9.5
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.
Assignee

Comment 16

18 years ago
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
Assignee

Updated

18 years ago
Attachment #51228 - Attachment is obsolete: true
Assignee

Comment 17

18 years ago
Storing both would allow forward/backward compatibility.
Assignee

Comment 19

18 years ago
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.
Assignee

Updated

18 years ago
Summary: Linux & Mac load plugins on startup → Stop loading plugins on startup
Assignee

Updated

18 years ago
Attachment #51335 - Attachment is obsolete: true
Assignee

Comment 21

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

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

Updated

18 years ago
Attachment #51574 - Attachment is obsolete: true
Assignee

Comment 23

18 years ago
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

18 years ago
dp, I am looking at it. Still need some time to fully understand it.
Assignee

Comment 25

18 years ago
Let me know once you have reviewed it.
Target Milestone: mozilla0.9.5 → mozilla0.9.6

Comment 26

18 years ago
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.
Attachment #51699 - Flags: review+

Comment 27

18 years ago
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
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
Assignee

Comment 29

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

Comment 30

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

Comment 31

18 years ago
Fixed check in 0.9.6
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
(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
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 ?
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

18 years ago
I think the branch would like this as well.
Keywords: edt0.9.4
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.
No longer blocks: 77231
Keywords: edt0.9.4

Updated

18 years ago
Blocks: 105935

Comment 37

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

Comment 38

18 years ago
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

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

Comment 40

18 years ago
fixed a while ago. verif!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.