956398 - Support text relocations for Flash

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

In order to to load Flash on 4.4, we need to stub out some missing symbols. For that to work, we have to use our customer linker instead of the Android one. Right now it falls back to the Android linker because we don't support text relocations.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Support text relocations in the custom linker

Can you test whether this actually works? At least, it builds and doesn't break anything on try.

Comment 2

[Mike Hommey [:glandium]]

Note this in itself is not enough for landing, I need to add a build check to make builds fail when there are text relocations.

Comment 3

[Mike Hommey [:glandium]]

Attachment: Support text relocations in the custom linker

Fixed up alignment and removed leftovers from an earlier attempt.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Mike Hommey [:glandium] from comment #2)
> Note this in itself is not enough for landing, I need to add a build check
> to make builds fail when there are text relocations.

Because you don't want any of that in libxul? Maybe enough to have a runtime abort just for a couple libraries?

I'll test your patch here with my other stuff in a bit.

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Patch works here

Comment 6

[Mike Hommey [:glandium]]

Attachment: Error out at build time if we end up with text relocations

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8356996 [details] [diff] [review]
Support text relocations in the custom linker

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

No comment on the need for this.

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 8357474 [details] [diff] [review]
Error out at build time if we end up with text relocations

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

::: config/config.mk
@@ +841,5 @@
>  endif
>  endif
>  
> +ifeq (,$(filter $(OS_TARGET),WINNT Darwin OS2))
> +CHECK_TEXTREL = @$(TOOLCHAIN_PREFIX)readelf -d $(1) | grep TEXTREL > /dev/null && echo 'TEST-UNEXPECTED-FAIL | | We do not want text relocations in libraries and programs' || true

Can you stick a test name in the empty | | space here and above? Even just "checkcxx / checktextrelocations" for these would be fine.

Comment 9

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dec061c862ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/89a7d761a82f

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dec061c862ff
https://hg.mozilla.org/mozilla-central/rev/89a7d761a82f

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8356996 [details] [diff] [review]
Support text relocations in the custom linker

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 935676
User impact if declined: Flash doesn't work on KitKat
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): ?
String or IDL/UUID changes made by this patch: None

Comment 12

[Mike Hommey [:glandium]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #11)
> Risk to taking this patch (and alternatives if risky): ?

The patch is essentially a noop for libraries without text relocations, which is the majority of them. However, it does changes how libraries with text relocations are loaded, in that they are now loaded by our linker instead of the system linker. This might actually cause unexpected problems.

Comment 13

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

(In reply to Mike Hommey [:glandium] from comment #12)
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #11)
> > Risk to taking this patch (and alternatives if risky): ?
> 
> The patch is essentially a noop for libraries without text relocations,
> which is the majority of them. However, it does changes how libraries with
> text relocations are loaded, in that they are now loaded by our linker
> instead of the system linker. This might actually cause unexpected problems.

To mitigate risk, can we uplift a patch which whitelists libflashplayer.so to be loaded by our linker and kick anything else that needs text relocations out to the system linker?

Comment 14

[Mike Hommey [:glandium]]

Attachment: Only support text relocations for libflashplayer.so

Risk mitigation, cf. comment 13.

Comment 15

[Nathan Froyd [:froydnj]]

Comment on attachment 8363444 [details] [diff] [review]
Only support text relocations for libflashplayer.so

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

Assuming this is meant to be applied on top of the existing patch for aurora/beta uplift, r=me.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8356996 [details] [diff] [review]
Support text relocations in the custom linker

We need both patches on Aurora and Beta

Comment 17

[bhavana bajaj [:bajaj]]

Comment on attachment 8363444 [details] [diff] [review]
Only support text relocations for libflashplayer.so

Reqesting QA verification on a try build or trunk before we take this into beta.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a57e9a4224f2
https://hg.mozilla.org/releases/mozilla-aurora/rev/37f5ef8394b1

https://hg.mozilla.org/releases/mozilla-beta/rev/214cc3bf9469
https://hg.mozilla.org/releases/mozilla-beta/rev/9294d16ada62

Comment 19

[Ryan VanderMeulen [:RyanVM]]

And a follow-up for check-sync-dirs bustage.
https://hg.mozilla.org/releases/mozilla-aurora/rev/ac213f3a5bf9
https://hg.mozilla.org/releases/mozilla-beta/rev/cd615489ee57

Comment 20

[James Willcox (:snorp) ([e-mail]) (he/him)]

QA: if you can view the Flash content at http://people.mozilla.org/~jwillcox/flash/flash_test.html, then this patch (and others) are working.

Comment 21

[Mike Hommey [:glandium]]

(In reply to bhavana bajaj [:bajaj] from comment #17)
> Comment on attachment 8363444 [details] [diff] [review]
> Only support text relocations for libflashplayer.so
> 
> Reqesting QA verification on a try build or trunk before we take this into
> beta.

Here's a try build for QA verification of that latter patch:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-9b97ea6ff4f9/try-android/fennec-29.0a1.en-US.android-arm.apk

Comment 22

[Paul Feher]

Verified fixed using Nexus 4 (Android 4.4.2) on:
 27.0 Beta 9 (2014-01-24)
 Aurora 28.0a2 (2014-01-23)

Comment 23

[Mike Hommey [:glandium]]

(In reply to Paul Feher from comment #22)
> Verified fixed using Nexus 4 (Android 4.4.2) on:
>  27.0 Beta 9 (2014-01-24)
>  Aurora 28.0a2 (2014-01-23)

Please also test the try build from comment 21.

Comment 24

[Paul Feher]

(In reply to Mike Hommey [:glandium] from comment #23)
> (In reply to Paul Feher from comment #22)
> > Verified fixed using Nexus 4 (Android 4.4.2) on:
> >  27.0 Beta 9 (2014-01-24)
> >  Aurora 28.0a2 (2014-01-23)
> 
> Please also test the try build from comment 21.

Verified fixed using the try build on Nexus 4 (Android 4.4.2)

Comment 25

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/67b10879f654

Ryan, can you land this on aurora and beta? (cf. a+, comment 17, comment 21 and comment 24)

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4b8d14a78596
https://hg.mozilla.org/releases/mozilla-beta/rev/4b33a0699faa

Comment 27

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/67b10879f654

Comment 28

[Mike Hommey [:glandium]]

Can this be re-verified on aurora and beta after the landing of the last piece?

Comment 29

[Flaviu Cos]

The flash content is flickering on Google Nexus 5 (Android 4.4.2).
Tested on 28 Beta 2 and 29.0a2 (2014-02-11).

Comment 30

[Aaron Train [:aaronmt]]

Please open a new bug.

Comment 31

[Cristina Madaras, QA [:CristinaM]]

Based on Paul's comment 22 I will mark this bug as verified fixed.