Benjamin Smedberg Assignee
	Description • 19 years ago

We've had for a long time the problem that on unix/mac you have to set
(DY)?LD_LIBRARY_PATH *before* you launch your process if you intend to embed
XPCOM/gecko. This is bad, and I finally have a workable solution:

When the embedder finds a compatible glue and calls XPCOMGlueStartup(), the glue
will first look for a dependent-libs.list and manually load each of these libs
in order, before it loads the xpcom sharedlib itself.

I intend as part of this patch to drop all the dependentLibs stuff that is in
nsIGenericFactory, as it has never worked the way it was intended and will no
longer be needed.

	Doug Turner (:dougt)
	Comment 1 • 19 years ago

on linux, i thought that we fixed this problem with dependent libraries in our
compreg.dat file.

	Benjamin Smedberg Assignee
	Comment 2 • 19 years ago

That only fixes the problem if 1) compreg.dat doesn't go away and 2) you have
LD_LIBRARY_PATH set up the first time you autoregister, both of which are faulty
assumptions in most embedding context and in toolkit applications.

	Darin Fisher
	Comment 3 • 19 years ago

dependent-libs.list sounds like a good solution to me.  it also seems a bit more
optimal since we will only need to process it once.

	Benjamin Smedberg Assignee
	Comment 5 • 19 years ago

Just a note that this patch does not add the dependentlibs.list file itself: I
need to do that from the xpcom/stub makefile or somewhere else useful, but it
depends on libxul/notlibxul/static/not-static so it's not a simple file-copy
operation.

	Benjamin Smedberg Assignee
	Comment 6 • 19 years ago

Comment on attachment 188442 [details] [diff] [review]
Load dependencies explicitly from dependendlibs.list instead of relying on LD_LIBRARY_PATH, rev. 1

Needs a bit more work.

	Darin Fisher
	Comment 7 • 19 years ago

You sure you don't want to find some way to just compile prlink.c into the
standalone xpcom glue?

	Benjamin Smedberg Assignee
	Comment 8 • 19 years ago

I looked at that possibility but trying to remove all the depenencies that
prlink has on other NSPR stuff (locks, string handling, etc) make the task
gargantuan compared to this.

	Darin Fisher
	Comment 10 • 19 years ago

OK, it sounds like you've made the right choice then w.r.t. duplicating cross-
platform load library code.  It would probably be good to find some way to
simply not compile embedding support (xpcomglue) on platforms that lack an
implementation of this GRE loading stuff.  Otherwise, we will just have a bunch
of broken Firefox ports when Firefox doesn't even depend on this stuff.  Or do
you have some other plan for getting this ported everywhere?

	Benjamin Smedberg Assignee
	Comment 11 • 19 years ago

The dynamic linking code is not hard to write: I figured that breaking various
ports would get them to write the correct code, plus I plan to notify various
port owners before landing this. I can send out that email now if the general
approach in this patch looks correct.

	Darin Fisher
	Comment 12 • 19 years ago

Comment on attachment 188447 [details] [diff] [review]
Load dependencies explicitly from dependendlibs.list instead of relying on LD_LIBRARY_PATH, rev. 1.1

>Index: xpcom/glue/standalone/nsGlueLinking.h

>+#ifndef nsGlueLinking_h__
>+#define nsGlueLinking_h__
>+
>+#include "nsXPCOMPrivate.h"
>+#include <stdio.h>
>+
>+#define XPCOM_DEPENDENT_LIBS_LIST "dependentlibs.list"
>+
>+NS_HIDDEN_(GetFrozenFunctionsFunc)
>+XPCOMGlueLoad(const char *xpcomFile);
>+
>+NS_HIDDEN_(void)
>+XPCOMGlueUnload();
>+
>+typedef void (*DependentLibsCallback)(const char *aDependentLib);
>+
>+NS_HIDDEN_(void)
>+XPCOMGlueLoadDependentLibs(const char *xpcomDir, DependentLibsCallback cb);
>+
>+#endif // nsGlueLinking_h__

