Closed Bug 1077366 Opened 5 years ago Closed 5 years ago

Remove most symbol wrapping from Android builds

Categories

(Firefox Build System :: General, defect)

All
Android
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file)

No description provided.
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+
(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.
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
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.
(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 :(
James, see comment 5 and 6 ; assuming I fix the problem with bug 791419, would flash using jemalloc be a problem ?
Flags: needinfo?(snorp)
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.
(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)
https://hg.mozilla.org/mozilla-central/rev/b1deaba82ee2
https://hg.mozilla.org/mozilla-central/rev/ec600d4ffe61
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Blocks: 1109940
No longer blocks: 1109940
Depends on: 1109940
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.