The default bug view has changed. See this FAQ.

add Universal Binary (mach-o FAT) plugin support

RESOLVED FIXED

Status

()

Core
Plug-ins
--
critical
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Josh Aas, Assigned: Josh Aas)

Tracking

Trunk
PowerPC
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

12 years ago
We crash trying to use Universal Binary plugins on Mac OS X. We need to learn to
deal with them.
(Assignee)

Comment 1

12 years ago
Created attachment 191618 [details] [diff] [review]
prlink mac cleanup, v1.0

This is a somewhat large patch, but it does only one specific thing - removes
XP_MAC code from prlink.c. Should be easy to review. I have tested it,
everything seems to work. The #idfef stuff in prlink.c is really confusing, and
removing it will make future patches easier to write and review.
Attachment #191618 - Flags: review?(wtchang)

Comment 2

12 years ago
Comment on attachment 191618 [details] [diff] [review]
prlink mac cleanup, v1.0

If it's appropriate to remove XP_MAC from NSPR, I've got two suggestions:

Get rid of this:
 #define PStrFromCStr(src, dst) c2pstrcpy(dst, src)

and just use c2pstrcpy directly (remember to swap the arguments).

and

+#if defined(XP_MACOSX)

Use #ifdef x as shorthand for #if defined(x) when the logic is no more complex
than this.
(Assignee)

Comment 3

12 years ago
Comment on attachment 191618 [details] [diff] [review]
prlink mac cleanup, v1.0

For some reason this patch won't apply any more for me. Cancelling review.
Attachment #191618 - Attachment is obsolete: true
Attachment #191618 - Flags: review?(wtchang)

Comment 4

12 years ago
Comment on attachment 191618 [details] [diff] [review]
prlink mac cleanup, v1.0

I already reviewed this patch.	I will be happy to check it
in after you get any necessary approval.  I'd like to make
some additional changes.

1. Remove some blank lines that were there to separate the
#ifdef blocks.	Now that the #ifdef's are moved, those blank
lines are no longer necessary.