It looks like you can remove the #include <stdio.h> from this
header file.  Also, it is usually a good idea to provide a
|void*| data parameter along with any callback function.  That
way future implementations could pass additional state along
to the callback function.  This isn't a requirement here since
this API could be changed in the future if needed, but I just
thought I'd mention it anyways.


>Index: xpcom/glue/standalone/nsGlueLinkingDlopen.cpp

>+static void
>+ReadDependentCB(const char *aDependentLib)
>+{
>+    void *libHandle = dlopen(aDependentLib, RTLD_GLOBAL | RTLD_LAZY);
>+    if (!libHandle)
>+        return;
>+
>+    AppendDependentLib(libHandle);
>+}

Maybe this function should output a warning in debug builds
when dlopen fails.


>+
>+GetFrozenFunctionsFunc
>+XPCOMGlueLoad(const char *xpcomFile)
>+{
>+    char xpcomDir[MAXPATHLEN];
>+    if (realpath(xpcomFile, xpcomDir)) {

According to the man pages on my linux system (based on redhat 9),
the second parameter to realpath should be PATH_MAX bytes long.
Is MAXPATHLEN equal to PATH_MAX?  The man page mentions that on
solaris the value should be MAXPATHLEN instead of PATH_MAX FWIW.


>+        char *lastSlash = strrchr(xpcomDir, '/');
>+        if (lastSlash) {
>+            *lastSlash = '\0';
>+
>+            XPCOMGlueLoadDependentLibs(xpcomDir, ReadDependentCB);
>+        }
>+    }
>+
>+    void *libHandle = RTLD_DEFAULT;
>+
>+    if (xpcomFile[0] != '.' || xpcomFile[1] != '\0') {
>+        libHandle = dlopen(xpcomFile, RTLD_GLOBAL | RTLD_LAZY);
>+        if (libHandle) {
>+            AppendDependentLib(libHandle);
>+        }

this looks a lot like ReadDependentCB.	should that function
maybe be called here?  though you would then need a way to
find out if dlopen succeeded.  maybe ReadDependentCB should be
called LoadDependentLib and return a status code.


>+        else {
>+            libHandle = RTLD_DEFAULT;
>+        }
>+    }
>+
>+    // XXXbsmedberg: some OSes prefix C symbols with "_", how do we know?
>+    GetFrozenFunctionsFunc sym =
>+        (GetFrozenFunctionsFunc) dlsym(libHandle, "NS_GetFrozenFunctions");

According to NSPR's prlink.c:

  /*
   * On these platforms, symbols have a leading '_'.
   */
  #if defined(SUNOS4) || defined(DARWIN) || defined(NEXTSTEP) \
      || defined(WIN16) || defined(XP_OS2) \
      || ((defined(OPENBSD) || defined(NETBSD)) && !defined(__ELF__))
  #define NEED_LEADING_UNDERSCORE
  #endif

Perhaps you should just duplicate the logic from prlink.c


>Index: xpcom/glue/standalone/nsGlueLinkingMac.cpp

Our low-level code tends to use the suffix "Mac" to mean OS9
and "OSX" to mean OSX.	Should we do the same here?



>Index: xpcom/glue/standalone/nsGlueLinkingWin.cpp

>+// like strpbrk but finds the *last* char, not the first
>+static char*
>+ns_strrpbrk(char *string, const char *strCharSet)
>+{
>+    char *found = NULL;
>+    for (; *string; ++string) {
>+        for(const char *search = strCharSet; *search; ++search) {

nit: space between "for" and "("

>+            if (*search == *string) {
>+                found = string;
>+            }

add a comment here explaining that you are going to keep 
searching until the end of the string.	(it might make
sense to compute the length of the input string up front
and then search the string in reverse order, but this is
probably fine.)


>Index: xpcom/glue/standalone/nsXPCOMGlue.cpp

>+void
>+XPCOMGlueLoadDependentLibs(const char *xpcomDir, DependentLibsCallback cb)
>+{
>+    char buffer[MAXPATHLEN];
>+    sprintf(buffer, "%s" XPCOM_FILE_PATH_SEPARATOR XPCOM_DEPENDENT_LIBS_LIST,
>+            xpcomDir);
> 
>+    FILE *flist = fopen(buffer, "r");
>+    if (!flist)
>+        return;
> 
>+    while (fgets(buffer, sizeof(buffer), flist)) {
>+        char *nl = strpbrk(buffer, "\n\r");

hrm... i thought that the win32 version of fgets converted
\r\n to \n, no?  you didn't pass the "b" flag to fopen, so
i think calling strpbrk here is unnecessary.  you should be
able to just test the last char of buffer for \n.


>+        if (nl)
>+            *nl = '\0';
> 
>+        char buffer2[MAXPATHLEN];
>+        sprintf(buffer2, "%s" XPCOM_FILE_PATH_SEPARATOR "%s",
>+                xpcomDir, buffer);

use snprintf.

	Darin Fisher
	Comment 13 • 19 years ago

(In reply to comment #11)
> The dynamic linking code is not hard to write: I figured that breaking various
> ports would get them to write the correct code, plus I plan to notify various
> port owners before landing this. I can send out that email now if the general
> approach in this patch looks correct.

Yes, I like your solution to this problem.  Notify ports owners sooner rather
than later.  How about implementing a NULL version of these functions that
simply do nothing?  That way ports can use those versions to keep Firefox useful
until the ports authors have a chance to implement these functions.

	Benjamin Smedberg Assignee
	Comment 15 • 19 years ago

I didn't display load warnings when dlopen failed because I'm pretty certain
there were cases where I expected dlopen to fail but the general load to
succeed. I'm hard-pressed to think of one right now.

	Benjamin Smedberg Assignee
	Comment 16 • 19 years ago

Oh, and I fixed the "for(" in my tree (trying to keep multiple platforms in sync).

	Benjamin Smedberg Assignee
	Comment 17 • 19 years ago

If you're applying this patch, I forgot to remove the #include
"nsINativeComponentLoader.h" from nsNativeComponentLoader.h

	Darin Fisher
	Comment 19 • 19 years ago

(In reply to comment #15)
> I didn't display load warnings when dlopen failed because I'm pretty certain
> there were cases where I expected dlopen to fail but the general load to
> succeed. I'm hard-pressed to think of one right now.

Perhaps add a comment explaining this?

	Benjamin Smedberg Assignee
	Comment 20 • 19 years ago

I checked the main patch in, and it caused some problems on a couple of -ports
where RTLD_DEFAULT is not defined (non-GNU toolchains, apparently?). In any
case, RTLD_DEFAULT is "(void*) 0" so I feel safe in using nsnull directly.

	neil@parkwaycc.co.uk
	Comment 21 • 19 years ago

I (RH7.3 gcc2.96) don't have RTLD_DEFAULT either.

	Robert Kaiser
	Comment 23 • 19 years ago

According to bug 301043 comment #4, this has busted suite windows installer
builds. How can we get that fixed?

	Philip K. Warren
	Comment 24 • 19 years ago

AIX supports dlopen/dlclose, so all that is needed to add support is to toggle
it in the makefile.

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 25 • 19 years ago

+ifeq (,$(filter-out WINNT WINCE,$(OS_ARCH)))
+DEPENDENT_LIBS_LIST += js$(MOZ_BITS)$(VERSION_NUMBER)$(DLL_SUFFIX)
+else
+DEPENDENT_LIBS_LIST += $(LIB_PREFIX)mozjs$(DLL_SUFFIX)

why this, in an xpcom directory?

	Philip K. Warren
	Comment 26 • 19 years ago

AIX fix checked in. Thanks for the review.

Checking in Makefile.in;
/cvsroot/mozilla/xpcom/glue/standalone/Makefile.in,v  <--  Makefile.in
new revision: 1.22; previous revision: 1.21
done

	Darin Fisher
	Comment 27 • 19 years ago

Comment on attachment 189411 [details] [diff] [review]
Generate dependentlibs.list, rev. 1

r=darin

	Benjamin Smedberg Assignee
	Comment 28 • 19 years ago

The dependency chain is xpcom.dll -> xul.dll -> js3240.dll

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 29 • 19 years ago

ok hm, could that be generated from the libxul makefile then?

	Benjamin Smedberg Assignee
	Comment 30 • 19 years ago

Checked in the dependentlibs.list patch.