Split JNI init so native library preloading dosn't cause native init.

Split JNI init so native library preloading dosn't cause native init.

The zygote relies on preloading the shared library. However we can't
determine the process type at this point (for monochrome it could be a
chrome renderer, or a webview renderer), and so current OnJNIOnLoadInit
code that relies on knowing the process type has to be delayed until
LibraryLoader is called, and the process type is known.

To get around this, we split this code in two, and have the first stage
(JNI_OnLoad) register a hook that is run in the second stage, to process
the OnJNIOnLoadInit work.

BUG=

Review-Url: https://codereview.chromium.org/2593653002
Cr-Commit-Position: refs/heads/master@{#441678}
Committed: https://chromium.googlesource.com/chromium/src/+/48fdf7dc65a82510505c68c7e03fba8f3a88009b

[Tobias Sargeant, 2016-12-20 15:00:40]

tobiasjs@chromium.org changed reviewers:
+ bauerb@chromium.org, rsesek@chromium.org, torne@chromium.org

[Tobias Sargeant, 2016-12-20 15:02:32]

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

[commit-bot: I haz the power, 2016-12-20 15:02:42]

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

[Bernhard Bauer, 2016-12-20 15:23:24]

LGTM

[commit-bot: I haz the power, 2016-12-20 16:28:07]

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

[commit-bot: I haz the power, 2016-12-20 16:28: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_...)

[Robert Sesek, 2016-12-20 17:48:29]

I patched this in locally and am getting browser-side crashes:

12-20 12:46:18.944 19662 20611 W System.err: java.lang.ClassNotFoundException:
Didn't find class "org.chromium.base.PathUtils" on path: DexPathList[[directory
"."],nativeLibraryDirectories=[/system/lib64, /vendor/lib64, /system/lib64,
/vendor/lib64]]
12-20 12:46:18.944 19662 20611 W System.err:    at
dalvik.system.BaseDexClassLoader.findClass(BaseDexClassLoader.java:74)
12-20 12:46:18.944 19662 20611 W System.err:    at
java.lang.ClassLoader.loadClass(ClassLoader.java:379)
12-20 12:46:18.944 19662 20611 W System.err:    at
java.lang.ClassLoader.loadClass(ClassLoader.java:312)

https://codereview.chromium.org/2593653002/diff/1/base/android/library_loader...
File base/android/library_loader/library_loader_hooks.h (right):