2. Remove a block of comment in pr_LoadViaCFM that no longer
makes sense because the comment ("The name did contain a ":",
so it must be a full path name. ...") refers to code that has
been removed.

3. In pr_LoadViaCFM, remove unnecessary braces and move the code
to the left.

I want you to review another change I want to make:

> static PRStatus
> pr_LoadViaCFM(const char *name, PRLibrary *lm)
> {
>     OSErr err;
>     char cName[64];
...
>         p2cstrcpy(cName, fileSpec.name);
...
>     return (err == noErr) ? PR_SUCCESS : PR_FAILURE;
> }

I believe "cName" can be removed because it is created
(by the p2cstrcpy call) but not used.  Do you agree?
Attachment #191618 - Flags: review+

Comment 5

12 years ago
I will make the two changes that Mark suggested in comment 2.

Comment 6

12 years ago
Created attachment 191756 [details] [diff] [review]
Mark and Wan-Teh's suggested changes (cvs diff -w)

OK, here is a patch (whitespace ignored, so the indentation
of some code won't look right) that will help you understand
my suggested changes.  It also has the two changes Mark
suggested in comment 2.

Note that I expand SYM_OFFSET to 1 because NEED_LEADING_UNDERSCORE
is defined for DARWIN, hence for XP_MACOSX.
Attachment #191756 - Flags: review?(joshmoz)

Comment 7

12 years ago
Created attachment 191760 [details] [diff] [review]
Patch for prlink.h to reflect removed code

This patch adds comments to the obsolete enum constants
and union members that are only used by Mac OS Classic.
We can't remove the obsolete union members because that
will break binary compatibility (the union's size will
change).
Attachment #191760 - Flags: review?(joshmoz)
(Assignee)

Comment 8

12 years ago
Comment on attachment 191756 [details] [diff] [review]
Mark and Wan-Teh's suggested changes (cvs diff -w)

Looks good to me. So you will land both my patch (do you need a new one?) and
yours once I get approval for them?
Attachment #191756 - Flags: review?(joshmoz) → review+
(Assignee)

Updated

12 years ago
Attachment #191760 - Flags: review?(joshmoz)
Attachment #191760 - Flags: review+
Attachment #191760 - Flags: approval1.8b4?
(Assignee)

Updated

12 years ago
Attachment #191756 - Flags: superreview?(sfraser_bugs)
(Assignee)

Updated

12 years ago
Attachment #191618 - Attachment is obsolete: false
Attachment #191618 - Flags: approval1.8b4?

Comment 9

12 years ago
Comment on attachment 191760 [details] [diff] [review]
Patch for prlink.h to reflect removed code

Strictly speaking, the comments should refer to "Mac CFM", not "Mac OS Classic"
Attachment #191760 - Flags: superreview+

Comment 10

12 years ago
Comment on attachment 191756 [details] [diff] [review]
Mark and Wan-Teh's suggested changes (cvs diff -w)

>--- prlink-josh.c	2005-08-05 17:12:53.968750000 -0700
>+++ prlink.c	2005-08-05 17:13:45.812500000 -0700

>-#if defined(XP_MACOSX)
>+#ifdef XP_MACOSX
> #include <CodeFragments.h>
> #include <TextUtils.h>
> #include <Types.h>
> #include <Aliases.h>
>-
> #include <CFURL.h>
> #include <CFBundle.h>
> #include <CFString.h>
> #include <CFDictionary.h>
> #include <CFData.h>

Replace all these with <Carbon/Carbon.h> ?

>-#if defined(XP_MACOSX)
>-#if defined(NEED_LEADING_UNDERSCORE)
>-#define SYM_OFFSET 1
>-#else
>-#define SYM_OFFSET 0
>-#endif
>+#ifdef XP_MACOSX
>     if (lm->bundle) {
>-        CFStringRef nameRef = CFStringCreateWithCString(NULL, name + SYM_OFFSET, kCFStringEncodingASCII);
>+        CFStringRef nameRef = CFStringCreateWithCString(NULL, name + 1, kCFStringEncodingASCII);

I'd like to see a comment explaining "name + 1". I presume this is to skip the
underscore? Have we already tested that strlen(name) > 1 ?
Attachment #191760 - Flags: approval1.8b4? → approval1.8b4+
Attachment #191618 - Attachment is obsolete: true
Attachment #191618 - Flags: approval1.8b4?
(Assignee)

Comment 11

12 years ago
This bug needs to be a blocker. We will need to do this on the branch at some
point, since x86 Macs and plugins for them will be shipping before we move off
of the branch. If plugin vendors move to UB plugins, we will be broken on PPC as
well as x86. I'm working on this now.
Flags: blocking1.8b4?
(Assignee)

Comment 12

12 years ago
Created attachment 191903 [details] [diff] [review]
fix v1.0

Don't use CFM function pointers on Mac OS X x86. Allows UB plugins to work on
x86.
Attachment #191903 - Flags: review?(sfraser_bugs)
Comment on attachment 191903 [details] [diff] [review]
fix v1.0

r=pink
Attachment #191903 - Flags: review?(sfraser_bugs) → review+
(Assignee)

Updated

12 years ago
Attachment #191903 - Flags: superreview?(sfraser_bugs)

Comment 14

12 years ago
Comment on attachment 191903 [details] [diff] [review]
fix v1.0

So in tree I see __POWERPC__ and __powerpc__, but not __PowerPC__. Which is it?
(Assignee)

Comment 15

12 years ago
Its POWERPC, or at least that has been tested and works for me. I'll fix casing
on checkin.
(Assignee)

Comment 16

12 years ago
I should note though, that both __PowerPC__ and __POWERPC__ work for me, so my
patch isn't technically wrong.

Comment 17

12 years ago
Comment on attachment 191903 [details] [diff] [review]
fix v1.0

sr with

+#ifdef XP_MACOSX && __PowerPC__

changed to 

+#if defined(XP_MACOSX) && defined(__POWERPC__)
Attachment #191903 - Flags: superreview?(sfraser_bugs) → superreview+
(Assignee)

Updated

12 years ago
Attachment #191903 - Flags: approval1.8b4?

Comment 18

12 years ago
(In response to comment 14:)
> So in tree I see __POWERPC__ and __powerpc__, but not __PowerPC__. Which is it?

__ppc__ or __POWERPC__.  I prefer the former, it's more modern.  __powerpc__
doesn't work for Macs, only other operating systems.  __PowerPC__ is just broken.
(Assignee)

Comment 19

12 years ago
__ppc__ never works for me - I've tried it at your request a few times already,
so we can't do that

Updated

12 years ago
Attachment #191903 - Flags: approval1.8b4? → approval1.8b4+
(Assignee)

Comment 20

12 years ago
Landed. Marking this bug fixed.

WTC - that cleanup turns out not to be necessary for this bug fix. However, I'd
still like to see it cleaned up since those macros are really confusing for
those trying to read the code. Would you rather we continue to collect approvals
on this bug or open another one? Thanks for your help!
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 21

12 years ago
Comment on attachment 191903 [details] [diff] [review]
fix v1.0

>+#ifdef XP_MACOSX && __PowerPC__

Now sure if all compilers allow more text after the first
identifier after #ifder.  It is safer to write this as

    #if defined(XP_MACOSX) && __PowerPC__

or

    #if defined(XP_MACOSX) && defined(__PowerPC__)

Comment 22

12 years ago
(In reply to comment #21)
> (From update of attachment 191903 [details] [diff] [review] [edit])
> >+#ifdef XP_MACOSX && __PowerPC__
> 
> Now sure if all compilers allow more text after the first
> identifier after #ifder.  It is safer to write this as
> 
>     #if defined(XP_MACOSX) && __PowerPC__
> 
> or
> 
>     #if defined(XP_MACOSX) && defined(__PowerPC__)

It was landed as
#if defined(XP_MACOSX) && defined(__POWERPC__)

Updated

12 years ago
Flags: blocking1.8b4?

Comment 23

12 years ago
Created attachment 192005 [details] [diff] [review]
Combined patch for prlink.c (cvs diff -w)

This combined patch contains all the changes for prlink.c
proposed so far, except Simon's suggestion of including
<Carbon/Carbon.h> instead of the individual headers.

I restored the SYM_OFFSET macro and added a comment to
explain that the offset is added to skip the leading
underscore.  On Darwin, we always add a leading underscore
to the symbol's name before calling the static function
pr_FindSymbolInLib.

Updated

12 years ago
Attachment #191756 - Attachment is obsolete: true
Attachment #192005 - Flags: superreview?(sfraser_bugs)
Attachment #192005 - Flags: review?(joshmoz)

Comment 24

12 years ago
Comment on attachment 191760 [details] [diff] [review]
Patch for prlink.h to reflect removed code

Simon: I want an accurate comment, but a non-Mac developer
won't know what "CFM" is.  Even I have forgotten what CFM is.
Do you have any other suggestion?

Comment 25

12 years ago
Comment on attachment 192005 [details] [diff] [review]
Combined patch for prlink.c (cvs diff -w)

>Index: prlink.c
>===================================================================

> PR_IMPLEMENT(PRLibrary*)
> PR_LoadLibraryWithFlags(PRLibSpec libSpec, PRIntn flags)
> {
>     if (flags == 0) {
>         flags = _PR_DEFAULT_LD_FLAGS;
>     }
>     switch (libSpec.type) {
>         case PR_LibSpec_Pathname:
>             return pr_LoadLibraryByPathname(libSpec.value.pathname, flags);
>-#ifdef XP_MAC
>-        case PR_LibSpec_MacNamedFragment:
>-            return pr_Mac_LoadNamedFragment(
>-                libSpec.value.mac_named_fragment.fsspec,
>-                libSpec.value.mac_named_fragment.name);
>-        case PR_LibSpec_MacIndexedFragment:
>-            return pr_Mac_LoadIndexedFragment(
>-                libSpec.value.mac_indexed_fragment.fsspec,
>-                libSpec.value.mac_indexed_fragment.index);
>-#endif /* XP_MAC */

In theory these still apply for CFM ("Code Fragment Manager") libraries on Mac
OS X, but they're probably never been used.


> static void* TV2FP(CFMutableDictionaryRef dict, const char* name, void *tvp)
> {

We dont' want any of this TVector-wrapping stuff for i386, so it should be
#ifdef __POWERPC_.

> static PRStatus
> pr_LoadViaCFM(const char *name, PRLibrary *lm)

Should pr_LoadViaCFM be #ifdeffed for powerpc only?


>     static const macLibraryLoadProc loadProcs[] = {
>-#if defined(XP_MACOSX)
>         pr_LoadViaDyld, pr_LoadCFBundle, pr_LoadViaCFM
>-#elif TARGET_CARBON
>-        pr_LoadViaCFM, pr_LoadCFBundle
>-#else
>-        pr_LoadViaCFM
>-#endif

Take out pr_LoadViaCFM for intel?


>-#if defined(XP_MAC) || defined(XP_MACOSX)
>-#if defined(NEED_LEADING_UNDERSCORE)
>+#ifdef XP_MACOSX
>+/* add this offset to skip the leading underscore in name */
> #define SYM_OFFSET 1
>-#else
>-#define SYM_OFFSET 0
>-#endif
>-#if TARGET_CARBON
>     if (lm->bundle) {
>         CFStringRef nameRef = CFStringCreateWithCString(NULL, name + SYM_OFFSET, kCFStringEncodingASCII);
>         if (nameRef) {
>             f = CFBundleGetFunctionPointerForName(lm->bundle, nameRef);
>             CFRelease(nameRef);
>         }
>     }
>-#endif
>     if (lm->connection) {
>         Ptr                 symAddr;
>         CFragSymbolClass    symClass;
>         Str255              pName;
>         
>         PR_LOG(_pr_linker_lm, PR_LOG_MIN, ("Looking up symbol: %s", name + SYM_OFFSET));
>         
>-        PStrFromCStr(name + SYM_OFFSET, pName);
>+        c2pstrcpy(pName, name + SYM_OFFSET);

As far as I can see, the function doesn't check for empty "name", so this might
copy random memory.

Comment 26

12 years ago
Comment on attachment 192005 [details] [diff] [review]
Combined patch for prlink.c (cvs diff -w)

Simon:

Thanks for the code review.  I should explain the purpose
of this patch.	This patch is strictly to remove obsolete
code for the "XP_MAC" platform.  It is constructed with
the following assumptions:

1. The macro XP_MAC is never defined.
2. Whenever the macro XP_MACOSX is defined, the macro
   TARGET_CARBON is defined with the value 1.
3. Whenever the macro XP_MACOSX is defined, the macro
   NEED_LEADING_UNDERSCORE is also defined.  (This is
   true because we define NEED_LEADING_UNDERSCORE if
   DARWIN is defined.)

After the mechanical changes were made, I applied some
finishing touches.  I didn't try to fix any pre-existing
problems.

So, to save everyone's time, perhaps I should just ask
you to validate the three assumptions above, and then
ask you to review a smaller patch with just my "finishing
touches".  I don't want to waste your time in doublechecking
the mechanical changes.
(Assignee)

Comment 27

12 years ago
Comment on attachment 192005 [details] [diff] [review]
Combined patch for prlink.c (cvs diff -w)

r+ in the sense that I agree with your assumptions. I did not actually review
mechanical changes. Thanks for doing this!
Attachment #192005 - Flags: review?(joshmoz) → review+

Comment 28

12 years ago
Comment on attachment 192005 [details] [diff] [review]
Combined patch for prlink.c (cvs diff -w)

The patch is fine as "cleanup".
Attachment #192005 - Flags: superreview?(sfraser_bugs) → superreview+

Updated

12 years ago
Attachment #191756 - Flags: superreview?(sfraser_bugs)

Comment 29

12 years ago
Comment on attachment 192005 [details] [diff] [review]
Combined patch for prlink.c (cvs diff -w)

Requesting drivers' approval.

This is a code cleanup patch to eliminate dead code and
help future maintenance.  The dead code is removed by
searching for macros that are either never defined or
always defined.  After these mechanical changes are made,
I make a couple of "finishing touches" to remove an unused
variable and adjust some comments.  The risk of this patch
is extremely low.
Attachment #192005 - Flags: approval1.8b4?

Updated

12 years ago
Attachment #192005 - Flags: approval1.8b4? → approval1.8b4+
You need to log in before you can comment on or make changes to this bug.