Android: Use linker script to hide all non-JNI symbols

Android: Use linker script to hide all non-JNI symbols

The linker script is applied to all shared_library() targets by default,
just like the hide_jni one was.

This reduces the size of libchrome.so by 220kb.

BUG=448386
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2633593004
Cr-Commit-Position: refs/heads/master@{#448988}
Committed: https://chromium.googlesource.com/chromium/src/+/6888d5002b2eabe4fac226f0e5124fbd62bdd215

[agrieve, 2017-01-14 03:49:30]

Description was changed from

==========
Android: Use linker script to hide all non-JNI symbols

The linker script is applied to all shared_library() targets by default,
just like the hide_jni one was.

This reduces the size of libchrome.so by 220kb.

BUG=todo
==========

to

==========
Android: Use linker script to hide all non-JNI symbols

The linker script is applied to all shared_library() targets by default,
just like the hide_jni one was.

This reduces the size of libchrome.so by 220kb.

BUG=todo
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[agrieve, 2017-01-14 03:49:41]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-14 03:49:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-14 03:54:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-14 03:54:14]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[agrieve, 2017-01-16 01:32:47]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 01:32:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-16 01:45:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-16 01:45:44]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[agrieve, 2017-01-16 13:07:05]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 13:07:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-16 13:40:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-16 13:40:02]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[agrieve, 2017-01-16 14:42:41]

Description was changed from

==========
Android: Use linker script to hide all non-JNI symbols

The linker script is applied to all shared_library() targets by default,
just like the hide_jni one was.

This reduces the size of libchrome.so by 220kb.

BUG=todo
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Android: Use linker script to hide all non-JNI symbols

The linker script is applied to all shared_library() targets by default,
just like the hide_jni one was.

This reduces the size of libchrome.so by 220kb.

BUG=681557
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[agrieve, 2017-01-16 14:46:37]

agrieve@chromium.org changed reviewers:
+ torne@chromium.org

[agrieve, 2017-01-16 14:46:38]

I'll need to roll both angle and ffmpeg since they have shared_library targets
:/. I think it's good to make this the default though. That way, if something
should be exporting more, things will break. We'll never silently export too
much.

[Torne, 2017-01-16 15:03:26]

I have an existing bug open for preventing this from regressing (I fixed them
all manually some time ago, but I guess it's regressed), and at the time I was
originally going to do it this way but got a lot of objections from people that
having a special whitelisting mechanism for android was confusing and made it
hard to develop shared library functionality properly (especially things around
mojo).

crbug.com/448386 is the old bug for this, and some of the CLs linked there might
have the discussion about this.

I think whitelisting is mostly a reasonable option, but I'm not sure if there's
still going to be a lot of opposition to it.

The alternative would be to treat exported symbols a bit like global
initialisers, and have something that monitors them and alerts when new ones are
added (after removing all the incorrectly exported ones again).

There's probably some value in fixing up whatever has introduced all these newly
exported symbols since last time I fixed it (it's potentially useful on other
platforms as well), regardless of which direction we go.

[agrieve, 2017-01-16 15:17:41]

On 2017/01/16 15:03:26, Torne wrote:
> I have an existing bug open for preventing this from regressing (I fixed them
> all manually some time ago, but I guess it's regressed), and at the time I was
> originally going to do it this way but got a lot of objections from people
that
> having a special whitelisting mechanism for android was confusing and made it
> hard to develop shared library functionality properly (especially things
around
> mojo).
> 
> crbug.com/448386 is the old bug for this, and some of the CLs linked there
might
> have the discussion about this.
> 
> I think whitelisting is mostly a reasonable option, but I'm not sure if
there's
> still going to be a lot of opposition to it.
> 
> The alternative would be to treat exported symbols a bit like global
> initialisers, and have something that monitors them and alerts when new ones
are
> added (after removing all the incorrectly exported ones again).
> 
> There's probably some value in fixing up whatever has introduced all these
newly
> exported symbols since last time I fixed it (it's potentially useful on other
> platforms as well), regardless of which direction we go.

Thanks for this!

From what I could dig up, it was jamesr and qsr	who objected, and neither still
work on Chrome. I'll start a thread on chromium-dev to see if anyone else thinks
it's a bad idea. I think it's pretty compelling to go with the whitelist given
that:
a) mac has one, and
b) it's regressed pretty badly since 2 years ago.

[Torne, 2017-01-16 15:21:22]

On 2017/01/16 15:17:41, agrieve wrote:
> Thanks for this!
> 
> From what I could dig up, it was jamesr and qsr	who objected, and neither
still
> work on Chrome. I'll start a thread on chromium-dev to see if anyone else
thinks
> it's a bad idea. I think it's pretty compelling to go with the whitelist given
> that:
> a) mac has one, and
> b) it's regressed pretty badly since 2 years ago.

Sure, if you want to push that forward I'm happy to review/comment :)

It would be sorta interesting to see where the symbols come from, though,
because most of the "leaks" in the past were caused by libraries doing silly
things that should probably get fixed anyway.

[Torne, 2017-01-18 13:29:16]

One other thought that occurs to me: I forget what the current state actually
is, but at least at one point it was the case that in a component build, no
symbol stripping happened at all, which meant that if you forgot to call a JNI
registration function, everything still worked in a component build, but would
fail in a static build.

I don't remember if that got fixed or not, but presumably your whitelist doesn't
apply to component builds (since that would break them entirely)? It seems still
valuable to strip JNI exports in that case..

[agrieve, 2017-01-18 16:49:23]

On 2017/01/18 13:29:16, Torne wrote:
> One other thought that occurs to me: I forget what the current state actually
> is, but at least at one point it was the case that in a component build, no
> symbol stripping happened at all, which meant that if you forgot to call a JNI
> registration function, everything still worked in a component build, but would
> fail in a static build.
> 
> I don't remember if that got fixed or not, but presumably your whitelist
doesn't
> apply to component builds (since that would break them entirely)? It seems
still
> valuable to strip JNI exports in that case..

I noticed that, and it's certainly still the case. I wondered if it was done
that way so that WebView could use component mode?

[Torne, 2017-01-18 16:53:58]

On 2017/01/18 16:49:23, agrieve wrote:
> On 2017/01/18 13:29:16, Torne wrote:
> > One other thought that occurs to me: I forget what the current state
actually
> > is, but at least at one point it was the case that in a component build, no
> > symbol stripping happened at all, which meant that if you forgot to call a
JNI
> > registration function, everything still worked in a component build, but
would
> > fail in a static build.
> > 
> > I don't remember if that got fixed or not, but presumably your whitelist
> doesn't
> > apply to component builds (since that would break them entirely)? It seems
> still
> > valuable to strip JNI exports in that case..
> 
> I noticed that, and it's certainly still the case. I wondered if it was done
> that way so that WebView could use component mode?

Oh. Yeah, that's why I didn't fix that. I remember now ;)

[digit1, 2017-01-19 16:57:07]

The component build requires shared libraries to continue exposing their symbols
(but hiding the Java_ ones should be ok). That's why the Android _dbg builds are
failing in the CQ :)

Generally speaking, most of the removed symbols come from libc++, and that
should not happen in the
first place. I'm trying to look at this issue myself (see
https://bugs.chromium.org/p/chromium/issues/detail?id=682694)
but it looks like a slightly different toolchain or build issue.

That doesn't mean that it's not a good idea to limit release shared libraries
like libchrome.so that are not
supposed to export anything apart from JNI_OnLoad, this will take care of other
"wild exports" that may happen
by accident.

Can you make this change more conditional on the type of build being performed?

[digit1, 2017-01-19 16:57:41]

digit@chromium.org changed reviewers:
+ digit@chromium.org

[Torne, 2017-01-19 16:59:15]

On 2017/01/19 16:57:07, digit1 wrote:
> The component build requires shared libraries to continue exposing their
symbols
> (but hiding the Java_ ones should be ok). That's why the Android _dbg builds
are
> failing in the CQ :)

Hiding the Java_ ones is not okay, because then component builds of WebView and
Monochrome won't work, since they don't do any JNI registration at startup.
That's why I didn't implement this before, now I remember :)

> Generally speaking, most of the removed symbols come from libc++, and that
> should not happen in the
> first place. I'm trying to look at this issue myself (see
> https://bugs.chromium.org/p/chromium/issues/detail?id=682694)
> but it looks like a slightly different toolchain or build issue.

Previously I had masked a lot of the static libraries that leaked symbols with
-Wl,--exclude-libs but I suspect the change of C++ library has meant the flag I
added no longer applies due to the different filename?

> That doesn't mean that it's not a good idea to limit release shared libraries
> like libchrome.so that are not
> supposed to export anything apart from JNI_OnLoad, this will take care of
other
> "wild exports" that may happen
> by accident.
> 
> Can you make this change more conditional on the type of build being
performed?