https://codereview.chromium.org/2593653002/diff/1/base/android/library_loader...
base/android/library_loader/library_loader_hooks.h:33: BASE_EXPORT void
SetNativeInitializationHook(
Comment this.

[Tobias Sargeant, 2016-12-20 17:52:58]

On 2016/12/20 17:48:29, Robert Sesek wrote:
> I patched this in locally and am getting browser-side crashes:

Yeah, this clearly fails for at least things like GSA that initialize webview
via CookieManager. is that the source of this crash, or are you seeing it via a
different path? I tested starting the webview shell and chrome, and both worked.
It was only later (when GSA crashed, and in the trybot tests) that I realised
there was still an issue. There's clearly more work to do, but I think this is
the approach that we have to take.

>
https://codereview.chromium.org/2593653002/diff/1/base/android/library_loader...
> File base/android/library_loader/library_loader_hooks.h (right):
> 
>
https://codereview.chromium.org/2593653002/diff/1/base/android/library_loader...
> base/android/library_loader/library_loader_hooks.h:33: BASE_EXPORT void
> SetNativeInitializationHook(
> Comment this.

Will do.

[Robert Sesek, 2016-12-20 17:57:46]

On 2016/12/20 17:52:58, Tobias Sargeant OOO 21dec-6jan wrote:
> On 2016/12/20 17:48:29, Robert Sesek wrote:
> > I patched this in locally and am getting browser-side crashes:
> 
> Yeah, this clearly fails for at least things like GSA that initialize webview
> via CookieManager. is that the source of this crash, or are you seeing it via
a
> different path? I tested starting the webview shell and chrome, and both
worked.
> It was only later (when GSA crashed, and in the trybot tests) that I realised
> there was still an issue. There's clearly more work to do, but I think this is
> the approach that we have to take.

Yes, I was testing through GSA. Using the license activity worked.

[Tobias Sargeant, 2017-01-03 10:58:17]

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

[commit-bot: I haz the power, 2017-01-03 10:58:31]

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

[Tobias Sargeant, 2017-01-03 11:41:03]

On 2016/12/20 17:57:46, Robert Sesek wrote:
> On 2016/12/20 17:52:58, Tobias Sargeant OOO 21dec-6jan wrote:
> > On 2016/12/20 17:48:29, Robert Sesek wrote:
> > > I patched this in locally and am getting browser-side crashes:
> > 
> > Yeah, this clearly fails for at least things like GSA that initialize
webview
> > via CookieManager. is that the source of this crash, or are you seeing it
via
> a
> > different path? I tested starting the webview shell and chrome, and both
> worked.
> > It was only later (when GSA crashed, and in the trybot tests) that I
realised
> > there was still an issue. There's clearly more work to do, but I think this
is
> > the approach that we have to take.
> 
> Yes, I was testing through GSA. Using the license activity worked.

I think (based upon tests passing now) that PS2  is the correct fix.

[commit-bot: I haz the power, 2017-01-03 11:56:32]

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

[commit-bot: I haz the power, 2017-01-03 11:56:33]

Dry run: This issue passed the CQ dry run.

[Torne, 2017-01-03 12:13:50]

https://codereview.chromium.org/2593653002/diff/20001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwCookieManager.java
(right):

https://codereview.chromium.org/2593653002/diff/20001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwCookieManager.java:27:
LibraryLoader.get(LibraryProcessType.PROCESS_WEBVIEW).ensureInitialized();
I'm guessing this is because cookie manager relies on something that was called
during Init before (base/ initialisation?) but I'm curious what..

https://codereview.chromium.org/2593653002/diff/20001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/2593653002/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:368:
// Check that the version of the library we have loaded matches the version we
expect
Why was it necessary to move this? Just curious.

https://codereview.chromium.org/2593653002/diff/20001/base/android/library_lo...
File base/android/library_loader/library_loader_hooks.cc (right):

https://codereview.chromium.org/2593653002/diff/20001/base/android/library_lo...
base/android/library_loader/library_loader_hooks.cc:179: if
(g_registration_callback == NULL) {
g_registration_callback already appears to be the exact mechanism you're adding
here, though the variable name doesn't match the "SetLibraryLoadedHook" function
used to set it. Could we just use this instead (and rename the variable to
g_library_loaded_hook)?

The content layer Init() function already does nothing other than call
SetLibraryLoadedHook. It seems like the current Init function chain is an
unnecessary level of indirection that we could just remove and replace with
this, as a minimum.

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

https://codereview.chromium.org/2593653002/diff/20001/chrome/browser/android/...
chrome/browser/android/chrome_entry_point.cc:21: return
android::OnJNIOnLoadInit(base::Bind(&Init));
This chaining mechanism looks weird and confusing every time I see it, and I
think we should get rid of it.

Instead of passing the chrome layer init function to content, which adds it to a
vector and passes it to base, which then calls them in reverse order, it would
make MUCH more sense if the chrome Init function just called the content init
function directly before it did its own work, and the content init function just
called the base one directly likewise.

The tree of component dependencies isn't dynamic, it's entirely known at
code-writing time :)

[Torne, 2017-01-03 12:16:18]

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

[Torne, 2017-01-03 12:16:18]

Also, you should check out the discussion on
https://codereview.chromium.org/2501193003/ as estevenson is also planning to
touch closely related code..

[Tobias Sargeant, 2017-01-04 00:24:53]

https://codereview.chromium.org/2593653002/diff/20001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwCookieManager.java
(right):

https://codereview.chromium.org/2593653002/diff/20001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwCookieManager.java:27:
LibraryLoader.get(LibraryProcessType.PROCESS_WEBVIEW).ensureInitialized();
On 2017/01/03 12:13:50, Torne wrote:
> I'm guessing this is because cookie manager relies on something that was
called
> during Init before (base/ initialisation?) but I'm curious what..

In comment #8 Robert pasted the exception in question:

java.lang.ClassNotFoundException:
Didn't find class "org.chromium.base.PathUtils" on path: DexPathList[[directory
"."],nativeLibraryDirectories=[/system/lib64, /vendor/lib64, /system/lib64,
/vendor/lib64]]

https://codereview.chromium.org/2593653002/diff/20001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/2593653002/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:368:
// Check that the version of the library we have loaded matches the version we
expect
On 2017/01/03 12:13:50, Torne wrote:
> Why was it necessary to move this? Just curious.

