Use combined native/manual JNI registration.

Use combined native/manual JNI registration.

Add a new mode to the JNI generator which emits both native JNI exports
and manual registration code, and use this as the default mode (except
when compiling with clang as a clang bug prevents this from working at
present).

Native JNI exports are stripped from binaries by default to enforce that
the correct manual registration code is called (and to save increasing
the dynamic symbol table size), except for binaries that explicitly opt
in to using native exports (i.e. libwebviewchromium). Native exports are
not compatible with the crazy linker, so cannot be used universally.

The WebView-specific call to InitReplacementClassLoader, required by
native export mode, has been moved to base to make it easy for other
binaries to experiment with that mode.

Manual JNI registration can be disabled with a call to
base::android::DisableManualJniRegistration at the beginning of
JNI_OnLoad and this has been added to WebView. We plan to refactor the
Android library entry points to make it possible to avoid needing this
flag by just not calling JNI registration but the work is still ongoing;
the flag gets us the desired WebView startup time improvement in the
meantime.

BUG=442327
Committed: https://crrev.com/103bf19477cc121eee7a3c2bdc221f2874c971c3
Cr-Commit-Position: refs/heads/master@{#317434}

[Torne, 2015-02-12 18:28:50]

This is not ready for review and currently incorporates several other inflight
CLs for easy bot runs which will be landed separately first.

[michaelbai, 2015-02-13 04:36:07]

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

[michaelbai, 2015-02-13 04:36:07]

https://codereview.chromium.org/920883002/diff/1/base/android/base_jni_onload.cc
File base/android/base_jni_onload.cc (right):

https://codereview.chromium.org/920883002/diff/1/base/android/base_jni_onload...
base/android/base_jni_onload.cc:53: base::android::GetClassLoader(env));
Move this to BaseJNIOnLoadDelegate::Init()

[Torne, 2015-02-13 14:08:04]

https://codereview.chromium.org/920883002/diff/1/base/android/base_jni_onload.cc
File base/android/base_jni_onload.cc (right):

https://codereview.chromium.org/920883002/diff/1/base/android/base_jni_onload...
base/android/base_jni_onload.cc:53: base::android::GetClassLoader(env));
On 2015/02/13 04:36:07, michaelbai wrote:
> Move this to BaseJNIOnLoadDelegate::Init()

Hm. The JNI environment isn't passed to the init functions, which makes this
mildly annoying. Do none of them need it, or are they just all getting it with
AttachCurrentThread? If several of the init functions need it, then maybe it
should get passed to Init() as well as to RegisterJNI()..

[Torne, 2015-02-13 14:09:18]

The clang failure appears to be a clang bug - it believes the functions in
question are unused, but they are in fact required because there are other
functions aliased to them. GCC gets this right, so it should be legit. :)

[Torne, 2015-02-16 14:58:11]

On 2015/02/13 14:09:18, Torne wrote:
> The clang failure appears to be a clang bug - it believes the functions in
> question are unused, but they are in fact required because there are other
> functions aliased to them. GCC gets this right, so it should be legit. :)

Filed clang bug about the warning: http://llvm.org/bugs/show_bug.cgi?id=22602

For now I guess I'll have to stick some more __attribute__(("unused")) in the
generator output to work around it.

[Torne, 2015-02-20 10:43:36]

On 2015/02/13 04:36:07, michaelbai wrote:
>
https://codereview.chromium.org/920883002/diff/1/base/android/base_jni_onload.cc
> File base/android/base_jni_onload.cc (right):
> 
>
https://codereview.chromium.org/920883002/diff/1/base/android/base_jni_onload...
> base/android/base_jni_onload.cc:53: base::android::GetClassLoader(env));
> Move this to BaseJNIOnLoadDelegate::Init()

Done.

[Torne, 2015-02-20 11:52:10]

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

[Torne, 2015-02-20 11:52:11]

Not quite finished with this yet, still need to add some mechanism to skip doing
the manual registration for binaries using native exports, but it should work
and be generally reasonable.

[Torne, 2015-02-20 11:52:40]

