Selectively perform JNI registration in render processes on Android.

Selectively perform JNI registration in render processes on Android.

We currently register all JNI methods regardless of the process type
even though little JNI is needed in render processes. This patch
changes the JNI registration behavior for render processes so that
registration is only performed for Java classes that are annotated with
"@MainDex". Other Chrome processes, Webview, and Monochrome are not
affected by this change. 

BUG=657024
Review-Url: https://codereview.chromium.org/2501193003
Cr-Commit-Position: refs/heads/master@{#447315}
Committed: https://chromium.googlesource.com/chromium/src/+/5b04fbad6a653550a4540b70c2aa9647222e672a

[estevenson, 2016-11-14 23:18:52]

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

[commit-bot: I haz the power, 2016-11-14 23:19:47]

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, 2016-11-15 01:26:38]

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

[commit-bot: I haz the power, 2016-11-15 01:26:39]

Dry run: This issue passed the CQ dry run.

[agrieve, 2016-12-07 20:46:18]

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

[agrieve, 2016-12-07 20:46:19]

https://codereview.chromium.org/2501193003/diff/100001/base/android/jni_gener...
File base/android/jni_generator/SampleForTests_jni.golden (right):

https://codereview.chromium.org/2501193003/diff/100001/base/android/jni_gener...
base/android/jni_generator/SampleForTests_jni.golden:488: if
(base::android::IsMultidexEnabled(env) &&
This logic should hold whether or not multidex is enabled.

https://codereview.chromium.org/2501193003/diff/100001/base/android/jni_gener...
File base/android/jni_generator/jni_generator.py (right):

https://codereview.chromium.org/2501193003/diff/100001/base/android/jni_gener...
base/android/jni_generator/jni_generator.py:709: def InMainDex(contents):
nit: odd name. I'd probably just inline the function.

[estevenson, 2016-12-07 22:59:26]

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

[commit-bot: I haz the power, 2016-12-07 22:59:44]

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

[estevenson, 2016-12-07 23:00:05]

https://codereview.chromium.org/2501193003/diff/100001/base/android/jni_gener...
File base/android/jni_generator/SampleForTests_jni.golden (right):

https://codereview.chromium.org/2501193003/diff/100001/base/android/jni_gener...
base/android/jni_generator/SampleForTests_jni.golden:488: if
(base::android::IsMultidexEnabled(env) &&
On 2016/12/07 20:46:19, agrieve (OOO Dec 8) wrote:
> This logic should hold whether or not multidex is enabled.

Done.

https://codereview.chromium.org/2501193003/diff/100001/base/android/jni_gener...
File base/android/jni_generator/jni_generator.py (right):

https://codereview.chromium.org/2501193003/diff/100001/base/android/jni_gener...
base/android/jni_generator/jni_generator.py:709: def InMainDex(contents):
On 2016/12/07 20:46:19, agrieve (OOO Dec 8) wrote:
> nit: odd name. I'd probably just inline the function.

Done.

[estevenson, 2016-12-07 23:02:32]

Description was changed from

==========
Conditionally register JNI based on process type.

When multidex is turned on, the jni_generator.py script will only
register JNI in non-browser processes if the java source file contains
@MainDex.

BUG=657024
==========

to

==========
Conditionally register JNI based on process type.

Generally, only the browser process needs to complete JNI registration.
This CL modifies jni_generator.py to only register classes annotated
with "@MainDex" when in non-browser processes, which should make
registration a little quicker for non-browser processes.

BUG=657024
==========

[commit-bot: I haz the power, 2016-12-08 00:29:28]

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

[commit-bot: I haz the power, 2016-12-08 00:29:29]

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

[estevenson, 2016-12-08 19:56:24]

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

[commit-bot: I haz the power, 2016-12-08 19:56:57]

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, 2016-12-08 21:41:08]

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

[commit-bot: I haz the power, 2016-12-08 21:41:08]

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

[estevenson, 2016-12-12 23:57:20]

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

[commit-bot: I haz the power, 2016-12-12 23:58:44]

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, 2016-12-13 00:26:50]

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

[commit-bot: I haz the power, 2016-12-13 00:26:51]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[estevenson, 2016-12-13 16:33:32]

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

[estevenson, 2016-12-13 16:33:33]

ptal torne!

This change would be required in order to turn on multidex in release, but it
seems like a worthwhile change even if we don't end up doing that. wdyt? 

The only concern I have is that there may be codepaths where we need to perform
some JNI registration in non-browser processes for classes missing "@MainDex"
and we have no way to catch them.

[Torne, 2016-12-14 11:40:25]

One main comment (which if you agree might make some of the below comments
irrelevant): I don't like the GetLibraryProcessType API in the first place, it
has too many choices and it's not clear which are significant in which contexts.

I think we should remove the LibraryProcessType concept entirely and replace it
with something that's explicitly about controlling JNI registration and not used
for any other purpose. The only three values this would need are:

1) Register all JNI functions.
2) Register "core" JNI functions (not sure what to call this, but basically I
mean main-dex/non-browser).
3) Don't do any JNI registration at all.

This could replace the existing "manual jni registration disabled" thing as
well, giving us just one of two early exit conditions:
a) For @MainDex classes, register the class unless the registration mode is
"none"
b) For non-@MainDex classes, register the class if the registration mode is
"all".

What do you think?

(Also, in the process of doing this, we should also make Monochrome behave like
webview and never do any JNI registration at all).

https://codereview.chromium.org/2501193003/diff/160001/base/android/jni_gener...
File base/android/jni_generator/jni_generator.py (right):

https://codereview.chromium.org/2501193003/diff/160001/base/android/jni_gener...
base/android/jni_generator/jni_generator.py:671: maindex = '@MainDex' in
contents
The other Extract* functions are doing more than just searching for a substring
match; several of them are already recognising annotations I think? Is there a
better way to do this than just looking for a substring?

https://codereview.chromium.org/2501193003/diff/160001/base/android/jni_gener...
base/android/jni_generator/jni_generator.py:870:
base::android::LibraryProcessType proc_type =
Can you put this in jni_generator_helper.h as an inline function or something?
The logic here probably shouldn't be hardcoded in the generator itself - the
generator just needs to make the "main dex or not" call.

Maybe something like "RegisterMainDexClassesOnly()"?

https://codereview.chromium.org/2501193003/diff/160001/base/android/jni_gener...
base/android/jni_generator/jni_generator.py:871:
base::android::GetLibraryProcessType(env);
GetLibraryProcessType is a JNI call; if we're going to hit it a bunch of times
in startup like this then we should probably explicitly store it in native
instead.

https://codereview.chromium.org/2501193003/diff/160001/base/android/jni_gener...
base/android/jni_generator/jni_generator.py:873: proc_type !=
base::android::PROCESS_UNINITIALIZED) {
Is there really a case where the process type is unitialized? Can we not just
crash at an earlier point if it's not set, instead of trying to make it safe?

[estevenson, 2016-12-14 19:20:07]

On 2016/12/14 11:40:25, Torne wrote:
> One main comment (which if you agree might make some of the below comments
> irrelevant): I don't like the GetLibraryProcessType API in the first place, it
> has too many choices and it's not clear which are significant in which
contexts.
> 
> I think we should remove the LibraryProcessType concept entirely and replace
it
> with something that's explicitly about controlling JNI registration and not
used
> for any other purpose. The only three values this would need are:
> 
> 1) Register all JNI functions.
> 2) Register "core" JNI functions (not sure what to call this, but basically I
> mean main-dex/non-browser).
> 3) Don't do any JNI registration at all.
> 
> This could replace the existing "manual jni registration disabled" thing as
> well, giving us just one of two early exit conditions:
> a) For @MainDex classes, register the class unless the registration mode is
> "none"
> b) For non-@MainDex classes, register the class if the registration mode is
> "all".
> 
> What do you think?

This sounds better to me as well! I should have tried this approach after you
first suggested it. To make sure I understand correctly:

(1) The LibraryProcessType enum will still be used on the Java side of things
but removed from native - except for (3)
(2) Define an enum JniRegistrationType with values [ALL, SELECTIVE, NONE]
(naming tbd).
(3) Set this value once in OnLoad entry hooks 
(4) Remove IsManualJniRegistrationDisabled(), but add a DCHECK using the
jni_generator when registration type == NONE to make sure registration isn't
erroneously performed (monochrome + webview)
(5) Similar to this CL, selectively register if registration type == SELECTIVE 
(6) Register everything if registration type == ALL

[Torne, 2016-12-15 15:58:39]

On 2016/12/14 19:20:07, estevenson wrote:
> On 2016/12/14 11:40:25, Torne wrote:
> > One main comment (which if you agree might make some of the below comments
> > irrelevant): I don't like the GetLibraryProcessType API in the first place,
it
> > has too many choices and it's not clear which are significant in which
> contexts.
> > 
> > I think we should remove the LibraryProcessType concept entirely and replace
> it
> > with something that's explicitly about controlling JNI registration and not
> used
> > for any other purpose. The only three values this would need are:
> > 
> > 1) Register all JNI functions.
> > 2) Register "core" JNI functions (not sure what to call this, but basically
I
> > mean main-dex/non-browser).
> > 3) Don't do any JNI registration at all.
> > 
> > This could replace the existing "manual jni registration disabled" thing as
> > well, giving us just one of two early exit conditions:
> > a) For @MainDex classes, register the class unless the registration mode is
> > "none"
> > b) For non-@MainDex classes, register the class if the registration mode is
> > "all".
> > 
> > What do you think?
> 
> This sounds better to me as well! I should have tried this approach after you
> first suggested it. To make sure I understand correctly:
> 
> (1) The LibraryProcessType enum will still be used on the Java side of things
> but removed from native - except for (3)

Ideally we'd get rid of it entirely. I can't see it used for very much other
than passing it around to set it in the first place, and JNI registration (am I
missing something?)

If there's other uses we can look at them and see whether they really care about
the process type, or if the process type is being used as a way to infer some
other property that it would be better to directly refer to instead.

> (2) Define an enum JniRegistrationType with values [ALL, SELECTIVE, NONE]
> (naming tbd).
> (3) Set this value once in OnLoad entry hooks 
> (4) Remove IsManualJniRegistrationDisabled(), but add a DCHECK using the
> jni_generator when registration type == NONE to make sure registration isn't
> erroneously performed (monochrome + webview)

You may want to convert that to a DCHECK in a separate CL because I suspect the
code doesn't actually do this right at the moment. I'd suggest we start by
simply replacing IsManualJniRegistrationDisabled with early-outing if
registration type is NONE, and then try to turn it into a DCHECK in a followup
CL so we can get the MainDex issue resolved ASAP.

> (5) Similar to this CL, selectively register if registration type == SELECTIVE

> (6) Register everything if registration type == ALL


Yes, other than those two specific comments it sounds like you're thinking of
exactly what I meant. As per the other comments I left on this CL, it's
preferable if as much of the actual chromium-specific logic as possible goes
into an inline function/macro defined in jni_generator_helpers.h instead of
being inlined into jni_generator.py, and the generator is just responsible for
using the @MainDex annotation to decide whether to insert a call to the relevant
selective-registration-check function or not.

[estevenson, 2016-12-15 23:01:22]

On 2016/12/15 15:58:39, Torne wrote:
> On 2016/12/14 19:20:07, estevenson wrote:
> > On 2016/12/14 11:40:25, Torne wrote:
> > > One main comment (which if you agree might make some of the below comments
> > > irrelevant): I don't like the GetLibraryProcessType API in the first
place,
> it
> > > has too many choices and it's not clear which are significant in which
> > contexts.
> > > 
> > > I think we should remove the LibraryProcessType concept entirely and
replace
> > it
> > > with something that's explicitly about controlling JNI registration and
not
> > used
> > > for any other purpose. The only three values this would need are:
> > > 
> > > 1) Register all JNI functions.
> > > 2) Register "core" JNI functions (not sure what to call this, but
basically
> I
> > > mean main-dex/non-browser).
> > > 3) Don't do any JNI registration at all.
> > > 
> > > This could replace the existing "manual jni registration disabled" thing
as
> > > well, giving us just one of two early exit conditions:
> > > a) For @MainDex classes, register the class unless the registration mode
is
> > > "none"
> > > b) For non-@MainDex classes, register the class if the registration mode
is
> > > "all".
> > > 
> > > What do you think?
> > 
> > This sounds better to me as well! I should have tried this approach after
you
> > first suggested it. To make sure I understand correctly:
> > 
> > (1) The LibraryProcessType enum will still be used on the Java side of
things
> > but removed from native - except for (3)
> 
> Ideally we'd get rid of it entirely. I can't see it used for very much other
> than passing it around to set it in the first place, and JNI registration (am
I
> missing something?)
> 
> If there's other uses we can look at them and see whether they really care
about
> the process type, or if the process type is being used as a way to infer some
> other property that it would be better to directly refer to instead.
> 
> > (2) Define an enum JniRegistrationType with values [ALL, SELECTIVE, NONE]
> > (naming tbd).
> > (3) Set this value once in OnLoad entry hooks 
> > (4) Remove IsManualJniRegistrationDisabled(), but add a DCHECK using the
> > jni_generator when registration type == NONE to make sure registration isn't
> > erroneously performed (monochrome + webview)
> 
> You may want to convert that to a DCHECK in a separate CL because I suspect
the
> code doesn't actually do this right at the moment. I'd suggest we start by
> simply replacing IsManualJniRegistrationDisabled with early-outing if
> registration type is NONE, and then try to turn it into a DCHECK in a followup
> CL so we can get the MainDex issue resolved ASAP.
> 
> > (5) Similar to this CL, selectively register if registration type ==
SELECTIVE
> 
> > (6) Register everything if registration type == ALL
> 
> 
> Yes, other than those two specific comments it sounds like you're thinking of
> exactly what I meant. As per the other comments I left on this CL, it's
> preferable if as much of the actual chromium-specific logic as possible goes
> into an inline function/macro defined in jni_generator_helpers.h instead of
> being inlined into jni_generator.py, and the generator is just responsible for
> using the @MainDex annotation to decide whether to insert a call to the
relevant
> selective-registration-check function or not.

Sounds good, thanks for the advice. I'm thinking I'll do this in 3 CLs:

1) Add JniRegistrationType and support for maindex registration (this CL)
2) Change webview & monochrome to not register anything and add a DCHECK to
verify
3) Remove the concept of LibraryProcessType if possible or at least move it to
Java only

[Torne, 2016-12-16 11:39:34]

On 2016/12/15 23:01:22, estevenson wrote:
> Sounds good, thanks for the advice. I'm thinking I'll do this in 3 CLs:
> 
> 1) Add JniRegistrationType and support for maindex registration (this CL)
> 2) Change webview & monochrome to not register anything and add a DCHECK to
> verify
> 3) Remove the concept of LibraryProcessType if possible or at least move it to
> Java only

That seems like a good plan. This is my last day in the office before the
holidays, but I'll review anything you have when I'm back :)

[estevenson, 2016-12-19 22:28:52]

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

[commit-bot: I haz the power, 2016-12-19 22:29:17]

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, 2016-12-19 23:22:53]

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

[commit-bot: I haz the power, 2016-12-19 23:22:54]

Dry run: This issue passed the CQ dry run.

[estevenson, 2017-01-04 20:59:20]

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

[commit-bot: I haz the power, 2017-01-04 20:59: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-04 22:42:51]

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

[commit-bot: I haz the power, 2017-01-04 22:42:52]

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

[estevenson, 2017-01-05 22:34:10]

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

[commit-bot: I haz the power, 2017-01-05 22:34:57]

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-05 23:21:28]

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

[commit-bot: I haz the power, 2017-01-05 23:21:29]

Dry run: This issue passed the CQ dry run.

[estevenson, 2017-01-09 05:12:23]

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

[commit-bot: I haz the power, 2017-01-09 05:12:38]

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-09 05:50:27]

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

[commit-bot: I haz the power, 2017-01-09 05:50:28]

Dry run: This issue passed the CQ dry run.

[estevenson, 2017-01-09 13:20:00]

ptal Torne

https://codereview.chromium.org/2501193003/diff/240001/base/android/jni_gener...
File base/android/jni_generator/jni_generator_helper.h (right):

https://codereview.chromium.org/2501193003/diff/240001/base/android/jni_gener...
base/android/jni_generator/jni_generator_helper.h:57: // The registration type
isn't always initialized in tests by this point.
Not sure about this. In instrumentation tests the LibraryProcessType is often
PROCESS_UNINITIALIZED. Is there an error case where we might see a
PROCESS_UNINITIALIZED and assuming we're in a test and registering everything
would be incorrect? 

If so, we don't currently seem to handle that case - so is this an okay
temporary
solution until LibraryProcessType is removed from native?

[Torne, 2017-01-09 16:43:32]

https://codereview.chromium.org/2501193003/diff/240001/base/android/java/src/...
File base/android/java/src/org/chromium/base/CpuFeatures.java (right):

https://codereview.chromium.org/2501193003/diff/240001/base/android/java/src/...
base/android/java/src/org/chromium/base/CpuFeatures.java:25: @MainDex
Might be cleaner to land the added @MainDex requirements in a separate CL first,
since presumably it doesn't depend on anything?

https://codereview.chromium.org/2501193003/diff/240001/base/android/jni_gener...
File base/android/jni_generator/jni_generator_helper.h (right):

https://codereview.chromium.org/2501193003/diff/240001/base/android/jni_gener...
base/android/jni_generator/jni_generator_helper.h:57: // The registration type
isn't always initialized in tests by this point.
On 2017/01/09 13:20:00, estevenson wrote:
> Not sure about this. In instrumentation tests the LibraryProcessType is often
> PROCESS_UNINITIALIZED. Is there an error case where we might see a
> PROCESS_UNINITIALIZED and assuming we're in a test and registering everything
> would be incorrect? 
> 
> If so, we don't currently seem to handle that case - so is this an okay
> temporary
> solution until LibraryProcessType is removed from native?

I don't think it's okay that we ever see PROCESS_UNINITIALIZED :/
I'm not sure how that could even be happening; it's set by LibraryLoader.

https://codereview.chromium.org/2501193003/diff/240001/chrome/browser/android...
File chrome/browser/android/monochrome_entry_point.cc (right):

https://codereview.chromium.org/2501193003/diff/240001/chrome/browser/android...
chrome/browser/android/monochrome_entry_point.cc:18: if
(base::android::GetJniRegistrationType() ==
Don't use the JNI registration type to determine whether to call webview or
chrome init. This will have to continue depending on process type for now until
someone does the work to make monochrome's entry point sensible.

https://codereview.chromium.org/2501193003/diff/240001/third_party/gvr-androi...
File third_party/gvr-android-sdk/display_synchronizer_jni.h (right):

https://codereview.chromium.org/2501193003/diff/240001/third_party/gvr-androi...
third_party/gvr-android-sdk/display_synchronizer_jni.h:114: if
(jni_generator::ShouldSkipJniRegistration(false))
Is it okay to fork this third party code? :/

[estevenson, 2017-01-10 16:51:58]

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

[commit-bot: I haz the power, 2017-01-10 16:52:21]

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-10 18:36:54]

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

[commit-bot: I haz the power, 2017-01-10 18:36:55]

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

[estevenson, 2017-01-11 21:21:37]

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

[commit-bot: I haz the power, 2017-01-11 21:22:37]

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-11 23:07:27]

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

[commit-bot: I haz the power, 2017-01-11 23:07:28]

Dry run: This issue passed the CQ dry run.

[estevenson, 2017-01-12 03:46:48]

estevenson@chromium.org changed reviewers:
+ bshe@chromium.org

[estevenson, 2017-01-12 03:46:49]

bshe@chromium.org: Please review changes in //third_party/gvr-android-sdk/*

https://codereview.chromium.org/2501193003/diff/240001/base/android/java/src/...
File base/android/java/src/org/chromium/base/CpuFeatures.java (right):

https://codereview.chromium.org/2501193003/diff/240001/base/android/java/src/...
base/android/java/src/org/chromium/base/CpuFeatures.java:25: @MainDex
On 2017/01/09 16:43:32, Torne wrote:
> Might be cleaner to land the added @MainDex requirements in a separate CL
first,
> since presumably it doesn't depend on anything?

Done.

https://codereview.chromium.org/2501193003/diff/240001/base/android/jni_gener...
File base/android/jni_generator/jni_generator_helper.h (right):

https://codereview.chromium.org/2501193003/diff/240001/base/android/jni_gener...
base/android/jni_generator/jni_generator_helper.h:57: // The registration type
isn't always initialized in tests by this point.
On 2017/01/09 16:43:32, Torne wrote:
> On 2017/01/09 13:20:00, estevenson wrote:
> > Not sure about this. In instrumentation tests the LibraryProcessType is
often
> > PROCESS_UNINITIALIZED. Is there an error case where we might see a
> > PROCESS_UNINITIALIZED and assuming we're in a test and registering
everything
> > would be incorrect? 
> > 
> > If so, we don't currently seem to handle that case - so is this an okay
> > temporary
> > solution until LibraryProcessType is removed from native?
> 
> I don't think it's okay that we ever see PROCESS_UNINITIALIZED :/
> I'm not sure how that could even be happening; it's set by LibraryLoader.

Added init code everywhere necessary for tests.

https://codereview.chromium.org/2501193003/diff/240001/chrome/browser/android...
File chrome/browser/android/monochrome_entry_point.cc (right):

https://codereview.chromium.org/2501193003/diff/240001/chrome/browser/android...
chrome/browser/android/monochrome_entry_point.cc:18: if
(base::android::GetJniRegistrationType() ==
On 2017/01/09 16:43:32, Torne wrote:
> Don't use the JNI registration type to determine whether to call webview or
> chrome init. This will have to continue depending on process type for now
until
> someone does the work to make monochrome's entry point sensible.

Done.

https://codereview.chromium.org/2501193003/diff/240001/third_party/gvr-androi...
File third_party/gvr-android-sdk/display_synchronizer_jni.h (right):

https://codereview.chromium.org/2501193003/diff/240001/third_party/gvr-androi...
third_party/gvr-android-sdk/display_synchronizer_jni.h:114: if
(jni_generator::ShouldSkipJniRegistration(false))
On 2017/01/09 16:43:32, Torne wrote:
> Is it okay to fork this third party code? :/

Not sure, these files already have local modifications as stated in the README.
Added an OWNER for gvr-android-sdk for review.

[Torne, 2017-01-12 13:48:06]

This will clash with https://codereview.chromium.org/2620303004/ which is
touching a lot of the same functions.. (though it doesn't *logically* conflict).

https://codereview.chromium.org/2501193003/diff/280001/base/android/jni_andro...
File base/android/jni_android.cc (right):

https://codereview.chromium.org/2501193003/diff/280001/base/android/jni_andro...
base/android/jni_android.cc:43: void InitJniRegistrationType(JNIEnv* env, bool
force_registration) {
I assume you have force_registration here to handle the cases where
LibraryProcessType isn't set in time?

Maybe it'd be nicer to just skip the step of basing this on the process type at
all, and just have Java tell native what registration is required?

https://codereview.chromium.org/2501193003/diff/280001/content/shell/android/...
File content/shell/android/shell_library_loader.cc (right):

https://codereview.chromium.org/2501193003/diff/280001/content/shell/android/...
content/shell/android/shell_library_loader.cc:32: true);
Seems like we shouldn't have to force it for the shell?

[bshe, 2017-01-12 16:40:50]

On 2017/01/12 13:48:06, Torne wrote:
> This will clash with https://codereview.chromium.org/2620303004/ which is
> touching a lot of the same functions.. (though it doesn't *logically*
conflict).
> 
>
https://codereview.chromium.org/2501193003/diff/280001/base/android/jni_andro...
> File base/android/jni_android.cc (right):
> 
>
https://codereview.chromium.org/2501193003/diff/280001/base/android/jni_andro...
> base/android/jni_android.cc:43: void InitJniRegistrationType(JNIEnv* env, bool
> force_registration) {
> I assume you have force_registration here to handle the cases where
> LibraryProcessType isn't set in time?
> 
> Maybe it'd be nicer to just skip the step of basing this on the process type
at
> all, and just have Java tell native what registration is required?
> 
>
https://codereview.chromium.org/2501193003/diff/280001/content/shell/android/...
> File content/shell/android/shell_library_loader.cc (right):
> 
>
https://codereview.chromium.org/2501193003/diff/280001/content/shell/android/...
> content/shell/android/shell_library_loader.cc:32: true);
> Seems like we shouldn't have to force it for the shell?

gvr lgtm

[estevenson, 2017-01-25 23:47:25]

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

[commit-bot: I haz the power, 2017-01-25 23:47:59]

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-26 00:44:09]

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

[commit-bot: I haz the power, 2017-01-26 00:44:10]

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

[estevenson, 2017-01-27 22:43:45]

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

[commit-bot: I haz the power, 2017-01-27 22:44:33]

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-28 00:41:29]

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

[commit-bot: I haz the power, 2017-01-28 00:41:30]

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

[estevenson, 2017-01-29 23:27:29]

Description was changed from

==========
Conditionally register JNI based on process type.

Generally, only the browser process needs to complete JNI registration.
This CL modifies jni_generator.py to only register classes annotated
with "@MainDex" when in non-browser processes, which should make
registration a little quicker for non-browser processes.

BUG=657024
==========

to

==========
Selectively perform JNI registration in render processes on Android.

We currently register all JNI methods regardless of the process type
even though little JNI is needed in render processes. This patch
changes the JNI registration behavior for render processes so that
registration is only performed for Java classes that are annotated with
"@MainDex". Other Chrome processes, Webview, and Monochrome are not
affected by this change. 

BUG=657024
==========

[estevenson, 2017-01-29 23:31:03]

ptal torne! This is a little different than what we spoke about offline but I
think it's better than using LibraryProcessType.

[Torne, 2017-01-30 12:29:56]

This LGTM but it's slightly weird:

https://codereview.chromium.org/2501193003/diff/380001/base/android/jni_andro...
File base/android/jni_android.h (right):

https://codereview.chromium.org/2501193003/diff/380001/base/android/jni_andro...
base/android/jni_android.h:73: BASE_EXPORT void DisableManualJniRegistration();
So with this change we do all registration by default, and the child process
services switch to selective before loading native.

Then, separately, the Monochrome/WebView entry points call this to override it
to none?

That should work and I get why it's easier to do it that way (since it's the
entrypoint of the native code that knows whether it's compiled to need
registration or not). It is slightly weird that the value on the java side and
the native side end up different though..

[estevenson, 2017-01-30 18:04:19]

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

[commit-bot: I haz the power, 2017-01-30 18:04:39]

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

[estevenson, 2017-01-30 19:19:27]

https://codereview.chromium.org/2501193003/diff/380001/base/android/jni_andro...
File base/android/jni_android.h (right):

https://codereview.chromium.org/2501193003/diff/380001/base/android/jni_andro...
base/android/jni_android.h:73: BASE_EXPORT void DisableManualJniRegistration();
On 2017/01/30 12:29:56, Torne wrote:
> So with this change we do all registration by default, and the child process
> services switch to selective before loading native.
> 
> Then, separately, the Monochrome/WebView entry points call this to override it
> to none?
> 
> That should work and I get why it's easier to do it that way (since it's the
> entrypoint of the native code that knows whether it's compiled to need
> registration or not). It is slightly weird that the value on the java side and
> the native side end up different though..

We could remove JniRegistrationType from the Java side + change
JNIUtils.setJniRegistrationType(JniRegistrationType) and
JNIUtils.getJniRegistrationType() to JNIUtils.enableSelectiveJniRegistration()
and JNIUtils.isSelectiveJniRegistrationEnabled().

Then have ONLY the native side worry about JniRegistrationType and set it in
each *_entry_point for more consistency.

Still not optimal, but might be a small improvement, wdyt?

[Torne, 2017-01-30 19:21:39]

On 2017/01/30 19:19:27, estevenson wrote:
>
https://codereview.chromium.org/2501193003/diff/380001/base/android/jni_andro...
> File base/android/jni_android.h (right):
> 
>
https://codereview.chromium.org/2501193003/diff/380001/base/android/jni_andro...
> base/android/jni_android.h:73: BASE_EXPORT void
DisableManualJniRegistration();
> On 2017/01/30 12:29:56, Torne wrote:
> > So with this change we do all registration by default, and the child process
> > services switch to selective before loading native.
> > 
> > Then, separately, the Monochrome/WebView entry points call this to override
it
> > to none?
> > 
> > That should work and I get why it's easier to do it that way (since it's the
> > entrypoint of the native code that knows whether it's compiled to need
> > registration or not). It is slightly weird that the value on the java side
and
> > the native side end up different though..
> 
> We could remove JniRegistrationType from the Java side + change
> JNIUtils.setJniRegistrationType(JniRegistrationType) and
> JNIUtils.getJniRegistrationType() to JNIUtils.enableSelectiveJniRegistration()
> and JNIUtils.isSelectiveJniRegistrationEnabled().
> 
> Then have ONLY the native side worry about JniRegistrationType and set it in
> each *_entry_point for more consistency.
> 
> Still not optimal, but might be a small improvement, wdyt?

Hm, I think that sounds pretty good actually - that way it's pretty clear that
we have a default behaviour and then we override it in a specific way, I think?

[commit-bot: I haz the power, 2017-01-30 19:35:28]

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

[commit-bot: I haz the power, 2017-01-30 19:35:29]

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

[estevenson, 2017-01-30 22:00:34]

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

[commit-bot: I haz the power, 2017-01-30 22:01:18]

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

[estevenson, 2017-01-30 23:14:23]

estevenson@chromium.org changed reviewers:
+ tedchoc@chromium.org

[estevenson, 2017-01-30 23:14:24]

tedchoc@chromium.org: Please review changes in chrome_entry_point.cc,
monochrome_entry_point.cc, and ChildProcessServiceImpl.java (+ any comments on
the approach are welcome as well!)

[commit-bot: I haz the power, 2017-01-30 23:30:49]

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

[commit-bot: I haz the power, 2017-01-30 23:30:50]

Dry run: This issue passed the CQ dry run.

[Torne, 2017-01-31 11:59:26]

Thanks, I think this is pretty understandable and reasonable. LGTM :)

[Ted C, 2017-01-31 17:56:54]

lgtm w/ a few small comments

https://codereview.chromium.org/2501193003/diff/400001/base/android/java/src/...
File base/android/java/src/org/chromium/base/JNIUtils.java (right):

https://codereview.chromium.org/2501193003/diff/400001/base/android/java/src/...
base/android/java/src/org/chromium/base/JNIUtils.java:40:
sSelectiveJniRegistrationEnabled = true;
should we make sSelectiveJniRegistrationEnabled a capital B Boolean?  Set it to
null as default, then do like:

public static boolean isSelectiveJniRegistrationEnabled() {
    if (sSelectiveJniRegistrationEnabled == null)
sSelectiveJniRegistrationEnabled = false;
    return sSelectiveJniRegistrationEnabled;
}

public static void enableSelectiveJniRegistration() {
    assert sSelectiveJniRegistrationEnabled == null;
    sSelectiveJniRegistrationEnabled = true;
}

Then we'll be able to catch regressions if we don't set this correctly.

https://codereview.chromium.org/2501193003/diff/400001/base/android/jni_utils.h
File base/android/jni_utils.h (right):

https://codereview.chromium.org/2501193003/diff/400001/base/android/jni_utils...
base/android/jni_utils.h:21: BASE_EXPORT bool
isSelectiveJniRegistrationEnabled(JNIEnv* env);
comment for this?

https://codereview.chromium.org/2501193003/diff/400001/chrome/browser/android...
File chrome/browser/android/chrome_entry_point.cc (right):

https://codereview.chromium.org/2501193003/diff/400001/chrome/browser/android...
chrome/browser/android/chrome_entry_point.cc:32: base::android::InitVM(vm);
why do we need this InitVM here?  Wondering why the ordering is different from
the monochrome entry point?

https://codereview.chromium.org/2501193003/diff/400001/content/public/android...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(right):

https://codereview.chromium.org/2501193003/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:196:
if ("renderer".equals(CommandLine.getInstance().getSwitchValue("type"))) {
Sad to kill your 100 char perfect line, but I think this should be:

if (ContentSwitches.SWITCH_RENDERER_PROCESS.equals(
        CommandLine.getInstance().getSwitchValue(
                ContentSwitches.SWITCH_PROCESS_TYPE))) {

Also, tangent...any idea why we have ContentSwitches.getSwitchValue and the
corresponding one in CommandLine?

[Torne, 2017-01-31 18:04:50]

https://codereview.chromium.org/2501193003/diff/400001/chrome/browser/android...
File chrome/browser/android/chrome_entry_point.cc (right):

https://codereview.chromium.org/2501193003/diff/400001/chrome/browser/android...
chrome/browser/android/chrome_entry_point.cc:32: base::android::InitVM(vm);
On 2017/01/31 17:56:54, Ted C wrote:
> why do we need this InitVM here?  Wondering why the ordering is different from
> the monochrome entry point?

You need to call InitVM before AttachCurrentThread works, and the
SetJniRegistrationType here is conditional on something that needs that. It'd be
fine for monochrome to do it in the same order, though.

[estevenson, 2017-01-31 18:56:41]

Thanks for the reviews :)

https://codereview.chromium.org/2501193003/diff/400001/base/android/java/src/...
File base/android/java/src/org/chromium/base/JNIUtils.java (right):

https://codereview.chromium.org/2501193003/diff/400001/base/android/java/src/...
base/android/java/src/org/chromium/base/JNIUtils.java:40:
sSelectiveJniRegistrationEnabled = true;
On 2017/01/31 17:56:53, Ted C wrote:
> should we make sSelectiveJniRegistrationEnabled a capital B Boolean?  Set it
to
> null as default, then do like:
> 
> public static boolean isSelectiveJniRegistrationEnabled() {
>     if (sSelectiveJniRegistrationEnabled == null)
> sSelectiveJniRegistrationEnabled = false;
>     return sSelectiveJniRegistrationEnabled;
> }
> 
> public static void enableSelectiveJniRegistration() {
>     assert sSelectiveJniRegistrationEnabled == null;
>     sSelectiveJniRegistrationEnabled = true;
> }
> 
> Then we'll be able to catch regressions if we don't set this correctly.

Good call. Done.

https://codereview.chromium.org/2501193003/diff/400001/base/android/jni_utils.h
File base/android/jni_utils.h (right):

https://codereview.chromium.org/2501193003/diff/400001/base/android/jni_utils...
base/android/jni_utils.h:21: BASE_EXPORT bool
isSelectiveJniRegistrationEnabled(JNIEnv* env);
On 2017/01/31 17:56:54, Ted C wrote:
> comment for this?

Done.

https://codereview.chromium.org/2501193003/diff/400001/chrome/browser/android...
File chrome/browser/android/chrome_entry_point.cc (right):

https://codereview.chromium.org/2501193003/diff/400001/chrome/browser/android...
chrome/browser/android/chrome_entry_point.cc:32: base::android::InitVM(vm);
On 2017/01/31 18:04:50, Torne wrote:
> On 2017/01/31 17:56:54, Ted C wrote:
> > why do we need this InitVM here?  Wondering why the ordering is different
from
> > the monochrome entry point?
> 
> You need to call InitVM before AttachCurrentThread works, and the
> SetJniRegistrationType here is conditional on something that needs that. It'd
be
> fine for monochrome to do it in the same order, though.

Yeah, we need JNIEnv for making calls to Java. I think this currently gets
called here for regular Chrome
(https://cs.chromium.org/chromium/src/base/android/base_jni_onload.cc?q=base_j...)
- we need to leave that regardless for places where tests call
base::android::OnJNIOnLoadRegisterJNI directly.

Changed the order of monochrome_entry_point to match chrome_entry_point.

https://codereview.chromium.org/2501193003/diff/400001/content/public/android...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(right):

https://codereview.chromium.org/2501193003/diff/400001/content/public/android...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:196:
if ("renderer".equals(CommandLine.getInstance().getSwitchValue("type"))) {
On 2017/01/31 17:56:54, Ted C wrote:
> Sad to kill your 100 char perfect line, but I think this should be:
> 
> if (ContentSwitches.SWITCH_RENDERER_PROCESS.equals(
>         CommandLine.getInstance().getSwitchValue(
>                 ContentSwitches.SWITCH_PROCESS_TYPE))) {
> 
> Also, tangent...any idea why we have ContentSwitches.getSwitchValue and the
> corresponding one in CommandLine?

:(. Done.

Not sure, my best guess is that ContentSwitches.getSwitchValue was probably
added so that ChildProcessLauncher can use it
(https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro...)
since the CommandLine instance isn't created until the child process is set up?

[estevenson, 2017-01-31 19:36:15]

The CQ bit was checked by estevenson@chromium.org

[estevenson, 2017-01-31 19:36:15]

The patchset sent to the CQ was uploaded after l-g-t-m from bshe@chromium.org,
tedchoc@chromium.org, torne@chromium.org
Link to the patchset: https://codereview.chromium.org/2501193003/#ps420001
(title: "Address Ted C comments")

[commit-bot: I haz the power, 2017-01-31 19:37:07]

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-31 20:35:00]

CQ is committing da patch.

Bot data: {"patchset_id": 420001, "attempt_start_ts": 1485891375304810,
"parent_rev": "40df344eeb73fa17e450750ef72d373c004f9f62", "commit_rev":
"5b04fbad6a653550a4540b70c2aa9647222e672a"}

[commit-bot: I haz the power, 2017-01-31 20:35:35]

Description was changed from

==========
Selectively perform JNI registration in render processes on Android.

We currently register all JNI methods regardless of the process type
even though little JNI is needed in render processes. This patch
changes the JNI registration behavior for render processes so that
registration is only performed for Java classes that are annotated with
"@MainDex". Other Chrome processes, Webview, and Monochrome are not
affected by this change. 

BUG=657024
==========

to

==========
Selectively perform JNI registration in render processes on Android.

We currently register all JNI methods regardless of the process type
even though little JNI is needed in render processes. This patch
changes the JNI registration behavior for render processes so that
registration is only performed for Java classes that are annotated with
"@MainDex". Other Chrome processes, Webview, and Monochrome are not
affected by this change. 

BUG=657024

Review-Url: https://codereview.chromium.org/2501193003
Cr-Commit-Position: refs/heads/master@{#447315}
Committed:
https://chromium.googlesource.com/chromium/src/+/5b04fbad6a653550a4540b70c2aa...
==========

[commit-bot: I haz the power, 2017-01-31 20:35:36]

Committed patchset #22 (id:420001) as
https://chromium.googlesource.com/chromium/src/+/5b04fbad6a653550a4540b70c2aa...