Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Support text relocations for Flash

VERIFIED FIXED in Firefox 27

Status

()

Firefox for Android
General
VERIFIED FIXED
4 years ago
a year ago

People

(Reporter: snorp, Assigned: glandium)

Tracking

Trunk
Firefox 29
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox27 verified, firefox28 verified, firefox29 verified, fennec29+)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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.
tracking-fennec: --- → ?
(Assignee)

Comment 1

4 years ago
Created attachment 8356988 [details] [diff] [review]
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.
(Assignee)

Comment 2

4 years ago
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.
(Assignee)

Comment 3

4 years ago
Created attachment 8356996 [details] [diff] [review]
Support text relocations in the custom linker

Fixed up alignment and removed leftovers from an earlier attempt.
(Assignee)

Updated

4 years ago
Attachment #8356988 - Attachment is obsolete: true
(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.
Patch works here
(Assignee)

Updated

4 years ago
Attachment #8356996 - Flags: review?(nfroyd)
(Assignee)

Comment 6

4 years ago
Created attachment 8357474 [details] [diff] [review]
Error out at build time if we end up with text relocations
Attachment #8357474 - Flags: review?(ted)
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.
Attachment #8356996 - Flags: review?(nfroyd) → review+
tracking-fennec: ? → 29+
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.
Attachment #8357474 - Flags: review?(ted) → review+
(Assignee)

Comment 9

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/dec061c862ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/89a7d761a82f
https://hg.mozilla.org/mozilla-central/rev/dec061c862ff
https://hg.mozilla.org/mozilla-central/rev/89a7d761a82f
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
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
Attachment #8356996 - Flags: approval-mozilla-beta?
Attachment #8356996 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 12

4 years ago
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
(In reply to Mike Hommey [:glandium] from comment #12)
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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?
(Assignee)

Comment 14

4 years ago
Created attachment 8363444 [details] [diff] [review]
Only support text relocations for libflashplayer.so

Risk mitigation, cf. comment 13.
Attachment #8363444 - Flags: review?(nfroyd)
(Assignee)

Updated

4 years ago
Attachment #8357474 - Attachment is obsolete: true
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.
Attachment #8363444 - Flags: review?(nfroyd) → review+
Attachment #8356996 - Flags: approval-mozilla-beta?
Attachment #8356996 - Flags: approval-mozilla-aurora?
Comment on attachment 8356996 [details] [diff] [review]
Support text relocations in the custom linker

We need both patches on Aurora and Beta
Attachment #8356996 - Flags: approval-mozilla-beta?
Attachment #8356996 - Flags: approval-mozilla-aurora?
Attachment #8363444 - Flags: approval-mozilla-beta?
Attachment #8363444 - Flags: approval-mozilla-aurora?

Updated

4 years ago
Attachment #8356996 - Flags: approval-mozilla-beta?
Attachment #8356996 - Flags: approval-mozilla-beta+
Attachment #8356996 - Flags: approval-mozilla-aurora?
Attachment #8356996 - Flags: approval-mozilla-aurora+
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.
Attachment #8363444 - Flags: approval-mozilla-beta?
Attachment #8363444 - Flags: approval-mozilla-beta+
Attachment #8363444 - Flags: approval-mozilla-aurora?
Attachment #8363444 - Flags: approval-mozilla-aurora+

Updated

4 years ago
Keywords: verifyme
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
status-firefox27: --- → fixed
status-firefox28: --- → fixed
status-firefox29: --- → fixed
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
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.

Updated

4 years ago
Status: RESOLVED → VERIFIED
status-firefox29: fixed → verified
Keywords: verifyme
OS: Mac OS X → Android
Hardware: x86 → ARM
(Assignee)

Updated

4 years ago
Attachment #8357474 - Attachment is obsolete: false
(Assignee)

Comment 21

4 years ago
(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/mh@glandium.org-9b97ea6ff4f9/try-android/fennec-29.0a1.en-US.android-arm.apk

Comment 22

4 years ago
Verified fixed using Nexus 4 (Android 4.4.2) on:
 27.0 Beta 9 (2014-01-24)
 Aurora 28.0a2 (2014-01-23)
status-firefox27: fixed → verified
status-firefox28: fixed → verified
(Assignee)

Comment 23

4 years ago
(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

4 years ago
(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)
(Assignee)

Comment 25

4 years ago
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)
Flags: needinfo?(ryanvm)
https://hg.mozilla.org/releases/mozilla-aurora/rev/4b8d14a78596
https://hg.mozilla.org/releases/mozilla-beta/rev/4b33a0699faa
Flags: needinfo?(ryanvm)
https://hg.mozilla.org/mozilla-central/rev/67b10879f654
(Assignee)

Comment 28

4 years ago
Can this be re-verified on aurora and beta after the landing of the last piece?
Status: VERIFIED → RESOLVED
Last Resolved: 4 years ago4 years ago
Keywords: verifyme

Comment 29

4 years ago
The flash content is flickering on Google Nexus 5 (Android 4.4.2).
Tested on 28 Beta 2 and 29.0a2 (2014-02-11).
Please open a new bug.
Based on Paul's comment 22 I will mark this bug as verified fixed.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Blocks: 1025090

Updated

3 years ago
Depends on: 1059255
You need to log in before you can comment on or make changes to this bug.