Oh also this still includes the dependent CL
https://codereview.chromium.org/938373002/ - will rebase to eliminate that once
the other CL lands.

[Torne, 2015-02-20 13:34:49]

On 2015/02/20 11:52:40, Torne wrote:
> Oh also this still includes the dependent CL
> https://codereview.chromium.org/938373002/ - will rebase to eliminate that
once
> the other CL lands.

Rebased; this CL is now standalone.

[Torne, 2015-02-20 14:12:48]

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

[Torne, 2015-02-20 14:12:50]

+cjhopman to cover remaining OWNERS

Chris, Ross, Michael, this is now ready for review and all the dependent CLs
have landed. The bug describes more about what I'm trying to achieve here, but
the short story is: with this change, WebView can use automatic JNI symbol
lookup and skip the time-consuming manual JNI registration at startup, but other
binaries continue to do manual registration and thus retain crazy linker
compatibility.

I still need to test the component build and the full Chrome build locally, so I
won't submit it until I have verified those are both definitely working
correctly, but can you review this as soon as you have free time? We'd very very
much like to get this in to M42 to enable us to get rid of the downstream
WebView builds as soon as possible.

https://codereview.chromium.org/920883002/diff/140001/base/android/jni_genera...
File base/android/jni_generator/jni_generator.py (right):

https://codereview.chromium.org/920883002/diff/140001/base/android/jni_genera...
base/android/jni_generator/jni_generator.py:925: ${EARLY_EXIT}${CLASSES}
The newlines here are being handled in this way to avoid rebaselining all the
JNI generator tests in this CL.

[Torne, 2015-02-20 14:53:16]

android_chromium_gn_compile_rel failure is
https://code.google.com/p/chromium/issues/detail?id=460463 (infra problem).

[Torne, 2015-02-20 15:21:15]

Tested component and non-component builds of content shell, chrome shell, and
webview shell; they all work. Tested official release build of the full chrome
apk and that also works.

I'd like to test the startup performance of the full chrome apk with this change
(should be no difference) but can't get the perf tests to run on my machine,
I've emailed the internal team list to look for help with that.

[rmcilroy, 2015-02-20 17:19:35]

A couple of nits, but LGTM, thanks!

https://codereview.chromium.org/920883002/diff/140001/base/android/jni_genera...
File base/android/jni_generator/jni_generator.py (right):

https://codereview.chromium.org/920883002/diff/140001/base/android/jni_genera...
base/android/jni_generator/jni_generator.py:925: ${EARLY_EXIT}${CLASSES}
On 2015/02/20 14:12:49, Torne wrote:
> The newlines here are being handled in this way to avoid rebaselining all the
> JNI generator tests in this CL.

Can you create a followup CL which does this rebasing please (after branch point
is fine).

https://codereview.chromium.org/920883002/diff/140001/base/android/jni_genera...
base/android/jni_generator/jni_generator.py:940: if
(base::android::IsManualJniRegistrationDisabled()) return true;
nit - add a new line to the generated code after this.

https://codereview.chromium.org/920883002/diff/140001/base/android/jni_genera...
File base/android/jni_generator/jni_generator_tests.py (right):

https://codereview.chromium.org/920883002/diff/140001/base/android/jni_genera...
base/android/jni_generator/jni_generator_tests.py:1100:
self.assertGoldenTextEquals(jni_from_java.GetContent())
Could we extract the common code from testNativeExportsOptionalOption and
testNativeExportsOption into a common function and call it with the appropriate
parameters in the two tests?

https://codereview.chromium.org/920883002/diff/140001/base/android/jni_genera...
File base/android/jni_generator/testNativeExportsOptionalOption.golden (right):

https://codereview.chromium.org/920883002/diff/140001/base/android/jni_genera...
base/android/jni_generator/testNativeExportsOptionalOption.golden:241:
static_cast<base::subtle::AtomicWord>(env->NewWeakGlobalRef(clazz));
I realize it wasn't you, but the intentation seems wrong for this Release_Store
(presumably also in NativeExportsOption).  Could you fix it while your here?

[cjhopman, 2015-02-20 19:11:55]

lgtm. The gn failure is a flake.

