JNI_OnLoad and related initialisation is confusing |
|||
Issue descriptionDuring JNI_OnLoad refactoring in https://codereview.chromium.org/2593653002/ to fix some issues with library initialisation in monochrome multiprocess, we've noticed several weird/unfortunate things about how JNI library initialisation is done that should be cleaned up: 1) It's not clear why CookieManager initialisation fails in the exact way that was observed in the earlier version of the CL. CookieManager init is a weird special case and we should maybe try to figure out what's different here and see if we can make this look less strange. 2) Comparing the native library version had to be moved to later in init, which might not be ideal if there's actually a mismatch as we might go wrong before then. The version should be a constant so it should really be possible to check it without any dependency on other initialisation. 3) The callback chain of Init functions is weird and seems backwards; we should instead just have a regular forward chain of functions to do initialisation, but this will require fixing a bunch of test code. 4) The newly added hook for init effectively duplicates the existing LibraryLoadedHook, but is currently required to sequence initialisation correctly. We should probably be able to refactor this so that init happens in the right order with just one phase. See the CL discussion for more info about these.
,
Jan 6 2017
I'm not sure I understand what you mean Michael; isn't that already the exact thing that your JNI init chain does anyway?
,
Jan 6 2017
The difference is subtle, it seemed he is ok with passing callback in a function's parameter and calling it in that function, like base::android::OnJNIOnLoadInit does, I knew we also have base::android::SetLibraryLoadedHook() which is same as SetNativeInitializationHook, IIRC, SetLibraryLoadedHook has existed at that time? I can't find what he exactly said, it might be oral discussion, what I can remember is I provided similar solution at that time, but he wanted me to do the current way. Also, it is probably fine to have such callback now.
,
Jan 9 2017
Well, my proposal is that we don't call from base up to the higher levels here at all, so that seems like it removes any layering objection entirely. My suggestion is that we simply call *downwards* in a completely static (not callback-based) way.
,
Jan 30 2017
tobiasjs@ is working on this.
,
Feb 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b9e287e5af645fe02056c99c36b6ffc40925ff07 commit b9e287e5af645fe02056c99c36b6ffc40925ff07 Author: tobiasjs <tobiasjs@chromium.org> Date: Wed Feb 08 11:09:50 2017 Remove JNI onload callback vector construction in favour of direct calls. As part of cleaning up JNI onload and initialization, this CL removes the current code that builds up a vector of callbacks, which are eventually run by base::android::OnJNIOnLoad{RegisterJNI,Init} in favour of calling the appropriate code directly from those functions (and the functions that call them). BUG= 678579 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2620303004 Cr-Commit-Position: refs/heads/master@{#448955} [modify] https://crrev.com/b9e287e5af645fe02056c99c36b6ffc40925ff07/android_webview/lib/main/webview_jni_onload.cc [modify] https://crrev.com/b9e287e5af645fe02056c99c36b6ffc40925ff07/android_webview/lib/main/webview_jni_onload.h [modify] https://crrev.com/b9e287e5af645fe02056c99c36b6ffc40925ff07/base/android/base_jni_onload.cc [modify] https://crrev.com/b9e287e5af645fe02056c99c36b6ffc40925ff07/base/android/base_jni_onload.h [modify] https://crrev.com/b9e287e5af645fe02056c99c36b6ffc40925ff07/chrome/app/android/chrome_jni_onload.cc [modify] https://crrev.com/b9e287e5af645fe02056c99c36b6ffc40925ff07/chrome/app/android/chrome_jni_onload.h [modify] https://crrev.com/b9e287e5af645fe02056c99c36b6ffc40925ff07/chrome/browser/android/chrome_entry_point.cc [modify] https://crrev.com/b9e287e5af645fe02056c99c36b6ffc40925ff07/chrome/browser/android/monochrome_entry_point.cc [modify] https://crrev.com/b9e287e5af645fe02056c99c36b6ffc40925ff07/chromecast/app/android/cast_jni_loader.cc [modify] https://crrev.com/b9e287e5af645fe02056c99c36b6ffc40925ff07/components/cronet/android/cronet_library_loader.cc [modify] https://crrev.com/b9e287e5af645fe02056c99c36b6ffc40925ff07/components/cronet/android/test/cronet_test_jni.cc [modify] https://crrev.com/b9e287e5af645fe02056c99c36b6ffc40925ff07/components/test/android/browsertests_apk/components_browser_tests_jni_onload.cc [modify] https://crrev.com/b9e287e5af645fe02056c99c36b6ffc40925ff07/content/app/android/content_jni_onload.cc [modify] https://crrev.com/b9e287e5af645fe02056c99c36b6ffc40925ff07/content/public/app/content_jni_onload.h [modify] https://crrev.com/b9e287e5af645fe02056c99c36b6ffc40925ff07/content/shell/android/browsertests_apk/content_browser_tests_jni_onload.cc [modify] https://crrev.com/b9e287e5af645fe02056c99c36b6ffc40925ff07/content/shell/android/linker_test_apk/chromium_linker_test_android.cc [modify] https://crrev.com/b9e287e5af645fe02056c99c36b6ffc40925ff07/content/shell/android/shell_library_loader.cc [modify] https://crrev.com/b9e287e5af645fe02056c99c36b6ffc40925ff07/mojo/android/javatests/init_library.cc [modify] https://crrev.com/b9e287e5af645fe02056c99c36b6ffc40925ff07/net/test/android/net_test_entry_point.cc [modify] https://crrev.com/b9e287e5af645fe02056c99c36b6ffc40925ff07/net/test/android/net_test_jni_onload.cc [modify] https://crrev.com/b9e287e5af645fe02056c99c36b6ffc40925ff07/net/test/android/net_test_jni_onload.h [modify] https://crrev.com/b9e287e5af645fe02056c99c36b6ffc40925ff07/remoting/client/jni/remoting_jni_onload.cc [modify] https://crrev.com/b9e287e5af645fe02056c99c36b6ffc40925ff07/testing/android/native_test/native_test_jni_onload.cc
,
Oct 1
Marking as fixed, but: Torne, do you think there's more to do here?
,
Oct 1
I'd still like monochrome's entry point to be untangled, but that's covered in issue 674937 so I think that's fine. |
|||
►
Sign in to add a comment |
|||
Comment 1 by michaelbai@chromium.org
, Jan 5 2017