need a way to distinguish GREs based on architecture in nsGREGlue

RESOLVED FIXED in mozilla1.9beta5

Status

defect
RESOLVED FIXED
12 years ago
4 years ago

People

(Reporter: wolfiR, Assigned: wolfiR)

Tracking

Trunk
mozilla1.9beta5
x86
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

On some Linux distributions (64bit ones like x86-64, s390x and ppc64 with biarch support) you have the following situation:
The distribution installs a system wide xulrunner as 64bit version which registers in /etc/gre.d/*.conf and can also install a 32bit flavour of the same or different version which also has some conf file registered.

The stub can't decide now what GRE to use. A 32bit stub could end up trying to use a 64bit libxul or vice versa. Both doesn't work and would just throw an error.

Making the GREGlue aware of that situation wouldn't be too hard I think but all solutions I can think of will break compat for xul applications and xulrunners out there.
It could be based on a special property which holds the architecture, just a different naming scheme for conf files like /etc/gre.d/*-64.conf or a different GRE dir /etc/gre64.d.

Any ideas or proposals?
The glue already supports the ability to look for arbitrary "properties" attached to XR versions (GRE_GetGREPathWithProperties)... so I think the best answer here is:

* when registering a GRE, add a property to the registration file

ABI=x86_64-gcc3
ABI=x86-gcc3

These values are taken from http://developer.mozilla.org/en/docs/XPCOM_ABI

When GRE_GetGREPathWithProperties is called, we should automatically add an "ABI" property to the search list, matching the compiled ABI.

Make sense?
Sounds good to me. I'll take a closer look next week (after holidays).
Posted patch patchSplinter Review
That would do it.
I'm not sure if I use the proper way for the memory allocation. Actually it leaks one GREProperty struct what's probably not an issue as it's just one and small.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attachment #310836 - Flags: review?(benjamin)
Comment on attachment 310836 [details] [diff] [review]
patch

Drivers, this patch should only affect XULRunner clients and is fairly safe.
Attachment #310836 - Flags: review?(benjamin)
Attachment #310836 - Flags: review+
Attachment #310836 - Flags: approval1.9?
Comment on attachment 310836 [details] [diff] [review]
patch

a+ schrep
Attachment #310836 - Flags: approval1.9b5+
Attachment #310836 - Flags: approval1.9?
Attachment #310836 - Flags: approval1.9+
Keywords: checkin-needed
Checking in xpcom/build/Makefile.in;
/cvsroot/mozilla/xpcom/build/Makefile.in,v  <--  Makefile.in
new revision: 1.105; previous revision: 1.104
done
Checking in xpcom/glue/Makefile.in;
/cvsroot/mozilla/xpcom/glue/Makefile.in,v  <--  Makefile.in
new revision: 1.58; previous revision: 1.57
done
Checking in xpcom/glue/nsGREGlue.cpp;
/cvsroot/mozilla/xpcom/glue/nsGREGlue.cpp,v  <--  nsGREGlue.cpp
new revision: 1.23; previous revision: 1.22
done
Checking in xpcom/glue/standalone/Makefile.in;
/cvsroot/mozilla/xpcom/glue/standalone/Makefile.in,v  <--  Makefile.in
new revision: 1.41; previous revision: 1.40
done
Checking in xulrunner/app/Makefile.in;
/cvsroot/mozilla/xulrunner/app/Makefile.in,v  <--  Makefile.in
new revision: 1.39; previous revision: 1.38
done
Checking in xulrunner/app/nsXULRunnerApp.cpp;
/cvsroot/mozilla/xulrunner/app/nsXULRunnerApp.cpp,v  <--  nsXULRunnerApp.cpp
new revision: 1.40; previous revision: 1.39
done
Checking in xulrunner/installer/Makefile.in;
/cvsroot/mozilla/xulrunner/installer/Makefile.in,v  <--  Makefile.in
new revision: 1.14; previous revision: 1.13
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta5
Comment on attachment 310836 [details] [diff] [review]
patch

>+#else
>+  GREProperty *allProperties = properties;
>+  PRUint32 allPropertiesLength = propertiesLength;
>+#endif
Possibly because it was a cross-compile (I can't check right now because the office firewall crashed) I didn't have things set up right to give me a TARGET_XPCOM_ABI, and my compiler choked on this line because properties is const and allProperties isn't. Does this just need an extra const at the beginning of the line?
Yes, just adding a const should do it. Sorry, I didn't really test that case since it shouldn't happen usually.
Attachment #311364 - Flags: review?(benjamin)
Attachment #311364 - Flags: review?(benjamin) → review+
Comment on attachment 311364 [details] [diff] [review]
trivial followup

Technically not part of the build, unless it's misconfigured like mine was ;-)
Attachment #311364 - Flags: approval1.9?
perhaps casting this to the non-const would be better to mirror the non-ABI case
Comment on attachment 311364 [details] [diff] [review]
trivial followup

a=beltzner
Attachment #311364 - Flags: approval1.9? → approval1.9+
Duplicate of this bug: 424754
Blocks: 468835
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.