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

RESOLVED FIXED in mozilla1.8beta3

Status

Core Graveyard
Embedding: GRE Core
RESOLVED FIXED
13 years ago
2 years ago

People

(Reporter: Benjamin Smedberg, Assigned: Benjamin Smedberg)

Tracking

Trunk
mozilla1.8beta3
PowerPC
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

13 years ago
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
(Assignee)

Comment 1

13 years ago
Created attachment 186621 [details] [diff] [review]
Build XULRunner as a framework named "XUL" and find it in the GRE-finding code, rev. 1
Attachment #186621 - Flags: superreview?(darin)
Attachment #186621 - Flags: review?(jhpedemonte)
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".
(Assignee)

Comment 3

13 years ago
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).
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.
(Assignee)

Comment 5

13 years ago
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 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.
Attachment #186621 - Flags: review?(jhpedemonte) → review+

Comment 7

13 years ago
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
Attachment #186621 - Flags: superreview?(darin) → superreview+
(Assignee)

Comment 8

13 years ago
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.
Attachment #186621 - Flags: approval1.8b3?
(Assignee)

Comment 9

13 years ago
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
Attachment #186621 - Flags: approval1.8b3? → approval1.8b3+
(Assignee)

Comment 10

13 years ago
Fixed for 1.8b3.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.8beta3
(Assignee)

Updated

13 years ago
Depends on: 311352
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.