Closed Bug 105440 Opened 23 years ago Closed 23 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: 23 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: 23 years ago → 23 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: