Binary compatibility broken for maintenance releases due to strict version-script

VERIFIED FIXED in Firefox 26

Status

defect
VERIFIED FIXED
6 years ago
Last year

People

(Reporter: Fallen, Assigned: Fallen)

Tracking

({addon-compat})

unspecified
mozilla28
All
Linux
Dependency tree / graph

Firefox Tracking Flags

(firefox26 fixed, firefox27 verified, firefox28 verified, firefox-esr2426+ verified, b2g-v1.2 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

bug 809430 instroduced the use of version-script, which causes problems with extensions that have binary components and minor releases. For example, Lightning 2.6 (built for Thunderbird 24.0) will not work with Thunderbird 24.0.1 with this error:

"Failed to load native module at path '/home/user/../.thunderbird/...default/extensions/{..}/components/Linux_x86_64-gcc3/libcalbasecomps.so': (80004005) /usr/lib64/thunderbird/libxul.so: version `xul24.0' not found (required by /home/user/.../.thunderbird/....default/extensions/{..}/components/Linux_x86_64-gcc3/libcalbasecomps.so)"

Thunderbird 24.0.1 defines `xul24.0.1`, therefore the required symbol is not found. I propose to change the version tag to always advertise "xul24.0" for any 24.0.x version.

Note this is Linux only, Windows and Mac do not have these restrictions.
Posted patch Fix - v1 (obsolete) β€” β€” Splinter Review
Attachment #817357 - Flags: review?(mh+mozilla)

Comment 2

6 years ago
Looks good to me.  Just to confirm, though, is the libXUL and libmozjs API/ABI frozen across all releases of MOZILLA_VERSION_MAJOR ?  If it isn't, we'll end up with the same issue that needed this version script in the first place.
:fallen, would this also be an issues for binary extension compat in Firefox24?
Flags: needinfo?(philipp)
Yes, this should affect Firefox as well as Thunderbird, I haven't actually tested it though. I will push for this being on the mozilla-esr24 branch so we can include it in Thunderbird 24.0.2, which will make it available for Firefox too.

From what I know all 24.0.x releases' libxul/libmozjs should be binary compatible, but if someone else wants to confirm this it would be nice.
Flags: needinfo?(philipp)

Comment 5

6 years ago
It would only affect binary extensions that are built against (ie, link to) libxul or libmozjs, afaik.  The only symbols being versioned are there, unless my original patches have been expanded across more of the source tree than originally planned.
Comment on attachment 817357 [details] [diff] [review]
Fix - v1

Review of attachment 817357 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/library/Makefile.in
@@ +117,4 @@
>  OS_LIBS += -lrt
>  EXTRA_DSO_LDOPTS += -Wl,-version-script,symverscript
>  
> +symverscript: MOZILLA_MAJOR_VERSION=$(shell echo $(MOZILLA_VERSION) | sed "s|\(^[0-9]*\.[0-9]*\).*|\1|")

MOZILLA_MAJOR_VERSION=$(subst $(NULL) ,.,$(wordlist 1,2,$(subst ., ,$(MOZILLA_VERSION))))

Either way, that doesn't keep the alpha bits from the version, which should be kept (i.e. aurora should have x.ya2 and nightly x.ya1)
Attachment #817357 - Flags: review?(mh+mozilla) → review-
Ok, so this needs further discussion. I have a few issues with the choice of tags, mostly related to how the Lightning release process works. Making these changes might cause us to go back to releases every 6 weeks, which I am not ready to do.


In the scheme of major.minor.maintenance.build, lets start with the minor version:

Firefox ESR and Thunderbird does minor releases every 6 weeks. Binary stability should be guaranteed between such versions, so in this case, "xul24" should be all that is required. If "xul24.1" is required, this means a Lightning release every 6 weeks, just to make Linux users happy. We don't have the capacity to do this.

We need to upload the release a week early to addons.mozilla.org for review, and this in turn means we need to take the last beta as the release, so we really only have 5 weeks. Which brings us to the next potential problem:

I don't know if with "alpha bits" you meant both alpha and beta bits, if so then b3 will not be binary compatible to the release. Hence we cannot use the b3 builds as the Lightning release, which means we either need to get the addons team to reintroduce the trusted status (unlikely) and upload the release builds immediately, or postpone each Thunderbird release until the addons team has reviewed Lightning (also sounds unlikely).

The only way I see that we can put the proposals together is to require "xul24" on the ESR branch and "xul24.0" on the non-esr branch. Not sure off the top of my head how we can do this without messing with the version script every time a new ESR branch is created, as Thunderbird doesn't use "esr" in the version name.
Versions are 24.0, 24.0.1, 24.0.2, 25.0, 26.0, etc. I'm suggesting the symbol version becomes, respectively, xul24.0, xul24.0, xul24.0, xul25.0, xul26.0. Beta would have the same version. Only aurora and nightly would be xul30.0a2 and xul30.0a1 respectively.
bug 916215 suggests otherwise, the new version scheme seems to be 24.1esr or 24.1.0esr which Mark confirmed for Thunderbird.
As discussed via IRC, the new versioning scheme discussed in bug 869568 is purely cosmetic and doesn't make any different statement about binary compatibility. A new patch will have:

xul24 for 24.0, 24.0.1, 24.1 and all betas for 24
xul30a2 for aurora
xul30a1 for nightly
Posted patch Fix - v2 (obsolete) β€” β€” Splinter Review
This one takes care. I've explored using only make functions to take apart the version, but its a little more complicated and a lot less readable:

MOZILLA_MAJOR_VERSION=$(basename $(MOZILLA_VERSION))$(strip $(foreach num,0 1 2 3 4 5 6 7 8 9,$(if $(findstring $(num)a,$(MOZILLA_VERSION)),$(patsubst $(num)a%,a%,$(subst .,,$(suffix $(MOZILLA_VERSION)))),)))

Therefore I went with sed. Hope its ok, if you have an easier version I'm happy to use that.
Attachment #817357 - Attachment is obsolete: true
Attachment #820990 - Flags: review?(mh+mozilla)

Comment 12

6 years ago
Is "symberscript" (note the 'b') what you meant to have?

Also, to be consistent with js/src it would probably make sense to do this in configure.in rather than in Makefile.in; no reason why MOZILLA_MAJOR_VERSION can't be used elsewhere if necessary.

Should this change to symbol versioning for xul also occur for mozjs, to keep them consistent?
(In reply to Ian Stakenvicius from comment #12)
> Is "symberscript" (note the 'b') what you meant to have?
No, that is of course a typo, sorry!

> Also, to be consistent with js/src it would probably make sense to do this
> in configure.in rather than in Makefile.in; no reason why
> MOZILLA_MAJOR_VERSION can't be used elsewhere if necessary.
Would be fine with me, but note this is a quite mangled version, as it doesn't contain anything but the major version and the alpha version tag. There are not many places I see this being used aside from symbols for binary files.


> Should this change to symbol versioning for xul also occur for mozjs, to
> keep them consistent?
I don't know enough about mozjs to say so, but it would seem right. Can someone confirm?

I'll upload a new version as soon as I have answers on the above.
Attachment #820990 - Flags: review?(mh+mozilla) → review-

Comment 14

6 years ago
(In reply to Philipp Kewisch [:Fallen] from comment #13)
> > Also, to be consistent with js/src it would probably make sense to do this
> > in configure.in rather than in Makefile.in; no reason why
> > MOZILLA_MAJOR_VERSION can't be used elsewhere if necessary.
> Would be fine with me, but note this is a quite mangled version, as it
> doesn't contain anything but the major version and the alpha version tag.
> There are not many places I see this being used aside from symbols for
> binary files.
> 

Ahh, good point.  Probably two vars, MOZILLA_MAJOR_VERSION and MOZILLA_ALPHA would make more sense, and then the symbol version would be the concatenation of the two.  js/src/configure.in actually splits out all elements of the version into MAJOR, MINOR, PATCH and ALPHA.  Not saying that the sed's used there should be used here, because they're fugly, but the general idea might be applicable.
Posted patch WiP - v3 (obsolete) β€” β€” Splinter Review
This patch is not ready yet, especially because I didn't actually use MOZILLA_SYMBOLVERSION in js/src and haven't tested it, but I thought I'd upload it to get some feedback on the concept.

It does not use MOZILLA_MAJOR_VERSION or MOZILLA_ALPHA, but instead follows the principal of MOZILLA_UAVERSION which puts together the full version at once.

What do you think of this one? Or do you still prefer adding a configure.in variable for each component?
Attachment #820990 - Attachment is obsolete: true
Attachment #821499 - Flags: feedback?(axs)

Comment 16

6 years ago
Comment on attachment 821499 [details] [diff] [review]
WiP - v3

Apologies for the delay.  Yes, this looks like a good way to go.  

I haven't applied/tested it, so there may be minor technical issues; however the concept seems great to me and I don't see anything that would cause issues with either the javascript or the main mozilla build system.

Flagging glandium for review
Attachment #821499 - Flags: review?(mh+mozilla)
Attachment #821499 - Flags: feedback?(axs)
Attachment #821499 - Flags: feedback+
Blocks: 920200
Attachment #821499 - Flags: review?(mh+mozilla) → review+
Comment on attachment 821499 [details] [diff] [review]
WiP - v3

Review of attachment 821499 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/configure.in
@@ +240,5 @@
>  
>  AC_DEFINE_UNQUOTED(MOZILLA_VERSION,"$MOZILLA_VERSION")
>  AC_DEFINE_UNQUOTED(MOZILLA_VERSION_U,$MOZILLA_VERSION)
>  AC_DEFINE_UNQUOTED(MOZILLA_UAVERSION,"$MOZILLA_UAVERSION")
> +AC_DEFINE_UNQUOTED(MOZILLA_SYMBOLVERSION,"$MOZILLA_SYMBOLVERSION")

While you're here, you could use this value in js/src/moz.build.
(In reply to Mike Hommey [:glandium] from comment #17)
> 
> While you're here, you could use this value in js/src/moz.build.

Thanks, this was the file I was looking for. I've changed it, here is a try run:

https://tbpl.mozilla.org/?tree=Try&rev=b6d2e2f104b6
Posted patch Fix - v4 β€” β€” Splinter Review
Ok, here is the patch I pushed to try. It seems mostly green, the oranges and red seem unrelated to the patch. Please correct me if needed.

I will push this to inbound in just a moment.
Attachment #821499 - Attachment is obsolete: true
Attachment #824251 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9e197c3158b0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Comment on attachment 824251 [details] [diff] [review]
Fix - v4

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 809430
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  - See user impact if declined. Binary compatibility between minor
    releases is currently broken.
User impact if declined: 
  - Extension with binary components will need to create one release for
    each minor release of Firefox/Thunderbird (i.e 24.0.1, 24.2.0, ...)
  - Lots of user complaints from users will cause addon ratings to drop
Testing completed (on m-c, etc.):
  - Tested locally, will test nightly as soon as it shows up.
Risk to taking this patch (and alternatives if risky):
  - Only thing I can think of is a possible regression to bug 809430, but as
    we've only changed the version tag not removed it, I think it should be
    fine.
String or IDL/UUID changes made by this patch:
  - None


Sorry if I am requesting too much at once, but I really need this landed on ESR24, this is giving major headaches. I need to create one Lightning release per minor Thunderbird release and it means a lot of support requests from Lightning's users.
Attachment #824251 - Flags: approval-mozilla-esr24?
Attachment #824251 - Flags: approval-mozilla-beta?
Attachment #824251 - Flags: approval-mozilla-aurora?
Comment on attachment 824251 [details] [diff] [review]
Fix - v4

This patch is actually half borked. Standalone builds end up with a lib named libmozjs-None.so. See bug 933123 comment 5.
Attachment #824251 - Flags: approval-mozilla-esr24?
Attachment #824251 - Flags: approval-mozilla-beta?
Attachment #824251 - Flags: approval-mozilla-aurora?
Sorry about that, I don't know my way around autoconf too much. Here is the patched based on your comment. I don't understand why it needs to be different to the other version variables though?
Attachment #825852 - Flags: review?(mh+mozilla)
Comment on attachment 825852 [details] [diff] [review]
Fix standalone build - v1

Review of attachment 825852 [details] [diff] [review]:
-----------------------------------------------------------------

AC_DEFINE is for defines that end up in C/C++ code or on Preprocessor's command line.
AC_SUBST is to make things available as make variables.
Attachment #825852 - Flags: review?(mh+mozilla) → review+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #824251 - Flags: checkin+
Blocks: ltn263
https://hg.mozilla.org/mozilla-central/rev/cbd97422f174
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Comment on attachment 824251 [details] [diff] [review]
Fix - v4

I think we are good now, see comment 22 for the approval request comments.
Attachment #824251 - Flags: approval-mozilla-esr24?
Attachment #824251 - Flags: approval-mozilla-beta?
Attachment #824251 - Flags: approval-mozilla-aurora?
Comment on attachment 825852 [details] [diff] [review]
Fix standalone build - v1

Same goes for this patch as its a regression fix for the previous, comment 22 applies as well.
Attachment #825852 - Flags: approval-mozilla-esr24?
Attachment #825852 - Flags: approval-mozilla-beta?
Attachment #825852 - Flags: approval-mozilla-aurora?
Attachment #824251 - Flags: approval-mozilla-esr24?
Attachment #824251 - Flags: approval-mozilla-esr24+
Attachment #824251 - Flags: approval-mozilla-beta?
Attachment #824251 - Flags: approval-mozilla-beta+
Attachment #824251 - Flags: approval-mozilla-aurora?
Attachment #824251 - Flags: approval-mozilla-aurora+
Comment on attachment 825852 [details] [diff] [review]
Fix standalone build - v1

Approving for ESR even though it is outside of the primary security needs because addon compatibility and pain for addon developers can be avoided by taking this.
Attachment #825852 - Flags: approval-mozilla-esr24?
Attachment #825852 - Flags: approval-mozilla-esr24+
Attachment #825852 - Flags: approval-mozilla-beta?
Attachment #825852 - Flags: approval-mozilla-beta+
Attachment #825852 - Flags: approval-mozilla-aurora?
Attachment #825852 - Flags: approval-mozilla-aurora+

Updated

6 years ago
Blocks: 922777
Changeset 9e197c3158b0 didn't make it to the branches :(
No longer blocks: 922777

Comment 36

6 years ago
This is resolved fixed, but where does that leave us for Firefox 24 ESR? Will there be a new release, and will we have to rebuild binary plugins?

Comment 37

6 years ago
Can anyone point to an add-on QA can test this with?
I don't have a Firefox addon available, but you can test this with Thunderbird 24.1.1 and Lightning 2.6.3.

Another way to verify, on Linux:

1) Download Firefox 24.2.0 (which seems to be the first version this works with)
2) unpack, then in the console:
    objdump -T libxul.so | grep xul24
3) Observe the 5th column, it should say xul24, not xul24.2.0 or similar

Example output from releases.mozilla.org's firefox-24.2.0esr.tar.bz2's libxul.so:

...
01ae6460 g    DF .text  0000001c  xul24       _ZN2JS25IsIncrementalGCInProgressEP9JSRuntime
01ae63d0 g    DF .text  00000015  xul24       _ZN2JS18SetGCSliceCallbackEP9JSRuntimePFvS1_NS_10GCProgressERKNS_13GCDescriptionEE
0144299e g    DF .text  00000003  xul24       NS_RegisterXPCOMExitRoutine
 

Brian, as you may have read from the above, Firefox 24.2.0 contains this fix. You will have to rebuild your binary plugin and make sure symbol dependencies are set up to use "xul24". If you want to support 24.0 - 24.1.1 as well as 24.2.0+, you will have to provide two binaries.

Comment 39

6 years ago
So just to clarify, it sounds like I should copy libxul.so from the Firefox 24.2.0 release and link against that to pick up the new symbols? (I normally link against the XulRunner SDK.)

(I'm not actually a C++ programmer; I just do releases sometimes.)
I don't know if there will be a xulrunner release, but I guess there should be. Linking against Firefox 24.2.0's libxul could be a valid workaround until thats the case. On the otherhand, it might be enough to manipulate the LD version-script parameter when linking: https://www.gnu.org/software/gnulib/manual/html_node/LD-Version-Scripts.html

If you don't use this, you might be fine. I don't understand all details of this parameter, please excuse.

Updated

6 years ago
Blocks: 925823
I was able to confirm the fix for this issue using the instructions provided by Philipp in Comment 38, under Ubuntu 13.10 64-bit with:
- the latest Beta (Build ID: 20140123185438)
- Fx 26 (Build ID: 20131205075310)
- Fx 24.2.0 ESR (Build ID: 20131205180928)

The latest Aurora (Build ID: 20140123004002) displays xul28a2 instead of just xul28. Philipp, could you please confirm whether this is an expected outcome or not?
Yes, this is the expected outcome, see comment 10.
Flags: needinfo?(philipp)
(In reply to Philipp Kewisch [:Fallen] from comment #42)
> Yes, this is the expected outcome, see comment 10.

Thank you for the clarification, Philipp. Marking this issue as fixed also on the latest Aurora (Build ID: 20140123004002).
Status: RESOLVED → VERIFIED
Keywords: verifyme

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.