297923 - Write GRE-finding code on macosx and make the standalone glue work

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: GRE Core, defect)

Description

[Benjamin Smedberg]

There might be an old bug on both or either of these problems, but I have a
patch in progress and I'm just going to stick it here and dup old bugs as necessary:

The standalone XPCOM glue does not work on mac because it uses PR_LoadLibrary
which only loads bundles, not dylibs. This can be fixed by using NSAddImage. I
have code in my tree which does this currently, and it correctly links to XPCOM,
but then there are problems after I call NS_InitXPCOM2 with system calls
crashing (CFURLGetFileSystemRepresentation or something similar, in the sequence
of calls at
http://lxr.mozilla.org/mozilla/source/xpcom/io/nsDirectoryService.cpp#202).

My GRE-finding strategy on mac is significantly different from unix, and is
based on a plan to ship xulrunner/libxul as a framework: the GRE finder will
look in the following locations for compatible GREs:

some override environment variable for testing purposes
<application bundle>/Contents/Frameworks/XUL
(don't version-check this, assuming that if it shipped with the app it's the
correct version)
~/Library/Frameworks/XUL/Versions/<version>
/Library/Frameworks/XUL/Versions/<version>

I still need to investigate

a) whether changes to DYLD_LIBRARY_PATH are effective dynamically or only when
the app launches
b) the system-call crashes

Comment 1

[Benjamin Smedberg]

Attachment: Build XULRunner as a framework named "XUL" and find it in the GRE-finding code, rev. 1

Comment 2

[jhp (no longer active)]

Comment on attachment 186621 [details] [diff] [review]
Build XULRunner as a framework named "XUL" and find it in the GRE-finding code, rev. 1

+    if ((xpcomFile[0] != '.' || xpcomFile[1] != '\0')) {
+	 (void) NSAddImage(xpcomFile,
+			   NSADDIMAGE_OPTION_RETURN_ON_ERROR |
+			   NSADDIMAGE_OPTION_WITH_SEARCHING |
+			   NSADDIMAGE_OPTION_MATCH_FILENAME_BY_INSTALLNAME);
+	 // We don't really care if this fails, as long as we can get
+	 // NS_GetFrozenFunctions below.
+    }
+
+    if (!NSIsSymbolNameDefined("_NS_GetFrozenFunctions"))
+	 return NS_ERROR_FAILURE;
+
+    NSSymbol sym = NSLookupAndBindSymbol("_NS_GetFrozenFunctions");
+    function = (GetFrozenFunctionsFunc) NSAddressOfSymbol(sym);

I don't understand why |PR_LoadLibrary()| would fail.  If I'm reading this
correctly, it should eventually call |pr_LoadViaDyld()|, which itself makes the
call to |NSAddImage()| if the initial lookup fails
(http://lxr.mozilla.org/mozilla/source/nsprpub/pr/src/linking/prlink.c#888). 
And if |PR_LoadLibrary()| really is broken, wouldn't it be better to fix it in
NSPR than in nsXPCOMGlueCode.cpp?

+	ln -s Versions/Current/libxpcom.so
$(DIST)/$(FRAMEWORK_NAME).framework/libxpcom.so
That should be ".dylib".

Comment 3

[Benjamin Smedberg]

jhp: I need the NSADDIMAGE_* flags, which NSPR doesn't provide, but in addition
I am weaning this code off of NSPR altogether (see bug 298047).

Comment 4

[jhp (no longer active)]

OK, in that case:

The documentation for |NSIsSymbolNameDefinedInImage()| says it is deprecated in
favor of |NSLookupSymbolInImage()|, who's documentation in turn says to use
|dlsym()| for "portability and efficiency".  Do you think it's worthwhile using
|dlopen()| and |dlsym()| instead?

Otherwise, everything else looks good to me.

Comment 5

[Benjamin Smedberg]

I'm pretty sure I tried to use dlopen/dlsym and was unsuccessful, probably
because of the same INSTALLNAME flag issues. I'm happy to use
NSLookupSymbolInImage if that's preferred.

Comment 6

[jhp (no longer active)]

Comment on attachment 186621 [details] [diff] [review]
Build XULRunner as a framework named "XUL" and find it in the GRE-finding code, rev. 1

I'd say if using |NSLookupSymbolInImage()| isn't too much trouble, then it
would be better.  But it's no biggie.

Comment 7

[Darin Fisher]

Comment on attachment 186621 [details] [diff] [review]
Build XULRunner as a framework named "XUL" and find it in the GRE-finding code, rev. 1

>Index: xpcom/glue/nsGREGlue.cpp

>+#ifdef XP_MACOSX
>+  // Check the bundle first, for <bundle>/Contents/Frameworks/XUL/libxpcom.dylib
>+  CFBundleRef appBundle = CFBundleGetMainBundle();
>+  if (appBundle) {
>+    CFURLRef fwurl = CFBundleCopyPrivateFrameworksURL(appBundle);
>+    if (fwurl) {
>+      CFURLRef xulurl =
...
>+          if (CFURLGetFileSystemRepresentation(xpcomurl, PR_TRUE,
>+                                               (UInt8*) aBuffer, aBufLen) &&
>+              access(aBuffer, R_OK | X_OK) == 0)
>+            return NS_OK;
...
>+        CFRelease(xulurl);
...
>+      CFRelease(fwurl);

Does that "return NS_OK" result in a leak of xulurl and fwurl?


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

>@@ -157,17 +157,16 @@ GRE_GetCurrentProcessDirectory(char* buf
>                 {
>                     CFStringGetCString(path, buffer, MAXPATHLEN, kCFStringEncodingUTF8);
>                     CFRelease(path);
>                 }
>                 CFRelease(parentURL);
>             }
>             CFRelease(bundleURL);
>         }
>-        CFRelease(appBundle);

Why are you removing this line?  Does CFBundleGetMainBundle()
return a global resource or something that doesn't need to be
released?


sr=darin

Comment 8

[Benjamin Smedberg]

Comment on attachment 186621 [details] [diff] [review]
Build XULRunner as a framework named "XUL" and find it in the GRE-finding code, rev. 1

I went through again, and NSLookupSymbolInImage does not behave the same way on
10.2 that it does on other versions: we need the deprecated code as listed.

Comment 9

[Benjamin Smedberg]

Comment on attachment 186621 [details] [diff] [review]
Build XULRunner as a framework named "XUL" and find it in the GRE-finding code, rev. 1

a=chofmann from bens system

Comment 10

[Benjamin Smedberg]

Fixed for 1.8b3.