Remove most symbol wrapping from Android builds

RESOLVED FIXED in mozilla36

Status

()

Core
Build Config
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

unspecified
mozilla36
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8499508 [details] [diff] [review]
Remove most symbol wrapping from Android builds

Since essentially everything is linked to libmozglue and libmozglue takes
precedence in symbol resolution in our dynamic linker, there is no need
to wrap most symbols. PR_GetEnv/PR_SetEnv still needs wrapping because
there's no other way to actually wrap the calls from NSPR itself and NSS,
as well as the symbols wrapped because our dynamic linker can't find them
in system libraries on some devices because they're weak.

Note that I'm actually going to change the dynamic linker to make libmozglue
a kind-of LD_PRELOAD so that we don't need to link everything against it.
Attachment #8499508 - Flags: review?(nfroyd)
Comment on attachment 8499508 [details] [diff] [review]
Remove most symbol wrapping from Android builds

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

It's always nice when a pile of code gets removed.

::: memory/build/mozmemory_wrap.h
@@ +76,5 @@
>   * All these functions are meant to be called with no prefix from Gecko code.
>   * In most cases, this is because that's how they are available at runtime.
> + * However, on Android, this relies on faulty.lib (the custom dynamic linker)
> + * resolving mozglue symbols before libc symbols, which is guaranteed by the
> + * way faulty.lib works (it respects the DT_NEEDED order, and libc always

Just out of curiousity, does faulty.lib have tests that verify that DT_NEEDED order is respected?

@@ +77,5 @@
>   * In most cases, this is because that's how they are available at runtime.
> + * However, on Android, this relies on faulty.lib (the custom dynamic linker)
> + * resolving mozglue symbols before libc symbols, which is guaranteed by the
> + * way faulty.lib works (it respects the DT_NEEDED order, and libc always
> + * appears after mozglue ; which we double check when building anyways)

Nit: I think " ;" wants to be a comma.
Attachment #8499508 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 3

3 years ago
(In reply to Nathan Froyd (:froydnj) from comment #2)
> Comment on attachment 8499508 [details] [diff] [review]
> Remove most symbol wrapping from Android builds
> 
> Review of attachment 8499508 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It's always nice when a pile of code gets removed.
> 
> ::: memory/build/mozmemory_wrap.h
> @@ +76,5 @@
> >   * All these functions are meant to be called with no prefix from Gecko code.
> >   * In most cases, this is because that's how they are available at runtime.
> > + * However, on Android, this relies on faulty.lib (the custom dynamic linker)
> > + * resolving mozglue symbols before libc symbols, which is guaranteed by the
> > + * way faulty.lib works (it respects the DT_NEEDED order, and libc always
> 
> Just out of curiousity, does faulty.lib have tests that verify that
> DT_NEEDED order is respected?

That's simply how it works. It goes through the DT_NEEDED one by one. That said, it's still possible that dlsym would return a symbol that doesn't come from libmozglue if libmozglue does not come first... But then bug 1077384 just moves the problem entirely.
(Assignee)

Comment 4

3 years ago
So, for even more security, let's make this depend on bug 1077384, and I'll change the comment accordingly when landing.
Depends on: 1077384
(Assignee)

Comment 5

3 years ago
One notable downside of this is that the flash plugin would start using jemalloc (assuming it uses plain malloc calls)... and depending on the symbols it imports, it may fail to load on the devices that required the hack from bug 791419.
(Assignee)

Comment 6

3 years ago
(In reply to Mike Hommey [:glandium] from comment #5)
> One notable downside of this is that the flash plugin would start using
> jemalloc (assuming it uses plain malloc calls)...

(... which it does.)

> and depending on the
> symbols it imports, it may fail to load on the devices that required the
> hack from bug 791419.

... and it does use those symbols :(
(Assignee)

Comment 7

3 years ago
James, see comment 5 and 6 ; assuming I fix the problem with bug 791419, would flash using jemalloc be a problem ?
Flags: needinfo?(snorp)
(Assignee)

Comment 8

3 years ago
Hum, in fact, devices with bug 791419 wouldn't load the flash plugin with our linker anyways, because the linker wouldn't resolve those symbols from bug 791419. So the question boils down to whether using jemalloc for flash when loaded with our linker would be a problem.
(Assignee)

Updated

3 years ago
Blocks: 1080341
(Assignee)

Updated

3 years ago
Blocks: 1080342
(In reply to Mike Hommey [:glandium] from comment #8)
> Hum, in fact, devices with bug 791419 wouldn't load the flash plugin with
> our linker anyways, because the linker wouldn't resolve those symbols from
> bug 791419. So the question boils down to whether using jemalloc for flash
> when loaded with our linker would be a problem.

It should be ok. I don't remember any place where we allocate stuff for the plugin to free itself.
Flags: needinfo?(snorp)
(Assignee)

Comment 11

3 years ago
And a fixup to force a reconfigure in js/src.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec600d4ffe61
https://hg.mozilla.org/mozilla-central/rev/b1deaba82ee2
https://hg.mozilla.org/mozilla-central/rev/ec600d4ffe61
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36

Updated

3 years ago
Blocks: 1109940
(Assignee)

Updated

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