[digit1, 2017-01-19 17:11:53]

On 2017/01/19 16:59:15, Torne wrote:
> Hiding the Java_ ones is not okay, because then component builds of WebView
and
> Monochrome won't work, since they don't do any JNI registration at startup.
> That's why I didn't implement this before, now I remember :)
> 
Point taken :) I wonder why this is the case though? Can you clarify?

> 
> Previously I had masked a lot of the static libraries that leaked symbols with
> -Wl,--exclude-libs but I suspect the change of C++ library has meant the flag
I
> added no longer applies due to the different filename?
> 

Yes, very probably indeed. More information on the relevant bug entry. Thanks.

> > That doesn't mean that it's not a good idea to limit release shared
libraries
> > like libchrome.so that are not
> > supposed to export anything apart from JNI_OnLoad, this will take care of
> other
> > "wild exports" that may happen
> > by accident.
> > 
> > Can you make this change more conditional on the type of build being
> performed?

[Torne, 2017-01-19 17:20:13]

On 2017/01/19 17:11:53, digit1 wrote:
> On 2017/01/19 16:59:15, Torne wrote:
> > Hiding the Java_ ones is not okay, because then component builds of WebView
> and
> > Monochrome won't work, since they don't do any JNI registration at startup.
> > That's why I didn't implement this before, now I remember :)
> > 
> Point taken :) I wonder why this is the case though? Can you clarify?

We don't do any JNI registration on startup because it's not necessary, because
we have the Java_* symbols for the VM to dynamically look up. We prefer doing it
this way because it's faster (it smears the cost of JNI registration across time
and across threads, instead of paying it all at once on the UI thread). Only
pre-Monochrome builds of Chrome need to do manual JNI registration because the
dynamic lookup doesn't work with the custom linker - that's why we strip the
symbols out by default, to prevent people from accidentally relying on this
mechanism in binaries where it won't work in production.

> > 
> > Previously I had masked a lot of the static libraries that leaked symbols
with
> > -Wl,--exclude-libs but I suspect the change of C++ library has meant the
flag
> I
> > added no longer applies due to the different filename?
> > 
> 
> Yes, very probably indeed. More information on the relevant bug entry. Thanks.
> 
> > > That doesn't mean that it's not a good idea to limit release shared
> libraries
> > > like libchrome.so that are not
> > > supposed to export anything apart from JNI_OnLoad, this will take care of
> > other
> > > "wild exports" that may happen
> > > by accident.
> > > 
> > > Can you make this change more conditional on the type of build being
> > performed?

[agrieve, 2017-01-19 17:33:46]

Description was changed from

==========
Android: Use linker script to hide all non-JNI symbols

The linker script is applied to all shared_library() targets by default,
just like the hide_jni one was.

This reduces the size of libchrome.so by 220kb.

BUG=681557
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Android: Use linker script to hide all non-JNI symbols

The linker script is applied to all shared_library() targets by default,
just like the hide_jni one was.

This reduces the size of libchrome.so by 220kb.

BUG=448386
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[agrieve, 2017-01-19 17:39:49]

On 2017/01/19 16:57:07, digit1 wrote:
> The component build requires shared libraries to continue exposing their
symbols
> (but hiding the Java_ ones should be ok). That's why the Android _dbg builds
are
> failing in the CQ :)
I think they're failing because ffmpeg has a shared_library() that I'll need to
update to opt it out.

> 
> Generally speaking, most of the removed symbols come from libc++, and that
> should not happen in the
> first place. I'm trying to look at this issue myself (see
> https://bugs.chromium.org/p/chromium/issues/detail?id=682694)
> but it looks like a slightly different toolchain or build issue.
> 
> That doesn't mean that it's not a good idea to limit release shared libraries
> like libchrome.so that are not
> supposed to export anything apart from JNI_OnLoad, this will take care of
other
> "wild exports" that may happen
> by accident.
> 
> Can you make this change more conditional on the type of build being
performed?
What conditions do you think would be appropriate?

[agrieve, 2017-01-19 17:42:07]