The native native library version is set by the init chain, and so as a result
of delaying that, it wasn't possible to check the version was as expected at the
point that it was originally done.

https://codereview.chromium.org/2593653002/diff/20001/base/android/library_lo...
File base/android/library_loader/library_loader_hooks.cc (right):

https://codereview.chromium.org/2593653002/diff/20001/base/android/library_lo...
base/android/library_loader/library_loader_hooks.cc:179: if
(g_registration_callback == NULL) {
On 2017/01/03 12:13:50, Torne wrote:
> g_registration_callback already appears to be the exact mechanism you're
adding
> here, though the variable name doesn't match the "SetLibraryLoadedHook"
function
> used to set it. Could we just use this instead (and rename the variable to
> g_library_loaded_hook)?
> 
> The content layer Init() function already does nothing other than call
> SetLibraryLoadedHook. It seems like the current Init function chain is an
> unnecessary level of indirection that we could just remove and replace with
> this, as a minimum.

I started doing this, and removing the callback chain means touching a lot of
test code. Would you consider leaving it as a follow-up CL so that we can get
multiprocess fixed with this one?

I also can't coalesce the two hooks because currently the
g_registration_callback is used to run something after all native library
initialization is done, from a point where it can't just do it as the last
thing.

[Torne, 2017-01-05 12:26:05]

OK, so while I'm not thrilled by how this looks, it all should *work* and we
need to get things unbroken, so LGTM for now. I've filed
https://crbug.com/678579 to make a note of these and see if we can do better
once the immediate blocking problems are resolved; I've assigned it to myself
for now.

[Torne, 2017-01-05 12:33:33]

I'll retest this manually on the latest version of android + chromium and then
CQ it, since toby is still OOO.

[Robert Sesek, 2017-01-05 16:07:34]

LGTM. I tested this locally and it works including with GSA.

[Torne, 2017-01-05 16:09:05]

The CQ bit was checked by torne@chromium.org

[Torne, 2017-01-05 16:09:05]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2593653002/#ps20001
(title: "Add library initialization to AwCookieManager constructor")

[commit-bot: I haz the power, 2017-01-05 16:09:22]

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 16:58:10]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1483632545574100,
"parent_rev": "75e9b86472ac55d4a8a41150eeda026d76659fdc", "commit_rev":
"48fdf7dc65a82510505c68c7e03fba8f3a88009b"}

[commit-bot: I haz the power, 2017-01-05 16:58:41]

Description was changed from

==========
Split JNI init so native library preloading dosn't cause native init.

The zygote relies on preloading the shared library. However we can't
determine the process type at this point (for monochrome it could be a
chrome renderer, or a webview renderer), and so current OnJNIOnLoadInit
code that relies on knowing the process type has to be delayed until
LibraryLoader is called, and the process type is known.

To get around this, we split this code in two, and have the first stage
(JNI_OnLoad) register a hook that is run in the second stage, to process
the OnJNIOnLoadInit work.

BUG=
==========

to

==========
Split JNI init so native library preloading dosn't cause native init.

The zygote relies on preloading the shared library. However we can't
determine the process type at this point (for monochrome it could be a
chrome renderer, or a webview renderer), and so current OnJNIOnLoadInit
code that relies on knowing the process type has to be delayed until
LibraryLoader is called, and the process type is known.

To get around this, we split this code in two, and have the first stage
(JNI_OnLoad) register a hook that is run in the second stage, to process
the OnJNIOnLoadInit work.

BUG=

Review-Url: https://codereview.chromium.org/2593653002
Cr-Commit-Position: refs/heads/master@{#441678}
Committed:
https://chromium.googlesource.com/chromium/src/+/48fdf7dc65a82510505c68c7e03f...
==========

[commit-bot: I haz the power, 2017-01-05 16:58:42]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/48fdf7dc65a82510505c68c7e03f...

[agrieve, 2017-01-06 02:31:06]

On 2017/01/05 16:58:42, commit-bot: I haz the power wrote:
> Committed patchset #2 (id:20001) as
>
https://chromium.googlesource.com/chromium/src/+/48fdf7dc65a82510505c68c7e03f...

FYI - this change shrunk monochrome's native library by almost 100kb (but not
Chrome's).
https://chromeperf.appspot.com/report?sid=99caa6c5389f77e13b562afa5703ad8fc1d...

If anyone has insights as to why that might be, I'd be curious to know (I can't
see why this would be from the change)