Closed Bug 77231 Opened 24 years ago Closed 23 years ago

Code for finding plug-ins is wrong.

Categories

(Core Graveyard :: Plug-ins, defect)

Product:
Core Graveyard
Component:
Plug-ins
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.7

People

(Reporter: ccarlen, Assigned: ccarlen)

References

Details

(Keywords: topembed)

Attachments

(2 files, 1 obsolete file)

updated patch
46.92 KB, patch
serhunt
: review+
waterson
: superreview+
Details | Diff | Splinter Review
patch for 0.9.4 branch
45.87 KB, patch
serhunt
: review+
beard
: superreview+
Details | Diff | Splinter Review
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.4 → edt0.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: 23 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
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: