NSPR needs to support loading CFM, CFBundle and mach-o libraries

RESOLVED FIXED in 4.3

Status

P1
normal
RESOLVED FIXED
17 years ago
16 years ago

People

(Reporter: beard, Assigned: beard)

Tracking

PowerPC
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed on trunk & client branch])

Attachments

(2 attachments, 8 obsolete attachments)

(Assignee)

Description

17 years ago
Bug #131300 provides support for running CFM browser plugins on Mac OS X. NSPR
also needs to be enhanced to support this. Patches to follow.
(Assignee)

Comment 1

17 years ago
Created attachment 74430 [details] [diff] [review]
Patch to prlink.c v1

This enables loading of CFM and CFBundle format plugins.
(Assignee)

Comment 2

17 years ago
This needs some more work. There needs to be some kind of autoconf test to
#define XP_MACOSX when prlink.c is compiled. In addition, the Makefile needs to
define

OS_LIBS = -framework Carbon -framework CoreFoundation

not for Darwin, but for Mac OS X builds only.
(Assignee)

Updated

17 years ago
Blocks: 131300

Comment 3

17 years ago
Patrick,

As far as NSPR is concerned, what is the difference between
Darwin and Mac OS X?  It seems that NSPR only uses the Darwin
portion of Mac OS X, so I don't understand the distinction
you were trying to make.
Priority: -- → P1
Target Milestone: --- → 4.2
Wan-Teh, the problem is that Beard's patch uses parts of the Carbon toolkit
which is only available from OSX.  The standalone Darwin distributions will not
have access to that toolkit so we need a way to differentiate between the two. 

Beard, I think we should be able to just add a simple compile/link test using
'-framework Carbon' and set the define if the link succeeds.

 
Created attachment 74470 [details] [diff] [review]
Use presence of Carbon to denote OSX-ness

If the compile & link succeeds, we set OS_TARGET=MACOSX and add the define
-DXP_MACOSX=1.	We probably want to put something other than 'return 0;' in the
test prog though.
(Assignee)

Comment 6

17 years ago
Yes, Chris has it exactly right. Darwin is a standalone FreeBSD-derived kernel,
which won't have the Carbon frameworks available. One could also simply test for
the existence of the file:

/System/Library/Frameworks/Carbon.framework
Sure, you can make that assumption too.  I figured that if you're going to link
against it, you'd want to make sure that it worked before you started building.
 Anyway, your call.

Comment 8

17 years ago
Patrick, could you have someone review your patch?  Thanks.

Comment 9

17 years ago
r=hyatt
Created attachment 100962 [details] [diff] [review]
Combined patch that checks for the framework file
Attachment #74430 - Attachment is obsolete: true
Attachment #74470 - Attachment is obsolete: true
Created attachment 101533 [details] [diff] [review]
Fix classic mac bustage
Attachment #100962 - Attachment is obsolete: true

Comment 12

16 years ago
pr_LoadLibraryByPathname is a huge mess, and I think this would be a good time
to clean it up. Ideally, we should push the core of it down into 'md' code, and
remove all the #ifdefs. wtc?

-    PRLibrary *lm=NULL;
+q    PRLibrary *lm=NULL;

This looks like a finger fumble.
Created attachment 101669 [details] [diff] [review]
fix non-carbon classic builds
Attachment #101533 - Attachment is obsolete: true

Comment 14

16 years ago
Will a mach-o application still need to load CFM libraries
in 2003?

Which important browser plugins still don't have a mach-o
version?

Why don't we ask the browser plugin vendors to provide
mach-o browser plugins?

Comment 15

16 years ago
>Will a mach-o application still need to load CFM libraries in 2003?
Almost certainly.

>Which important browser plugins still don't have a mach-o version?
All of them. The MRJ Plugin is the only mach-o plugin that I know of.

Updated

16 years ago
Blocks: 176301

Comment 16

16 years ago
Comment on attachment 101669 [details] [diff] [review]
fix non-carbon classic builds

r=peterl
Attachment #101669 - Flags: review+

Comment 17

16 years ago
I plan to review this patch tomorrow.  I understand
the urgency of this patch and I apologize for my delay
in reviewing the patch.

Comment 18

16 years ago
Comment on attachment 101669 [details] [diff] [review]
fix non-carbon classic builds

I found that linking libnspr4.dylib with
-framework Carbon -framework CoreFoundation
causes NSPR test programs to fail to start
up in a command-line console window on Mac
OS X 10.1.5 with the these error messages:

kCGErrorIllegalArgument : initCGDisplayState: cannot map display interlocks.
kCGErrorIllegalArgument : CGSNewConnection cannot get connection port
kCGErrorIllegalArgument : CGSNewConnection cannot get connection port
kCGErrorInvalidConnection : CGSGetEventPort: Invalid connection

NSPR test programs work fine on in a
command-line console window Mac OS X
10.2.1.

Unless the problem on Mac OS X 10.1.5 can
be resolved, I am afraid that I can't take
this patch.  It is important that we be able
to run NSPR and NSS test programs in a
command-line console windows on Mac OS X.
Attachment #101669 - Flags: needs-work+

Comment 19

16 years ago
Also, all the non-debug printfs need to be #ifdeffed out.

Comment 20

16 years ago
Created attachment 104029 [details] [diff] [review]
fix non-carbon classic builds v2

I made some stylistic and build changes to attachment
101669 [details] [diff] [review].  All the debug printf statements have been
deleted.

I have some questions about the original patch.

1. In nsprpub/pr/src/linking/Makefile.in, we have:

+# on Darwin / Mac OS X link use flat #includes.
+ifeq ($(OS_ARCH)_$(OS_TARGET),Darwin_MACOSX)
+INCLUDES    += -I/Developer/Headers/FlatCarbon -I$(srcdir)/../md/mac/
+endif

What does that comment mean?

2. In nsprpub/pr/src/linking/prlink.c, we have

-	 memcpy(cName, fileSpec.name + 1, fileSpec.name[0]);
-	 cName[fileSpec.name[0]] = '\0';
+#if TARGET_CARBON
+	 p2cstrcpy(cName, fileSpec.name);
+#else	      
+	 strncpy(cName, p2cstr(fileSpec.name),sizeof(cName));
+#endif

Does this work with non-carbob classic builds?

3. Does this FIXME comment mean this patch is not
finished?

+	 if (err == noErr && lm->connection) {
+	     /* FIXME:	if we're a mach-o binary, need to wrap all CFM function
pointers. */
+	     /* will need a hash-table of already seen function pointers, etc.
*/
+	     lm->main = TV2FP(lm->main);
+	 }

4. In this code, it seems that the XP_MACOSX case needs to
set lm->dlh because _PR_InitLinker only sets
pr_exe_loadmap->dlh (under the code for XP_UNIX and
USE_MACH_DYLD).

+#if defined(XP_MAC) || defined(XP_MACOSX)
+    lm->connection  = pr_exe_loadmap ? pr_exe_loadmap->connection : 0;
+#else
     lm->dlh	     = pr_exe_loadmap ? pr_exe_loadmap->dlh : 0;
+#endif

So maybe this code should look like this?

+#if defined(XP_MAC)
+    lm->connection  = pr_exe_loadmap ? pr_exe_loadmap->connection : 0;
+#else
     lm->dlh	     = pr_exe_loadmap ? pr_exe_loadmap->dlh : 0;
+#endif
Attachment #101669 - Attachment is obsolete: true

Comment 21

16 years ago
Here is a simple test to demonstrate the startup
problem on Mac OS X 10.1.5 caused by linking with
the Carbon framework.

1. Save the following test program as hello.c.

-=-=-=-=-= hello.c -=-=-=-=-=
#include <stdio.h>

int main()
{
    printf("Hello world!\n");
    return 0;
}
-=-=-=-=-= hello.c -=-=-=-=-=

2. Compile and run it on Mac OS X 10.1.5. (Netscape
employees can telnet to hans.mcom.com or
makaila.mcom.com with your Unix account.)

Here is what I got.

hans:/u/wtc/tmp 328% cc hello.c
hans:/u/wtc/tmp 329% ./a.out
Hello world!
hans:/u/wtc/tmp 330% cc hello.c -framework Carbon
hans:/u/wtc/tmp 331% ./a.out
kCGErrorIllegalArgument : initCGDisplayState: cannot map display interlocks.
kCGErrorIllegalArgument : CGSNewConnection cannot get connection port
kCGErrorIllegalArgument : CGSNewConnection cannot get connection port
kCGErrorInvalidConnection : CGSGetEventPort: Invalid connection
hans:/u/wtc/tmp 332% uname -a
Darwin hans.nscp.aoltw.net 5.5 Darwin Kernel Version 5.5: Thu May 30 14:51:26 PD
T 2002; root:xnu/xnu-201.42.3.obj~1/RELEASE_PPC  Power Macintosh powerpc
hans:/u/wtc/tmp 333% cc -v
Reading specs from /usr/libexec/gcc/darwin/ppc/2.95.2/specs
Apple Computer, Inc. version gcc-934.3, based on gcc version 2.95.2 19991024 (re
lease)

Comment 22

