Closed Bug 77231 Opened 20 years ago Closed 19 years ago

Code for finding plug-ins is wrong.

Categories

(Core :: Plug-ins, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla0.9.7

People

(Reporter: ccarlen, Assigned: ccarlen)

References

Details

(Keywords: topembed)

Attachments

(2 files, 1 obsolete file)

nsPluginHostImpl uses it own nsFileSpec based code for finding plug-in dirs and
loading plug-ins. This completely bypasses nsIDirectoryService which, for
embedding, is wrong. There is a key defined in directory service for the
plug-ins dir which returns an nsIFile and is perfectly XP.
Another curious thing (on Mac) is that there is a different selector for the
"old" plug-ins dir called "Plug-ins" and the new one is "Plugins" I have never
seen an installation with a dir called "Plugins." Also, the rest of the world
uses the hyphenated form. If we need to add new selectors to
nsAppDirectoryServiceDefs.h, we can, but I think we should drop this one.
-> 0.9.1
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.1
-> 0.9.2
Target Milestone: mozilla0.9.1 → mozilla0.9.2
-> 0.9.3
Target Milestone: mozilla0.9.2 → mozilla0.9.3
Peter, you might want to know about this.
There are platform specific implementations of nsPluginDir (nsFileSpec) for 
searching for plugins. All of these should probably be converted if the Mac is 
converted.

However, I'm not quite sure why there is a problem using nsFileSpec? I'm not 
sure that an XP way of detecting plugins is correct here as each platform has 
it's own qirks for locating plugins. Andrei?

I didn't even know nsIDitrectoryService existed nor am familiar with it's usage. 
 Is there any documentation on this service or just sift through LXR?
There is documentation at http://www.mozilla.org/projects/xpcom/file_locations.html.

Using directory service, all of the platform specific code is hidden in the
directory service providers. To a caller, like the plugin loader, it's as clean
and XP as:

nsCOMPtr<nsIFile> pluginsDir;
NS_GetSpecialDirectory(NS_APP_PLUGINS_DIR, getter_AddRefs(pluginsDir));

If more directory service keys need to be defined, that's easy enough. But, it
makes things clean and XP for consumers. Also, not using directory service and
instead doing the file location by itself, the plugin loader is preventing
embeddors from supplying their own locations.
Hmm...I see the problem on Mac. In nsPluginsDirMac.cpp, getApplicationSpec() 
should probably use something like the windows version (nsPluginsDirWin.cpp) 
does with nsSpecialSystemDirectory 
plugDir(nsSpecialSystemDirectory::Moz_BinDirectory), see:

http://lxr.mozilla.org/seamonkey/source/modules/plugin/nglsrc/nsPluginsDirWin.cp
p#161

If you want to move all the platform specific stuff out of plugins and entirely 
get rid of nsPluginsDir, you'll need to supply constants for the Win32 plugins 
folder, Win32 JRE folder, the old mac "plug-ins" folder (vs. "plugins"), and 
the Mac Internet Plugins folder.

IMO, we do need to fix the Mac code to get the right Moz Bin folder and system 
"Plugins" folder, but plugin platform specific code should probably say in 
plugins. It's not like there isn't a lot of it already. It is faily assumed in 
all browsers and by plugin vendors and also by our mew proposed method of having 
plugin vendors locate this folder that it always be a subfolder in the location 
of the binary. 
Actually a plugins folder named "plugins" in the app folder will never be valid 
on the Mac and that case can be dropped.  It only appeared when there were 
thoughts of harmonizing the folder name cross platform but it broke every Mac 
plugin installer that knew to look for Netscape with an app type of 'MOSS' and 
then install into the folder named "Plug-ins" in the same location as the app.
> If you want to move all the platform specific stuff out of plugins and
> entirely get rid of nsPluginsDir,

That's the plan.

> you'll need to supply constants for the
> Win32 plugins folder, Win32 JRE folder, the old mac "plug-ins" folder
> (vs. "plugins"), and the Mac Internet Plugins folder.

As Dagley confirmed, there is no "plugins" folder on the Mac - only "plug-ins"
Currently, there is a directory service key NS_APP_PLUGINS_DIR. On every
platform it refers to the plug-ins dir in the bin directory. Two more keys could
be added.
(1) NS_SYSTEM_PLUGINS_DIR. This location would be platform dependent and would
be whatever the system considers to be the plug-ins dir.
(2) NS_WIN_JRE_DIR. Defined only for windows.

The advantage of this, in addition to using the right dir for the Moz Bin dir,
and allowing embeddors control over NS_APP_PLUGINS_DIR is that aliases on the
Mac would be handled properly. They're not right now and, using nsIFile, this
would come for free.




>> If you want to move all the platform specific stuff out of plugins and
>> entirely get rid of nsPluginsDir,

>That's the plan.

Fine by me as long as all platforms are covered. Andrei may have some comments.
OS: Mac System 9.x → All
Hardware: Macintosh → All
-> mozilla1.0
Target Milestone: mozilla0.9.3 → mozilla1.0
Depends on: 105440
*** Bug 106119 has been marked as a duplicate of this bug. ***
Now that xpti can scan any directory, I like Conrad's idea of implementing:

NS_SYSTEM_PLUGINS_DIR  (overridable by embedders)
NS_USER_PLUGINS_DIR
NS_4X_PLUGINS_DIR
NS_WIN_JRE_DIR

...but what about if we need an enumeration of directories? Perhaps this won't
be needed any more by embedders if #1 is implemented (hint, hint) or maybe could
we use a dynamically generated string key?
Keywords: topembed
How about this: a slight variation on the theme

NS_APP_PLUGINS_DIR (already have this defined, overridable by embeddor)
NS_USER_PLUGINS_DIR (OS defined, only on multi-user-systems, not overridable)
NS_GLOBAL_PLUGINS_DIR (OS defined, not overridable)
NS_4X_PLUGINS_DIR
NS_WIN_JRE_DIR


Then I think we may need another one, for a shared location for embedders. On
Windows, some embedders are going to want a shared plugins directory per
product, perhaps going in C:\Program Files\Common Files\<product shared plugins
folder> in addition to the application level one. Besides, the application level
one may also be desired for backwards compatibility and overridablity.
I think that the way we search for plugins should be the same as the way we
search for plugins.  See:

http://bugzilla.mozilla.org/show_bug.cgi?id=105440
Since topembed -> 0.9.6. I'm presently working on the XPCOM support needed by
this (bug 105440)
Target Milestone: mozilla1.0 → mozilla0.9.6
*** Bug 36973 has been marked as a duplicate of this bug. ***
*** Bug 102700 has been marked as a duplicate of this bug. ***
Here's what I found about how 4.x did a plugin scan on UNIX:

128 if($NPX_PLUGIN_PATH environment variable is set)
129 Look at $NPX_PLUGIN_PATH, where $NPX_PLUGIN_PATH is
130 a colon-delimited list of directories.
131 else
132 Look at all the following directories in order, overriding
133 previous entries in case of duplicates:
134 /usr/local/lib/netscape/plugins
135 $MOZILLA_HOME/plugins
136 $HOME/.netscape/plugins

http://lxr.mcom.com/nova/source/cmd/xfe/README.tmpl#128
Blocks: 45699
The patch gets rid of the use of nsPluginsDir in favor of directory service.
That means embeddors can hook into it. This patch requires attachment 55826 [details] [diff] [review] from
bug 105440.
Can I get some review on this?
This is quite a big patch. Please, give me some time.
nom'ing
Keywords: edt0.9.4
Attachment #55828 - Attachment is obsolete: true
The patch looks big but it really just removes a lot of code and moves some
elsewhere. Here's a summary for the reviewer's sake:

* All plugin location is done using directory service. This allowed most code to
be removed from nsPluginsDir and some from nsPluginFile.
* In nsPluginHostImpl::LoadPlugins(), scanning happens more consistently on all
platforms in this order:
  1. The list of dirs signified by NS_APP_PLUGINS_DIR_LIST is scanned. By
default, this dir contains what was MOZ_LOCAL. An embedding app can provide
whatever it wants in this list.
  2. The list of dirs signified by NS_OS_PLUGINS_DIR_LIST is scanned. This list
is currently non-empty on Mac OS. Notice that it's not conditionally compiled.
This allows the providers to be changed (wherever they may be) without affecting
LoadPlugins().
  3. On Windows only, the 4x plugins and Java JRE dirs are scanned. Since these
keys are only useful to the plugin loader, they are not provided by XPCOM or the
application's provider. It's done by a private provider implemented in the new
file nsPluginDirServiceProvider.cpp. The code for this was pretty much copied
from nsPluginsDirWin.cpp. Also, the creation and registration of this provider
is not conditionally compiled for the same reason as in 2.
* Some changes had to be made to nsPluginHostImpl::ScanPluginsDirectory() to
make it work with nsIFile. Eventually, this whole module needs to be converted
to use nsIFile.
* ns4xPlugin::CreatePlugin() was changed to take the full path of the plugin as
well as the file name. Doing this allows us not to re-scan two plugin dirs for
each 4.x plugin found. Again, that method should take an nsIFile but, in the
short term, this will avoid the re-scanning.

This is a great patch! Let me test it before I mark it reviewed. Meanwhile 
couple of minor comments:

- in nsPluginHostImpl.cpp header nsDirectoryServiceDefs.h is included twice now, 
we can probably remove the one which is inside #ifdef XP_WIN

- nsPluginHost.h should include nsIDirectoryService.h, otherwise it does not 
compile on Windows.
Another note. If this is going to the branch, dp's patch for caching the plugin 
info should go there first or we will get a lot of conflicts.
> If this is going to the branch, dp's patch for caching the plugin info should
> go there first.

That's for sure. This patch requires the fullPath info for the Mac which dp
added in order to avoid rescanning dirs in ns4xPlugin::CreatePlugin().

Forgot to say: I'll fix up what you pointed out and anything that your testing
may turn up after I hear from you on that.
Comment on attachment 56574 [details] [diff] [review]
updated patch

Works beautifully on Windows so far, r=av with my comments addressed.
Attachment #56574 - Flags: review+
Waterson, can you sr this patch?
Comment on attachment 56574 [details] [diff] [review]
updated patch

mmm, good. sr=waterson
Attachment #56574 - Flags: superreview+
This is blocked by bug 101769 for the 0.9.4 branch because it's built on top of
that patch in the current trunk.
Depends on: 101769
No longer depends on: 101769
I have this patch for the 0.9.4 branch coming up. As soon as I test it on Windows.
gnarly merge - it's changed a lot since 0.9.4 The patch is the same as the
trunk patch but addresses Andrei's comments with the first patch.
This will be great, as the Crossover plugin for Linux (allows viewing Quicktime)
is single-user only!   When attempting to run Mozilla as a user other than the
one who installed Crossover will result in a segmentation fault.
Mass move to 0.9.7
Target Milestone: mozilla0.9.6 → mozilla0.9.7
can we get r/sr on this for the 0.9.4 patch that conrad has applied?
Comment on attachment 57085 [details] [diff] [review]
patch for 0.9.4 branch

r=av
Attachment #57085 - Flags: review+
Checked into trunk. +207/499 :-) Leaving open 'til it goes into 0.9.4 
The checkin of the first patch on the trunk(in particular, the removal of the
nsPluginsDir constructor in the Unix implementation) seems to have removed the
reading of the MOZ_PLUGIN_PATH environment variable on unix.
Bug 110660 opened on that. This bug was about using diectory service to find
plugins. What directories are provided by dir service is a different matter.
looks good on windows trunk (1119). All plugins detection(install 
path)/working(operation) is fine. Will chek other plfs asap.  
looks ok on mac trunk as well.all required plugins were identified/loaded fine 
from HD and plugins folder. 
confirmed with peter that we do not do a sweep on linux like we do on 
mac/win, except for the env variable in bug 110660. So this bug is verif-fxd 
unless we want the fix for 110660 in the 0.9.4 branch as well.
why is this required for the embedding client at this point (i.e., what will be
the implications visible to the user if we do not take this patch)?
the 0.9.4 patch looks long. Does it include the at this point dp's patch for
caching the plugin or this is a separate bug that would need to be plussed?

