420373 - need a way to distinguish GREs based on architecture in nsGREGlue

Status: RESOLVED FIXED

(Toolkit Graveyard :: XULRunner, defect)

Description

[Wolfgang Rosenauer [:wolfiR]]

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?

Comment 1

[Benjamin Smedberg]

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?

Comment 2

[Wolfgang Rosenauer [:wolfiR]]

Sounds good to me. I'll take a closer look next week (after holidays).

Comment 3

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch

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.

Comment 4

[Benjamin Smedberg]

Comment on attachment 310836 [details] [diff] [review]
patch

Drivers, this patch should only affect XULRunner clients and is fairly safe.

Comment 5

[Mike Schroepfer]

Comment on attachment 310836 [details] [diff] [review]
patch

a+ schrep

Comment 6

[Reed Loden [:reed]]

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

Comment 7

[[e-mail]]

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?

Comment 8

[Wolfgang Rosenauer [:wolfiR]]

Yes, just adding a const should do it. Sorry, I didn't really test that case since it shouldn't happen usually.

Comment 9

[[e-mail]]

Attachment: trivial followup

Comment 10

[[e-mail]]

Comment on attachment 311364 [details] [diff] [review]
trivial followup

Technically not part of the build, unless it's misconfigured like mine was ;-)

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: casting no non-const

perhaps casting this to the non-const would be better to mirror the non-ABI case

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 311364 [details] [diff] [review]
trivial followup

a=beltzner