16 years ago
I think linking with Carbon causes the framework to try to make a connection
with the window server at load time.
> +# on Darwin / Mac OS X link use flat #includes.
> +ifeq ($(OS_ARCH)_$(OS_TARGET),Darwin_MACOSX)
> +INCLUDES    += -I/Developer/Headers/FlatCarbon -I$(srcdir)/../md/mac/
> +endif
> What does that comment mean?

"In /Developer/Headers/FlatCarbon are stub files for all public Mac OS 9 header
files.   These stub files redirect the compiler to the appropriate umbrella
header file or contain warnings if the API is not valid on Mac OS X. " 
http://developer.apple.com/techpubs/macosx/Essentials/SystemOverview/Umbrella/Linking_and_g_Guideline.html

> 2. In nsprpub/pr/src/linking/prlink.c, we have
> Does this work with non-carbob classic builds?

My non-carbon build compiled, ran and loaded plugins.  I didn't extensively test
it though.

> 3. Does this FIXME comment mean this patch is not finished?

No idea.  Beard?
Created attachment 104078 [details] [diff] [review]
Use --enable-os-target=MacOSX to denote carbon build

I converted the --enable-win32-target option to be general enough to use as
--enable-os-target.  The original target was added for cross-compiling to
mingw32 which doesn't work anymore (requires the patches from bug 134113).  The
new option can work for the old purpose as well but it requires that you use
uppercase WINNT & WIN95 instead of translating the input.
Attachment #104029 - Attachment is obsolete: true

Comment 25

16 years ago
I filed a bug report with Apple about the
startup problem in a remote text terminal.
The problem ID is 3085140.  The problem
title is "Apps linked with the Carbon
framework fail to start up in a remote text
terminal."

Comment 26

16 years ago
Created attachment 104505 [details] [diff] [review]
Merged latest changes from CHIMERA_M1_0_1_BRANCH

Please review and test this patch.  (This needs
to be tested in Carbon and non-Carbon Classic
builds.)

This patch is based on the prlink.c on the
CHIMERA_M1_0_1_BRANCH (rev. 3.51.2.3.2.1.16.2)
and Chris Seawood's changes to fix Classic Mac
build bustage.

I did some editing to minimize the diffs against
the trunk and the CHIMERA_M1_0_1_BRANCH.  I am
allowing the debug printf statements for now.
(In general NSPR does not print debug messages.)

The problem with starting up a text-mode app in
a remote text terminal has been resolved.  My
sources at Apple told me that we should link with
the CoreServices framework instead of Carbon.
Attachment #104078 - Attachment is obsolete: true

Comment 27

16 years ago
Comment on attachment 104505 [details] [diff] [review]
Merged latest changes from CHIMERA_M1_0_1_BRANCH

Patrick,

You changed NSFindSymbol to FinsSymbol.  Is this a
safe change for the Carbon and non-Carbon Classic
builds?

Comment 28

16 years ago
Created attachment 104590 [details] [diff] [review]
Use FindSymbol in Mach-O build and NSFindSymbol in CFM builds

The only difference between this patch and the previous patch
is that we only call FindSymbol in the Mach-O build (XP_MACOSX)
and continue to call NSFindSymbol in the Carbon and non-Carbon
CFM builds.
Attachment #104505 - Attachment is obsolete: true
Comment on attachment 104590 [details] [diff] [review]
Use FindSymbol in Mach-O build and NSFindSymbol in CFM builds

I see no obvious side-effects in my classic, cfm carbon & mach-o builds.
r=cls
Attachment #104590 - Flags: review+
The patch has been checked into the NSPR trunk & CLIENT_BRANCH.
Whiteboard: [fixed on trunk & client branch]
Target Milestone: 4.2 → 4.2.2

Updated

16 years ago
Target Milestone: 4.2.2 → 4.3
(Assignee)

Comment 31

16 years ago
Please check for leaks, I think that CFDataCreateMutable() returns a
CFMutableDataRef that has initial refcount of 1, and that CFDictionaryAddValue()
adds another reference count. It looks to me that we should add a call to
CFRelease() after the call to CFDictionaryAddValue() in TV2FP().
drivers doesn't want this in on the 1.2 branch before the 1.2 release, it's too
high risk.  If you guys want to check it in post-1.2 on that branch for chimera
or something that's OK but I don't think we want it before 1.2.
Attachment #105950 - Flags: review?(wtc) → review?(beard)
(Assignee)

Comment 34

16 years ago
Comment on attachment 105950 [details] [diff] [review]
Fix CFData leak

r=beard
Attachment #105950 - Flags: review?(beard) → review+
Flags: blocking1.3a?
The fix has been checked into the NSPR trunk & the client branch.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Flags: blocking1.3a?
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.