104191 - xpti needs to support multiple directories

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Judson Valeski]

Currently xpti only scan's the components directory rooted in the bin directory.
This limits plugin installation by requiring the the xpt files that plugins need
for scriptability to be installed in the components dir, while the plugins
themselves are installed in the plugins directory.

Furthermore, applications sharing a plugin directory, will actually not be able
to share scriptable plugins, because the applications installed *after* the
plugin was installed, will not have the necessary xpt files in their components
directory. This problem is solved by xpti scanning multiple directories.

Once we have a dynamic directory dermination model (probably keys hanging off of
nsIDirectoryServiceProvider; this still needs to be hashed out), xpti needs to
be able to scan those directories as well.

The work to support multiple directories (this bug) can be done independently of
directory resolution.

Comment 1

[John Bandhauer]

> The work to support multiple directories (this bug) can be done independently 
> of directory resolution.

I'd prefix that with "Most of...".

I'm not clear on who might be working on the bigger picture. In what bug? Bug
14923 is billed as a "tracking bug".

There is one issue external to xpti I really want to work out up front... How is
the list of directories communicated to xpti and the other users of the list?

In bug 100368 dougt said:
............................................................................
spoke to conrad about this a bit.  We concluded that it would be best to just
have 3 defined directory properties:

NS_XPCOM_COMPONENT_DIR
NS_XPCOM_COMPONENT_USER_DIR
NS_XPCOM_COMPONENT_GLOBAL_DIR

Then have the embedding API call nsComponentManager::AutoRegister as it sees fit. 
............................................................................

I'm not thrilled about that. Hardcoding the "variable names" (and implicit
order) of the items to go into the search list is shortsighted. We have at least
two subsystems that are going to be scanning these directories (maybe 3 if
plugins needs to also?). What if some embedding wants a 4th directory? Bah!

I say that xpcom should have acces to an ordered set of nsIFiles that
constitutes the component search path. I don't really care what service does
this, but it should be there when anyone asks.

Also, (perhaps stating the obvious) we need to rememeber that this is not just a
startup issue and control does not always flow from the core xpcom to the other
autoreg'ers. Each of these subsystems can independently be asked to re-autoreg
at anytime.

If it was up to me, I'd add an additional interface to the object that implments
nsIDirectoryServiceProvider (since that interface is FROZEN). This new interface
would supply the ordered component search path - perhaps as an
nsISimpleEnumerator or nsISupportsArray whose elements are nsIFiles. If that new
interface is not present, then only NS_XPCOM_COMPONENT_DIR is used like it is now.

If we really feel the need to only use the existing nsIDirectoryServiceProvider
interface, then I'd still vote for something like asking it for 
NS_XPCOM_COMPONENT_DIR and then the series... "AdditionalComponentDir1" then
"AdditonalComponentDir2", and so on, until it returns null. This is butt ugly,
but at least it's flexible.

Comment 2

[Judson Valeski]

Because nsIDirectoryServiceProvider is frozen, we'd have to use a new iface and
because this seems so tightly coupled to nsIDirectoryServiceProvider, I'd be
inclined to call it nsIDirectoryServiceProvider2, just rev'ing the existing iface.

So, you're saying it's up to the nsIDirectoryServiceProvider impl to define the
precedence.? That seems vulnerable to me. Don't we want to distinguish between
"core"/"app" stuff and "add-ons", so XPCOM can ensure some level of control over
what's registered?

Comment 3

[Conrad Carlen (not reading bugmail)]

If any subsystem had to obtain a list of directories which could be scanned, it
could be slow. Imagine if this list grew to some large number in the future.
Would a component have to scan the whole list of directories? If each of the
additional directories is explicitly defined, and rules are made as to which
clients can scan where, it would avoid excessive directory scanning.

If we need to add another named directory in the future, we can - unless we are
saying that, along with the nsIDirectoryService iface, the list of locations is
frozen as well. I don't think it is.  

Comment 4

