Closed Bug 105440 Opened 24 years ago Closed 24 years ago

nsIDirectoryServiceProvider needs new dir keys

Categories

(Core Graveyard :: Profile: BackEnd, defect)

Product:
Core Graveyard
Component:
Profile: BackEnd
Platform:
x86
Windows 2000
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.6

People

(Reporter: jud, Assigned: ccarlen)

References

Details

(Keywords: topembed)

Attachments

(3 files, 3 obsolete files)

patch to an embedding app to use this code
7.03 KB, patch
Details | Diff | Splinter Review
smaller patch without objectionable keys
20.34 KB, patch
jud
: review+
jband_mozilla
: superreview+
Details | Diff | Splinter Review
same as final patch but for 0.9.4
19.15 KB, patch
dougt
: review+
jband_mozilla
: superreview+
Details | Diff | Splinter Review
This is an offshoot of http://bugzilla.mozilla.org/show_bug.cgi?id=14923 . There are a few components that need access to application specified directories. XPCOM registration would like to register comps in the user's dir on *nix boxes, plugins would like to scan application specified dirs for plugins, and xpti needs to scan these dirs as well to pull in the type info data. I see at least two keys: - SHARED_PLUGINS - ADDITIONAL_COMPONENTS I think precidence should have these dirs scanned *after* Gecko core dirs are scanned which makes these dirs the "override" dirs.
Keywords: topembed
If we are going to have fix number of directory locations (which I think jband is against), I want to see at least a user and shared directories for each of these: - PLUGINS - PLUGINS_GLOBAL - PLUGINS_USER - COMPONENTS - COMPONENTS_GLOBAL - COMPONENTS_USER Also, we really want to have the modules/plugin code support this. I think right now they are doing their own directory parsing.
the modules/plugins would start using this.
I am unexcited about a fixed set, but understand the pragmatics. Remember that these need to be available at xpcom startup - no directories found based on reading user prefs or anything. So, be careful with what you mean by 'user' - do you mean user of this OS session? or user based on profile choice? I'd like it if we had a clear choice of keys to use to determine the *writable* place to put the xpti.dat manifest. Putting it in the components directory is a bug. This will matter more if it going to need to be written more often as per-'user' xpt files get added at times other than at install. time.
I meant OS session.
I think the keys doug listed mean "user" in terms of the OS. If those are in terms of the OS, they could be provided by the directory service provider implemented in nsDirectoryService.cpp. - ADDITIONAL_COMPONENTS This one, I think, is in the domain of the application and one which an embedding app may wish to customize with their own directory service provider. Is this one available to all OS users? All installations of that embedding app on the machine?
Also, The *_USER dirs dougt suggested implies that the app is going to find different stuff when used by different users (even if only in the OS sense). This only flys if we put the the components.reg and xpti.dat in user specific directories and build different ones (with no sharing of those registries) for different users. I'm fine with that. But are we going there now?
I always thought we'd merge the registries and xpti data, loading system, then shared, then user on top of each other. That way an admin can still meaningfully upgrade the shared installation, etc. (What _does_ happen in the case where we have Mozilla 0.9.6 installed with some titbits in the user's personal xpti.dat, and then the admin upgrades the system to Mozilla 0.9.7?)
This would make blizzard happy - user components.
- PLUGINS - PLUGINS_GLOBAL - PLUGINS_USER Having these different domains makes sense on a multi-user system. But on a single-user system? I think that on systems where some of these keys are not applicable, directory service should just return an error. It should be documented which keys are optional so that consumers can expect failure. They should still be scanned in the same order (system, global, user) but, on some systems, the 2nd and/or third locations may not exist.
shaver: You touch on the reason that xpti *always* trusts the first instance of an interface info it finds. And why we are *so* screwed with the whole idea of leaving behind xpcom components after an application update. For (mythical) plugins using only forever-frozen interfaces it may be OK to just have the new version of the app use the existing plugins. But, for xpcom components we are just *asking* for trouble. Our installer does this now. It only deletes the components it knows to delete. Any addon component the user might have added is left behind as a potential timebomb that may or may not crash the new version of the app when it runs.
accepting
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.6
Blocks: 106119
Blocks: 106122
Blocks: 70229
Here is what I mean: "USER" in the mozilla browser would be some directory in the user's profile. What is going to be interesting is that this is not selected until after autoreg completes. (new bug?) "GLOBAL" in the mozilla browser would be some mozilla-agreed upon place on the machine to look for components/plugins. Custom applications can do what they want (eg override this behavior via the nsIDirectoryService). If they want components installed by other mozilla-based applications, they don't modify this directory location. If they want to steear clear from these components/plugins, they can define this location as null. For example suppose a embedder has a couple applications that they want to share components/plugin between, but don't want to "share" these plugins with other mozilla-based applciation. They could redefine GLOBAL to point something other place that only their application know about. Hmm, basically what I think I am saying is GLOBAL is almost what ADDITIONAL_COMPONENTS is.
> "USER" in the mozilla browser would be some directory in the user's profile. > What is going to be interesting is that this is not selected until after > autoreg completes. (new bug?) I just don't see how we could reasonably do that. I can understand per OS user addons, but who needs per profile addons? We *must* do this lowlevel stuff before showing UI etc. We sure don't want to do it again after the profile is picked! Exactly how many keys are you suggesting now? Also, what is the propert way for the service to not support some given key? return an error code? or succeed with a null result?
hmm. you are right john. _USER directies can't be profile dependent. :-/
I misspoke when I said that you wanted to "redefine" in your directory service provider. If you are going to override a default property, you will want to directly access the nsDirectoryService and |Undefine| or |Define|. here is the intitial documentation of the nsIDirectoryService. I need to talk about overriding defaults... http://www.mozilla.org/projects/xpcom/nsDirectoryService.html
Why shouldn't my app be able to say "add this directory to the search path, and load component registry and XPTI and whatnot data from it now" at any point? Autoreg can have a category for "xpcom-search-path-contributors" and instantiate them and poke them after system autoreg is complete.
I like this idea much better than having to worry about the nsIDirectoryService.
Customizing locations is discussed here: http://www.mozilla.org/projects/xpcom/file_locations.html As I said there, whether you define a location by using nsIProperties::Define or by supplying a whole nsIDirectoryServiceProvider impl to NS_InitEmbedding or NS_InitXPCOM2, it's up to you. If you want to change just one location, the former is probably more convenient. If you want to redefine many, the latter is better. Either way, I think the keys should be defined and if they don't exist for a particular implementation, they're skipped (without error). This is better than having people explictly call an iface which we probably don't want them to know about or use and telling it "load component registry and XPTI and whatnot data from it now."
What interface that we don't want them to know about or use? This is the sort of thing (addComponentPath/removeComponentPath) that should be on the nsIComponentManager interface, as we freeze it. I think this is a much better plan than trying to predict what our downstream consumers are going to need in terms of component directory hierarchy, and then trying to help them outsmart us under the constraints of the API when it turns out we guessed wrong.
shaver: I'm trying to maintain a well ordered set of trusted type information. I want to continue persistently caching metadata about that set in order to avoid rebuilding it at runtime on each and every run. We started with a system where components and typelibs live in only one directory. We extended that system based on the requirement that the embedding would be able to supply a list of additional directories at startup time. I know full well that it *could* work any f'n way you want it to work. but I'm not interested in rewriting all the code at this point. Both xpt info scanning and component registration are order dependent. If your dynamic app pokes in directories in a different order from run to run then we would either need to re-autoreg or risk doing activation that is incompatible with the current search order (e.g. the shadowing is different in a search order a-b-c from an order of b-c-a). And what about the activations needed before poking is done. Do we not persist registries? Should we not activate from directores that are not yet in the path on a given run? (e.g I did autoreg when the order was a-b-c-d. It is now a-b-c. should I active something in d or not?). We would also need to be able to do incremental autoreg as *new* unseen directory was added. I think this is all rean nifty. But we we are trying to ship something that does what we need to do.
Every directory that's in the path gets a component.reg and an xpti.dat. You load them at startup, just like you do with the single one now, and we generate them at autoreg time, just like we do with the single one now. The same way you get a hook for xpcom-registration-observer notifications now, and do your xpti.dat thang, you will get one with an nsIFile parameter that points to the directory being registered. There's no ``what's in the path this time?'' problem: if the directory is in the search path, you'll be told to load from it. If it's not, you don't care: it's just another directory on the filesystem that has a file called xpti.dat in it. You don't have to grovel through typelibs, just merge a few xpti.dat files at startup. (And what do you do right now where two apps -- Komodo and Mozilla, say -- use the same system components dir as part of their path, and redefine another part of the hierarchy? We know that the ActiveState people want to be able to use an installed Mozilla as a basis for Komodo in the future; they've told us many time.) We _know_ that our consumers want to have per-profile plugins. I don't know why we did a one-off hack for having two directories, rather than N, to accomodate these consumers, but I came in late so there might be more context there than I see.
If you read my comments in the bug that we spun this off from you'll see I argue for adding an interface that allows an arbitrary length path. What I *am* pushing for is being able to know that path at app startup time and have it not change. I acknowlege that this is a problem if we are talking about per profile plugin dirs from which xpt files must be loaded. I'm not so sure this is a hard requirement for anyone. Your suggestion of multiple component.reg and xpti.dat files is fine and good, but it is not a matter of some simple changes to the code. Sets of type libraries are not additive. There are also verification and other issues to take into account. ActiveState can surely do what they want to do without dynamically building component search paths.
The immediate need for this bug is to address an app defined *plugins* directory (which, doesn't do XPCOM component registration at all). This bug is getting mired down in more complex XPCOM registration issues, which, w/ admittedly minimal thought on my part, can be considered orthogonal; no? I'm inclined to splinter yet another bug to address an app specified (profile independent; read that as global) plugins directory key (that would indeed hang off of nsIDirectoryServiceProvider). Is that a reasonable course of action?
Blocks: 77231
Jud, I think you could be talking about bug - or in any case, when you fix this bug, you could address additional plugin directories. http://bugzilla.mozilla.org/show_bug.cgi?id=77231 The question remains, how is the embedded application going to |set| these extra directories?
There already is a key defined for app-specific plug-ins: http://lxr.mozilla.org/mozilla/source/xpcom/io/nsAppDirectoryServiceDefs.h#71 It's defined by the application file loc provider so, assuming the embedding app is providing its own, its defined there. Really not a problem. The problem is that the plug-in loader is not using nsIDirectoryService. That's bug 77231. I think that this bug should be closed and I should focus on 77231.
Attached patch patch to xpcom/io (obsolete) — — Splinter Review
When I provide my own appfilelocationprovider to NS_InitEmbedding, the test code gives me: Getting list of directories for OSPluginsDL... X:Users:conrad:Library:Internet Plug-Ins X:Library:Internet Plug-Ins Getting list of directories for APluginsDL... X:Users:conrad:Development:Seamonkey:moz:mozilla:dist:viewer_debug:Plug-ins X:Users:conrad:Library:PPEmbed:Plug-ins When I don't provide my own appfilelocprovider and use the default, I get this: Getting list of directories for OSPluginsDL... X:Users:conrad:Library:Internet Plug-Ins X:Library:Internet Plug-Ins Getting list of directories for APluginsDL... X:Users:conrad:Development:Seamonkey:moz:mozilla:dist:viewer_debug:Plug-ins
Some things which still need to be done: (1) Locations for NS_OS_PLUGINS_DIR_LIST are fully supported only for the Mac. Mac OS has clear definitions of what these should be. I'm less sure on other platforms but I'm discussing it with plugin folks (Peter L. has responded, anyway). It's possible that NS_OS_PLUGINS_DIR_LIST doesn't apply on many platforms - in which case nothing needs to happen. NS_ERROR_FAILURE will be returned for that request and the consumer has to expect this. (2) There is now more platform specific code in nsDirectoryServiceProvider.cpp. I have to find a better place for it. A lot of the platform specifics dir service uses are hidden in nsSpecialSystemDirectory.cpp. That file needs to die because it's nsFileSpec based and, maybe at that point, some platform-specific files for the implementation of dir service can be introduced.
Maybe we should just have platform specific directory service providers?
Yeah. The provider object could be called nsCoreDirServiceProvider and be implemented in nsCoreDirSvcProvider[Mac | Win | Unix].cpp. These impls would mainly be code scavenged from nsSpecialSystemDirectory.cpp. That cleanup is another bug though.
Attachment #55228 - Attachment is obsolete: true
Attached patch new patch to xpcom (obsolete) — — Splinter Review
The updated patch adds directory service keys for two other locations needed by plugin scanning - 4.x Plugins and the Java JRE dir. Now the plugin loader can use only directory service. Reviews please?
It would be nice if the 4.x key was XP so we could support $NPX_PLUGIN_PATH or /usr/local/lib/netscape/plugins in the future. Here how 4.x did it: http://lxr.mcom.com/nova/source/cmd/xfe/README.tmpl#128 It would also be nice if the patch supported user plugins on all UNIX flavors, not just OSX. Scanning something like ~/.mozilla/plugins would be helpful to all those users who can't run Mozilla as root but want to use plugins. This should happen first, so those plugins get precedence. It can possibly just be hacked into GetNext() or should another platform specific class extending nsISimpleEnumerator like on Mac be created?
I mostly like this. A few comments... - You could rewrite nsAppPlugInsDirEnumerator to be data driven and generic. It could take a null terminated const* keys[] array and each 'GetNext' call could do a GetFile using the next key in the array. Then implmentors could leverage this code for other lists by just writing the code to handle additional (custom) keys and passing an array holding those keys. You could require the input strings to be static so that no key string copying would be required. - I'm thinking that caching the windows plugin dir might be good. I don't know how many times this is going to get called, but if it gets called much then this registry stuff is kind of expensive. Maybe it does not matter. - Are we going to also define some additional lists? I forget exactly what we decided about what embedders should expect regarding which lists get scanned for components, plugins, and typelibs.
Thanks for comments. First Peter's: > It would be nice if the 4.x key was XP If you're talking about the NS_WIN_4DOTX_PLUGINS_DIR, I thought we were trying to get rid of the notion of the 4.x plugin locations. I put it there only for Windows because that's the only platform that currently uses it and as soon as the installer migrates plugins, we can yank that key. > It would also be nice if the patch supported user plugins on all UNIX flavors I agree. Although, the gist of this bug is to provide new keys for directory service and provide a way of returning lists of locations. I think it should stand at that. The locations which are provided in this patch give the same locations as we are currently using plus the ability for an embedding app to hook up its own. I think this should go in as-is and then, with bug 77231, the locations themselves can be enhanced. And jband's: > You could rewrite nsAppPlugInsDirEnumerator to be data driven and generic. You're right. I was going to do that but then nsAppPluginsDirEnumerator ended up having only one location :-/ Since it may have more and it would make future expansion easier, I'll do what you suggest. > I'm thinking that caching the windows plugin dir might be good Directory service does cache. That's one thing that's so cool about it. It caches locations which the provider says are persistent. They all do AFAIK. nsISimpleEnumerators, of course, can't be cached. The key you're talking about are returned as single files so they are cached. > Are we going to also define some additional lists? I think we decided that additional components and typelibs would use the same keys as plugins. Anybody else cc'd here who was in this discussion remember better?
Attached patch patch to xpcom (obsolete) — — Splinter Review
Attachment #55826 - Attachment is obsolete: true
The new patch makes both nsAppPluginsDirEnumerator and nsSysPluginsDirEnumeratorMac be data driven and therefore more general. They were both renamed to reflect the greater generality. Also, on the enumerators, GetNext() increments the index even if it fails.
Can I get r/sr on the latest patch? dougt, peterlubczynski, jband?
Comment on attachment 55919 [details] [diff] [review] patch to xpcom shouldn't the nsAppDirectoryEnumerator hold a strong ref to the nsIDirectoryServiceProvider? I do not understand nor like having JAVA JRE or 4DOTX_PLUGINS_DIR defines in XPCOM. Why can't these defines live in OJI and plugins respectively? What is the point of nsSystemDirEnumeratorMac? Isn't there only one system directoy on the mac?
Doug, I could make it hold a strong ref. I didn't because this provider is more or less guarenteed to have the lifespan of xpcom itself and the lifespan of an iterator is usually short. JAVA JRE and 4DOTX_PLUGINS_DIR have to be defined where they are if those locations are to be provided by directory service. It makes getting file locations easy and consistent to only use directory service. nsSystemDirEnumeratorMac enumerates any set of directories that can be found by ::FindFolder() i.e. "System" directories.
> JAVA JRE and 4DOTX_PLUGINS_DIR have to be defined where they are I think that I am suggestiong that JRE and the modules/plugin code register these with the nsIDirectoryService when they are started up. Do clients these these directories prior to startup of these components?
> I think that I am suggestiong that JRE and the modules/plugin code register > these with the nsIDirectoryService when they are started up. If anything else at some point might want access to these dirs, they would need to be defined as I have them. Since that's unlikely, esp. for 4DOTX_PLUGINS_DIR, I can put this code back inside the plugin loader, using a provider which the plugin loader creates and registers.
I'll say it again... *If* you want xpti to see it then it better already be there at xpcom init time.
The patch looks OK to me. If it were me I'd leave out aNumKeys/mMaxIndex from the enumerator and just require a null terminated key list. This makes the HasMoreElements logic simple enough to just inline in GetNext also. I'm good for an sr=jband either way.
yeah. if it has to be this way, it has to be this way. r=me.
Wait, didn't we decide that the XPCOM-client application was supposed to set up these additional paths? We should be pushing NPX_PLUGIN_PATH and the JRE stuff into nsAppRunner or our initialization path, as part a LIST/nsISimpleEnumerator value provided by our app, not baking them into XPCOM (nsIDirectoryService) itself.
Comment on attachment 55919 [details] [diff] [review] patch to xpcom I feel very strongly that we shouldn't have these highly app-specific paths in our XPCOM interfaces, and should instead push the addition of these paths out to apprunner/etc.
Attachment #55919 - Flags: needs-work+
> Wait, didn't we decide that the XPCOM-client application was supposed to set > up these additional paths? It can define any number of additional paths under the NS_APP_PLUGINS_DIR_LIST key. Question is, whether NS_WIN_JAVA_JRE_DIR and NS_WIN_4DOTX_PLUGINS_DIR should be scanned by the plugin loader (modules/plugin) no matter which application is using that component. I think this would be true for NS_WIN_JAVA_JRE_DIR and that's why it's provided by xpcom's directory service provider. NS_WIN_4DOTX_PLUGINS_DIR may only be of interest to mozilla/ns6 so it could be provided by the application directory service provider. The reason NS_WIN_4DOTX_PLUGINS_DIR can't be returned as an element of NS_APP_PLUGINS_DIR_LIST has to do with scanning order. Since 4x plugins are scanned last and we are dropping support for this anyway, they're defined as an individual key. If this can't be resolved tonight, I still want to check in everything but the code for supporting NS_WIN_JAVA_JRE_DIR & NS_WIN_4DOTX_PLUGINS_DIR. Having NS_APP_PLUGINS_DIR_LIST and the ability to have directory service return lists of items fits the requirements for this bug. The rest can be argued later.
Attachment #55919 - Attachment is obsolete: true
The new patch removes mention of NS_WIN_JAVA_JRE_DIR and NS_WIN_4DOTX_PLUGINS_DIR from xpcom. Since those are only used by modules/plugin, they will be provided privately within there. The patch is identical to the last one but with cod removed. nsSpecialSystemDirectory.h/.cpp are no longer in the patch. The rest is just removing occurances of those keys. Ready for r/sr
Comment on attachment 56220 [details] [diff] [review] smaller patch without objectionable keys looks good.
Attachment #56220 - Flags: review+
Comment on attachment 56220 [details] [diff] [review] smaller patch without objectionable keys sr=jband
Attachment #56220 - Flags: superreview+
Checked in. This patch won't actually do anything until bug 77231 is checked in. I'll be updating that patch to account for the fact that NS_WIN_JAVA_JRE_DIR and NS_WIN_4DOTX_PLUGINS_DIR are not provided by xpcom.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
There were several minor conflicts because of changes to these files on the trunk.
nominating for edt0.9.4
Keywords: edt0.9.4
dougt, jband - sorry to ask again, but can you r=/sr= the patch for 0.9.4?
Comment on attachment 57016 [details] [diff] [review] same as final patch but for 0.9.4 make sure it builds xp, especially windows and mac since most of the deltas between the diffs were in #include files.
Attachment #57016 - Flags: review+
Comment on attachment 57016 [details] [diff] [review] same as final patch but for 0.9.4 sr=jband
Attachment #57016 - Flags: superreview+
Reopening - waiting for this fix to appear on the 0.9.4 branch
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
approved. please add "fixed0.9.4" to the keyword field once this hits 0.9.4.
Keywords: edt0.9.4 → edt0.9.4+
Checked into 0.9.4
Status: REOPENED → RESOLVED
Closed: 24 years ago → 24 years ago
Keywords: edt0.9.4+ → fixed0.9.4
Resolution: --- → FIXED
retroactive pixie dust
Keywords: edt0.9.4+
Verified code fix
Status: RESOLVED → VERIFIED
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: