1077366 - Remove most symbol wrapping from Android builds

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Mike Hommey [:glandium]]

Attachment: 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.

Comment 2

[Nathan Froyd [:froydnj]]

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.

Comment 3

[Mike Hommey [:glandium]]

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

Comment 4

[Mike Hommey [:glandium]]

So, for even more security, let's make this depend on bug 1077384, and I'll change the comment accordingly when landing.

Comment 5

[Mike Hommey [:glandium]]

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.

Comment 6

[Mike Hommey [:glandium]]

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

Comment 7

[Mike Hommey [:glandium]]

James, see comment 5 and 6 ; assuming I fix the problem with bug 791419, would flash using jemalloc be a problem ?

Comment 8

[Mike Hommey [:glandium]]

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.

Comment 9

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

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

Comment 10

[Mike Hommey [:glandium]]

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

Comment 11

[Mike Hommey [:glandium]]

And a fixup to force a reconfigure in js/src.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec600d4ffe61

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b1deaba82ee2
https://hg.mozilla.org/mozilla-central/rev/ec600d4ffe61