[John Bandhauer]

Attachment: most of the fix

Comment 5

[John Bandhauer]

The patch I attached does almost all of what needs to be done in the typelib 
loader to fix this bug.

xptiInterfaceInfoManager::BuildFileSearchPath is the method that will need to be 
updated to get the search path from wherever the search path is supposed to come 
from. In this patch that code gets the components directory and the plugins 
directory (using NS_XPCOM_COMPONENT_DIR and NS_APP_PLUGINS_DIR). We can change 
change this to whatever.

xptiInterfaceInfoManager::GetCloneOfManifestDir *could* be changed if we want to 
store xpti.dat in some place other than the components directory.

xpti figures out the search path and manifest directory at xpcom startup and 
caches the result. Whatever any given embedding wants to use for those 
directories *must* be available at that time. Any changes after that will be 
ignored. As things stand now, this precludes using anything based on the profile 
directory (at least in the mozilla browser embedding) because that is set later.

I've only tested this stuff on NT, but I think I got it right (using the 
PersistentDescriptor stuff to make Mac happy)

The patch was made against the 0.9.4 branch. I would expect it to apply to the 
trunk too.

Comment 6

[John Bandhauer]

Attachment: better patch

Comment 7

[John Bandhauer]

The patch I just attached is mostly the same as the one before. It adds a fix 
for bug 105042. And it disables the scanning of the plugins directory. So, this 
means that there should be not detectable a difference between this and current 
behavior. The patch works on the trunk too. The only additional change needed 
for the trunk is to add the string module to the REQUIRES lists in makefile.win 
and Makefile.in.

I'd like to get this reviewed as is and get it checked in to both the 
0.9.4 branch and the trunk. After that only a small change will be needed to 
support scanning whatever directories we decide to scan.

Comment 8

[Johnny Stenback (:jst)]

- You might wanto use the more typesafe CallQueryInterface() in
xptiCloneLocalFile() in stead of calling QueryInterface() directly.

- Do you really need the void** cast in xptiCloneElementAsLocalFile() when
calling QueryInterfaceAt()? Same in xptiWorkingSet::FindDirectory().

Either way, sr=jst

Comment 9

[Conrad Carlen (not reading bugmail)]

Overall, looks good. r=ccarlen.
One thing though:

+            xptiFile& file = mFileArray[i];
+            if(file.GetDirectory() == dir &&
+               0 == PL_strcasecmp(name, file.GetName()))

Is this right on file systems which are case-sensitive as well as those that aren't?

Comment 10

[John Bandhauer]

I updated the patch based on both reviewers' suggestions and checked it into the
trunk. I'm going to leave this bug open pending approval for checkin to the
0.9.4 branch. I'll attach the current patch to ensure that it does not get lost
in the shuffle. Thanks!

Comment 11

[John Bandhauer]

Attachment: patch for the 0.9.4 branch (as checked into the trunk)

Comment 12

[Jaime Rodriguez, Jr.]

Is this a regression, or was this issue on Netscape 6.1?

Comment 13

[Judson Valeski]

not a regression.

Comment 14

[Jaime Rodriguez, Jr.]

Can this wait to be checked in on the trunk, until next wednesday? If not, pls
call into the PDT at noon. Gonna PDT- for now, until we here this has to be in
branch before 10.23.

Comment 15

[John Bandhauer]

Attachment: refresh patch for 0.9.4 to avoid regressing bug 105437

Comment 16

[Marek Z. Jeziorek]

looks ready to be checked-in (once the branch is owned by the embedding team)

Comment 17

[Peter Lubczynski]

So I guess the plugin-side must now use nsIDirectoryService when doing scanning
so both will look in the same locations. Is there already a bug filed on this?

Comment 18

[Conrad Carlen (not reading bugmail)]

Yes. bug 77231.

Comment 19

[John Bandhauer]

Closing. Now also checked into 0.9.4 branch. a=valeski

Comment 20

[Phil Schwartau]

Marking Verified -