https://codereview.chromium.org/920883002/diff/140001/base/android/jni_android.h
File base/android/jni_android.h (right):

https://codereview.chromium.org/920883002/diff/140001/base/android/jni_androi...
base/android/jni_android.h:25: // JNI exports for startup performance. This is
not compatible with the crazy
Why isn't it compatible with the crazy linker?

[Torne, 2015-02-20 19:17:44]

On 2015/02/20 19:11:55, cjhopman wrote:
> lgtm. The gn failure is a flake.
> 
>
https://codereview.chromium.org/920883002/diff/140001/base/android/jni_android.h
> File base/android/jni_android.h (right):
> 
>
https://codereview.chromium.org/920883002/diff/140001/base/android/jni_androi...
> base/android/jni_android.h:25: // JNI exports for startup performance. This is
> not compatible with the crazy
> Why isn't it compatible with the crazy linker?

When the JVM calls dlsym() to look up the exported native JNI methods, it calls
the standard libc version of dlsym() because the JVM isn't linked to
libchromium_android_linker.so, and so the crazy linker doesn't get a chance to
search inside libraries that were loaded by the crazy linker. Thus, the JVM only
searches a bunch of other libraries unrelated to chromium, and doesn't find the
symbol.

[Torne, 2015-02-20 19:18:28]

On 2015/02/20 19:17:44, Torne wrote:
> On 2015/02/20 19:11:55, cjhopman wrote:
> > lgtm. The gn failure is a flake.
> > 
> >
>
https://codereview.chromium.org/920883002/diff/140001/base/android/jni_android.h
> > File base/android/jni_android.h (right):
> > 
> >
>
https://codereview.chromium.org/920883002/diff/140001/base/android/jni_androi...
> > base/android/jni_android.h:25: // JNI exports for startup performance. This
is
> > not compatible with the crazy
> > Why isn't it compatible with the crazy linker?
> 
> When the JVM calls dlsym() to look up the exported native JNI methods, it
calls
> the standard libc version of dlsym() because the JVM isn't linked to
> libchromium_android_linker.so, and so the crazy linker doesn't get a chance to
> search inside libraries that were loaded by the crazy linker. Thus, the JVM
only
> searches a bunch of other libraries unrelated to chromium, and doesn't find
the
> symbol.

If it wasn't for that, we wouldn't be doing this all optionally in the first
place, probably; we would have switched over to letting the JVM do all the
lookups for all chrome targets. :/

[Torne, 2015-02-20 21:35:30]

I've now completed a fairly long run of chrome_shell startup tests with and
without this patch and it does not seem to have any impact on chrome_shell's
startup performance. This should also be true for other chromium targets since
JNI initialisation is of a similar size across all of the more full browser
binaries.

[Torne, 2015-02-20 21:52:57]

New patchsets have been uploaded after l-g-t-m from
rmcilroy@chromium.org,cjhopman@chromium.org

[Torne, 2015-02-20 21:53:46]

I will take care of all the formatting/indentation in the JNI generator in a
followup CL on monday. I promise. :p

https://codereview.chromium.org/920883002/diff/140001/base/android/jni_genera...
File base/android/jni_generator/jni_generator_tests.py (right):

https://codereview.chromium.org/920883002/diff/140001/base/android/jni_genera...
base/android/jni_generator/jni_generator_tests.py:1100:
self.assertGoldenTextEquals(jni_from_java.GetContent())
On 2015/02/20 17:19:35, rmcilroy wrote:
> Could we extract the common code from testNativeExportsOptionalOption and
> testNativeExportsOption into a common function and call it with the
appropriate
> parameters in the two tests?

Done.

[Torne, 2015-02-20 21:53:54]

The CQ bit was checked by torne@chromium.org

[commit-bot: I haz the power, 2015-02-20 21:54:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/920883002/160001

[commit-bot: I haz the power, 2015-02-20 23:06:51]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-02-20 23:07:33]

Patchset 9 (id:??) landed as
https://crrev.com/103bf19477cc121eee7a3c2bdc221f2874c971c3
Cr-Commit-Position: refs/heads/master@{#317434}