On 2017/01/19 17:39:49, agrieve wrote:
> On 2017/01/19 16:57:07, digit1 wrote:
> > The component build requires shared libraries to continue exposing their
> symbols
> > (but hiding the Java_ ones should be ok). That's why the Android _dbg builds
> are
> > failing in the CQ :)
> I think they're failing because ffmpeg has a shared_library() that I'll need
to
> update to opt it out.
> 
> > 
> > Generally speaking, most of the removed symbols come from libc++, and that
> > should not happen in the
> > first place. I'm trying to look at this issue myself (see
> > https://bugs.chromium.org/p/chromium/issues/detail?id=682694)
> > but it looks like a slightly different toolchain or build issue.
> > 
> > That doesn't mean that it's not a good idea to limit release shared
libraries
> > like libchrome.so that are not
> > supposed to export anything apart from JNI_OnLoad, this will take care of
> other
> > "wild exports" that may happen
> > by accident.
> > 
> > Can you make this change more conditional on the type of build being
> performed?
> What conditions do you think would be appropriate?

I think this probably has reached "silent approval" in that no one has voiced
concerns yet on the post from the ML. To move forward with this though, I'll
plan to:
a) Add no-op configs for hide_all_but_jni and hide_all_but_jni_onload
b) Roll ffmpeg and angle with the config changes
c) Ensure rolls stick (they will bring in other commits)
d) Land this change.

[Torne, 2017-01-19 17:55:37]

On 2017/01/19 17:42:07, agrieve wrote:
> I think this probably has reached "silent approval" in that no one has voiced
> concerns yet on the post from the ML. To move forward with this though, I'll
> plan to:
> a) Add no-op configs for hide_all_but_jni and hide_all_but_jni_onload
> b) Roll ffmpeg and angle with the config changes
> c) Ensure rolls stick (they will bring in other commits)
> d) Land this change.

Sounds good to me.

[agrieve, 2017-01-19 22:25:25]

On 2017/01/19 17:55:37, Torne wrote:
> On 2017/01/19 17:42:07, agrieve wrote:
> > I think this probably has reached "silent approval" in that no one has
voiced
> > concerns yet on the post from the ML. To move forward with this though, I'll
> > plan to:
> > a) Add no-op configs for hide_all_but_jni and hide_all_but_jni_onload
> > b) Roll ffmpeg and angle with the config changes
> > c) Ensure rolls stick (they will bring in other commits)
> > d) Land this change.
> 
> Sounds good to me.

a) is here: https://codereview.chromium.org/2648543003/

[digit1, 2017-01-20 09:04:06]

> > Can you make this change more conditional on the type of build being
> performed?
> What conditions do you think would be appropriate?

I simply meant not to hide the symbols in component builds, keep what you're
doing on the non-component ones. Nothing more complicated than that. Sorry if I
wasn't clear :)

[agrieve, 2017-02-01 15:42:49]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-01 15:43:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[agrieve, 2017-02-01 15:44:33]

I think this is ready to go now.

[commit-bot: I haz the power, 2017-02-01 16:48:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-01 16:48:18]

Dry run: This issue passed the CQ dry run.

[Torne, 2017-02-02 12:29:38]

LGTM - any breakage from this should be pretty obvious if something got missed,
and it looks reasonable.

[agrieve, 2017-02-02 14:48:52]

agrieve@chromium.org changed reviewers:
+ brettw@chromium.org

[agrieve, 2017-02-02 14:48:53]

brettw for OWNERS

[brettw, 2017-02-08 05:01:42]

lgtm

[agrieve, 2017-02-08 12:34:48]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2017-02-08 12:35:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 14:35:52]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486557288478430,
"parent_rev": "933ddc2ee726091a7be2b1d2cb2a273231813313", "commit_rev":
"6888d5002b2eabe4fac226f0e5124fbd62bdd215"}

[commit-bot: I haz the power, 2017-02-08 14:36:25]

Description was changed from

==========
Android: Use linker script to hide all non-JNI symbols

The linker script is applied to all shared_library() targets by default,
just like the hide_jni one was.

This reduces the size of libchrome.so by 220kb.

BUG=448386
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Android: Use linker script to hide all non-JNI symbols

The linker script is applied to all shared_library() targets by default,
just like the hide_jni one was.

This reduces the size of libchrome.so by 220kb.

BUG=448386
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2633593004
Cr-Commit-Position: refs/heads/master@{#448988}
Committed:
https://chromium.googlesource.com/chromium/src/+/6888d5002b2eabe4fac226f0e512...
==========

[commit-bot: I haz the power, 2017-02-08 14:36:27]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/6888d5002b2eabe4fac226f0e512...

[engedy, 2017-02-08 17:44:00]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2685933002/ by engedy@chromium.org.

The reason for reverting is: Speculative revert, potential culprit for
webkit_tests crashing on "WebKit Android (Nexus4)". 

Stack traces show not much else than JNI managed->native transitions and
syscalls..