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

Status: VERIFIED FIXED

(Firefox Build System :: General, defect)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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.

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v1

Comment 2

[Ian Stakenvicius]

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.

Comment 3

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

:fallen, would this also be an issues for binary extension compat in Firefox24?

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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.

Comment 5

[Ian Stakenvicius]

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 6

[Mike Hommey [:glandium]]

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)

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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.

Comment 8

[Mike Hommey [:glandium]]

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.

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

bug 916215 suggests otherwise, the new version scheme seems to be 24.1esr or 24.1.0esr which Mark confirmed for Thunderbird.

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v2

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.

Comment 12

[Ian Stakenvicius]

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?

Comment 13

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(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.

Comment 14

[Ian Stakenvicius]

(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.

Comment 15

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: WiP - v3

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?

Comment 16

[Ian Stakenvicius]

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

Comment 17

[Mike Hommey [:glandium]]

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.

Comment 18

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(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

Comment 19

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v4

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.

Comment 20

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/9e197c3158b0

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9e197c3158b0

Comment 22

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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.

Comment 23

[Mike Hommey [:glandium]]

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.

Comment 24

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix standalone build - v1

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?

Comment 25

[Mike Hommey [:glandium]]

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.

Comment 26

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbd97422f174

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cbd97422f174

Comment 28

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 824251 [details] [diff] [review]
Fix - v4

I think we are good now, see comment 22 for the approval request comments.

Comment 29

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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.

Comment 30

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2a3c20049190
https://hg.mozilla.org/releases/mozilla-beta/rev/344b9cd8dc62
https://hg.mozilla.org/releases/mozilla-esr24/rev/d37b5aa18cc0

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/344b9cd8dc62

Comment 33

[Mike Hommey [:glandium]]

Changeset 9e197c3158b0 didn't make it to the branches :(

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Dammit.

https://hg.mozilla.org/releases/mozilla-aurora/rev/bc5422262107
https://hg.mozilla.org/releases/mozilla-beta/rev/d954a7ebe5ec
https://hg.mozilla.org/releases/mozilla-esr24/rev/0fa56f18ff99

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/d954a7ebe5ec

Comment 36

[Brian Slesinsky]

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

[Ioana (away)]

Can anyone point to an add-on QA can test this with?

Comment 38

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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

[Brian Slesinsky]

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.)

Comment 40

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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.

Comment 41

[Andrei Vaida [:avaida]]

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?

Comment 42

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Yes, this is the expected outcome, see comment 10.

Comment 43

[Andrei Vaida [:avaida]]

(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).