It's required for the embedding client because without it, they won't be able to
specify which directories are scanned for plugins. The 0.9.4 patch does not
include dp's caching code. Whether that's plussed along with this, I don't know.
This was to meet a requirement of an embedding client.
My sense is the plugin caching need not be taken for 0.9.4 : +ve is perf, -ve is
unneccessary risk. The +ve isn't high enough for windows - it is more for unix, mac.
I would give it a plus if patch is correct without dp's fix (I'm averse to risk
:-)). is it correct?
Yes - attachment 57085 [details] [diff] [review] was made against the 0.9.4 branch. It goes in as-is,
unless any other major landings have gone into the plugin loading code on the
0.9.4 branch.
I'm marking it edt0.9.4+
please check it into 0.9.4, but could we get sr for 0.9.4 patch before doing this?
Keywords: edt0.9.4edt0.9.4+
Waterson, can you sr= the 0.9.4 patch as well?
Comment on attachment 57085 [details] [diff] [review]
patch for 0.9.4 branch

nsPluginHostImpl::GetProgramPath() appears to leak the C string pointed
at by temp. I couldn't find any callers of this method using LXR, so
this may not be a big deal. nsPluginHostImpl::GetTempDirPath() has the
same leak.

sr=beard
Attachment #57085 - Flags: superreview+
Checked into 0.9.4.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Keywords: fixed0.9.4
Resolution: --- → FIXED
veirf both on trunk and branch that this is fixed.
Status: RESOLVED → VERIFIED
had forgotten to add keywd 'verified0.9.4'
Keywords: verified0.9.4
Keywords: fixed0.9.4
You need to log in before you can comment on or make changes to this bug.