[Android] Allow JNI initialization on background thread

[Android] Allow JNI initialization on background thread

The code used to insist that JNI initialization only happened on
the main (UI) thread. This was because it was believed that
the static initialization of the Java classes initialized as
a result of JNI calls might only work on the UI thread. In
practice it seems that none of these Java classes have this
requirement, so this seems to have been an unnecessary
restriction, and slowed up startup on Chrome for Android. 

Remove this restriction, but add locks to ensure that only one thread is trying to initialize the JNI at a time.

Also change some function names to reflect this change.

BUG=214560
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=189093

[aberent, 2013-03-18 13:38:00]

Downstream patch to actually use this to follow.

[Yaron, 2013-03-18 15:56:46]

Replacing myself with Marcus/joth since I'm pretty sure they wrote this.

From a quick scan of Main downstream, I think we already do load this on a
background thread in |startBackgroundTasks| and then we're just calling
LibraryLoader.ensureInitialized later on the main thread which is logically
equivalent to your locking?

[aberent, 2013-03-18 16:03:15]

The current code does the load on the background thread but sets up the JNI
(except for one function, needed to get initial access to the native code) on
the UI thread. This change is to allow the JNI initialization to also happen on
the background thread.

On 2013/03/18 15:56:46, Yaron wrote:
> Replacing myself with Marcus/joth since I'm pretty sure they wrote this.
> 
> From a quick scan of Main downstream, I think we already do load this on a
> background thread in |startBackgroundTasks| and then we're just calling
> LibraryLoader.ensureInitialized later on the main thread which is logically
> equivalent to your locking?

[bulach, 2013-03-18 16:21:11]

lgtm, thanks! some nits below, but also, and more importantly, the commit
message (and possibly an announcement) should be clarified:
- the original reason for mandating the java initialization on the main thread
was to ensure any static initializers in the java side would happen on that
thread.
this will no longer be valid after this patch (a good thing!) but it needs
clarifying.

feel free to go ahead once joth and the bots are happy :)

https://codereview.chromium.org/12700011/diff/1/content/public/android/java/s...
File content/public/android/java/src/org/chromium/content/app/LibraryLoader.java
(right):

https://codereview.chromium.org/12700011/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/app/LibraryLoader.java:96:
synchronized(sLoadLock) {
nit: space after synchronized

https://codereview.chromium.org/12700011/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/app/LibraryLoader.java:107:
if (!sLoaded) {
nit: unindent this block 107-113

[joth, 2013-03-18 16:32:55]

https://codereview.chromium.org/12700011/diff/1/content/app/android/library_l...
File content/app/android/library_loader_hooks.cc (right):

https://codereview.chromium.org/12700011/diff/1/content/app/android/library_l...
content/app/android/library_loader_hooks.cc:60: logging::InitLogging(NULL,
internally this calls LoggingLock::Init which has the following rider:-

"LoggingLock::Init() should be called from the main thread before any logging"

that may not strictly mean 'the main' thread, more 'a single thread prior to
anything else using logging'. I can't tell from a quick look. Could you check,
and perhaps update that comment? (probably in a separate patch)

https://codereview.chromium.org/12700011/diff/1/content/public/android/java/s...
File content/public/android/java/src/org/chromium/content/app/LibraryLoader.java
(right):

https://codereview.chromium.org/12700011/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/app/LibraryLoader.java:20:
* advantage in doing so.
There was an extra need: for webview we need to do this in two parts: one that's
safe to do in the zygote process (specifically, than runs single threaded and
does not create any threads) and then the second part that is run per-app and
really starts chromium (launching threads etc).
The existing delineation suits this very well.
see AwBrowserProcess.loadLibrary vs. AwBrowserProcess.start


But now I look, I can't see anywhere obvious in LibraryLoadedOnMainThread() that
does create threads. It's always possible that someone adds code in e.g.
logging::Init() that creates a thread though.... ideas if there some way we can
stop this regressing in future? ScopedDisallowCreateThreads? :-)

https://codereview.chromium.org/12700011/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/app/LibraryLoader.java:22:
* must prevent two threads trying to do so simultaneously.
nit: prevent => handle. (prevent makes me look for an assert / exception thrown)

[aberent, 2013-03-18 18:44:54]

Updated patch follows soon.

https://codereview.chromium.org/12700011/diff/1/content/app/android/library_l...
File content/app/android/library_loader_hooks.cc (right):

https://codereview.chromium.org/12700011/diff/1/content/app/android/library_l...
content/app/android/library_loader_hooks.cc:60: logging::InitLogging(NULL,
As I understand the code it does mean 'a single thread prior to anything else
using logging'. The reason is that this creates the pthread mutex used to
control access to the log. This can happen on any thread (pthreads doesn't even
know which thread is the main or UI thread), so long as it is completed before
anything attempts to use the mutex.

I will update this in a separate patch, but I need to check what to say about
Windows. Windows uses a Windows spinlock, and, for all I know, one may have to
create this on the UI thread.
On 2013/03/18 16:32:55, joth wrote:
> internally this calls LoggingLock::Init which has the following rider:-
> 
> "LoggingLock::Init() should be called from the main thread before any logging"
> 
> that may not strictly mean 'the main' thread, more 'a single thread prior to
> anything else using logging'. I can't tell from a quick look. Could you check,
> and perhaps update that comment? (probably in a separate patch)
>

https://codereview.chromium.org/12700011/diff/1/content/public/android/java/s...
File content/public/android/java/src/org/chromium/content/app/LibraryLoader.java
(right):

https://codereview.chromium.org/12700011/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/app/LibraryLoader.java:20:
* advantage in doing so.
I have changed the comment to mention the Webview case. 

I don't think my code change actually makes a difference to the scope for
incorrect thread creation in the first stage of initialization. I think all C++
threads in Chromium are created by PlatformThread class, so in theory one could
create a ScopedDisallowCreateThreads that was checked in PlatformThread.

On 2013/03/18 16:32:55, joth wrote:
> There was an extra need: for webview we need to do this in two parts: one
that's
> safe to do in the zygote process (specifically, than runs single threaded and
> does not create any threads) and then the second part that is run per-app and
> really starts chromium (launching threads etc).
> The existing delineation suits this very well.
> see AwBrowserProcess.loadLibrary vs. AwBrowserProcess.start
> 
> 
> But now I look, I can't see anywhere obvious in LibraryLoadedOnMainThread()
that
> does create threads. It's always possible that someone adds code in e.g.
> logging::Init() that creates a thread though.... ideas if there some way we
can
> stop this regressing in future? ScopedDisallowCreateThreads? :-)

https://codereview.chromium.org/12700011/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/app/LibraryLoader.java:22:
* must prevent two threads trying to do so simultaneously.
On 2013/03/18 16:32:55, joth wrote:
> nit: prevent => handle. (prevent makes me look for an assert / exception
thrown)

I have rewritten this paragraph to make this clearer.

https://codereview.chromium.org/12700011/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/app/LibraryLoader.java:96:
synchronized(sLoadLock) {
On 2013/03/18 16:21:11, bulach wrote:
> nit: space after synchronized

Done.

https://codereview.chromium.org/12700011/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/app/LibraryLoader.java:107:
if (!sLoaded) {
On 2013/03/18 16:21:11, bulach wrote:
> nit: unindent this block 107-113

Done.

[joth, 2013-03-18 19:32:26]

On 18 March 2013 11:44, <aberent@chromium.org> wrote:

> Updated patch follows soon.
>
>
>
> https://codereview.chromium.**org/12700011/diff/1/content/**
>
app/android/library_loader_**hooks.cc<https://codereview.chromium.org/12700011/diff/1/content/app/android/library_loader_hooks.cc>
> File content/app/android/library_**loader_hooks.cc (right):
>
> https://codereview.chromium.**org/12700011/diff/1/content/**
>
app/android/library_loader_**hooks.cc#newcode60<https://codereview.chromium.org/12700011/diff/1/content/app/android/library_loader_hooks.cc#newcode60>
> content/app/android/library_**loader_hooks.cc:60:
> logging::InitLogging(NULL,
> As I understand the code it does mean 'a single thread prior to anything
> else using logging'. The reason is that this creates the pthread mutex
> used to control access to the log. This can happen on any thread
> (pthreads doesn't even know which thread is the main or UI thread), so
> long as it is completed before anything attempts to use the mutex.
>
> I will update this in a separate patch, but I need to check what to say
> about Windows. Windows uses a Windows spinlock, and, for all I know, one
> may have to create this on the UI thread.
>
> On 2013/03/18 16:32:55, joth wrote:
>
>> internally this calls LoggingLock::Init which has the following
>>
> rider:-
>
>  "LoggingLock::Init() should be called from the main thread before any
>>
> logging"
>
>  that may not strictly mean 'the main' thread, more 'a single thread
>>
> prior to
>
>> anything else using logging'. I can't tell from a quick look. Could
>>
> you check,
>
>> and perhaps update that comment? (probably in a separate patch)
>>
>
>
> https://codereview.chromium.**org/12700011/diff/1/content/**
>
public/android/java/src/org/**chromium/content/app/**LibraryLoader.java<https://codereview.chromium.org/12700011/diff/1/content/public/android/java/src/org/chromium/content/app/LibraryLoader.java>
> File
> content/public/android/java/**src/org/chromium/content/app/**
> LibraryLoader.java
> (right):
>
> https://codereview.chromium.**org/12700011/diff/1/content/**
> public/android/java/src/org/**chromium/content/app/**
>
LibraryLoader.java#newcode20<https://codereview.chromium.org/12700011/diff/1/content/public/android/java/src/org/chromium/content/app/LibraryLoader.java#newcode20>
> content/public/android/java/**src/org/chromium/content/app/**
> LibraryLoader.java:20:
> * advantage in doing so.
> I have changed the comment to mention the Webview case.
>
> I don't think my code change actually makes a difference to the scope
> for incorrect thread creation in the first stage of initialization.


Previously the *only* native code called on the background thread was a
very specific bit of android-only code to register one JNI method. (oh, and
creating the AtExit manager. Hmm ok...)
But with this patch we're now initializing several subsystems (logging,
command line, trace event, plus all the breadth of JNI registration methods
that sometimes get other code slipped into them too) so I definitely think
there is an increased scope for accidentally starting threads now.

actually, for OS_ANDROID it would be good to have a global 'thread creation
allowed' flag that defaults to false and we explicitly set to true when
ContenMain.start() is called.




> I
> think all C++ threads in Chromium are created by PlatformThread class,
> so in theory one could create a ScopedDisallowCreateThreads that was
> checked in PlatformThread.
>
>
> On 2013/03/18 16:32:55, joth wrote:
>
>> There was an extra need: for webview we need to do this in two parts:
>>
> one that's
>
>> safe to do in the zygote process (specifically, than runs single
>>
> threaded and
>
>> does not create any threads) and then the second part that is run
>>
> per-app and
>
>> really starts chromium (launching threads etc).
>> The existing delineation suits this very well.
>> see AwBrowserProcess.loadLibrary vs. AwBrowserProcess.start
>>
>
>
>  But now I look, I can't see anywhere obvious in
>>
> LibraryLoadedOnMainThread() that
>
>> does create threads. It's always possible that someone adds code in
>>
> e.g.
>
>> logging::Init() that creates a thread though.... ideas if there some
>>
> way we can
>
>> stop this regressing in future? ScopedDisallowCreateThreads? :-)
>>
>
> https://codereview.chromium.**org/12700011/diff/1/content/**
> public/android/java/src/org/**chromium/content/app/**
>
LibraryLoader.java#newcode22<https://codereview.chromium.org/12700011/diff/1/content/public/android/java/src/org/chromium/content/app/LibraryLoader.java#newcode22>
> content/public/android/java/**src/org/chromium/content/app/**
> LibraryLoader.java:22:
> * must prevent two threads trying to do so simultaneously.
> On 2013/03/18 16:32:55, joth wrote:
>
>> nit: prevent => handle. (prevent makes me look for an assert /
>>
> exception thrown)
>
> I have rewritten this paragraph to make this clearer.
>
>
> https://codereview.chromium.**org/12700011/diff/1/content/**
> public/android/java/src/org/**chromium/content/app/**
>
LibraryLoader.java#newcode96<https://codereview.chromium.org/12700011/diff/1/content/public/android/java/src/org/chromium/content/app/LibraryLoader.java#newcode96>
> content/public/android/java/**src/org/chromium/content/app/**
> LibraryLoader.java:96:
> synchronized(sLoadLock) {
> On 2013/03/18 16:21:11, bulach wrote:
>
>> nit: space after synchronized
>>
>
> Done.
>
>
> https://codereview.chromium.**org/12700011/diff/1/content/**
> public/android/java/src/org/**chromium/content/app/**
>
LibraryLoader.java#newcode107<https://codereview.chromium.org/12700011/diff/1/content/public/android/java/src/org/chromium/content/app/LibraryLoader.java#newcode107>
> content/public/android/java/**src/org/chromium/content/app/**
> LibraryLoader.java:107:
> if (!sLoaded) {
> On 2013/03/18 16:21:11, bulach wrote:
>
>> nit: unindent this block 107-113
>>
>
> Done.
>
>
https://codereview.chromium.**org/12700011/<https://codereview.chromium.org/1...
>

[joth, 2013-03-18 20:58:23]

On 2013/03/18 19:32:26, joth wrote:
> On 18 March 2013 11:44, <mailto:aberent@chromium.org> wrote:
> 
> > Updated patch follows soon.
> >
> >
> >
> > https://codereview.chromium.**org/12700011/diff/1/content/**
> >
>
app/android/library_loader_**hooks.cc<https://codereview.chromium.org/12700011/diff/1/content/app/android/library_loader_hooks.cc>
> > File content/app/android/library_**loader_hooks.cc (right):
> >
> > https://codereview.chromium.**org/12700011/diff/1/content/**
> >
>
app/android/library_loader_**hooks.cc#newcode60<https://codereview.chromium.org/12700011/diff/1/content/app/android/library_loader_hooks.cc#newcode60>
> > content/app/android/library_**loader_hooks.cc:60:
> > logging::InitLogging(NULL,
> > As I understand the code it does mean 'a single thread prior to anything
> > else using logging'. The reason is that this creates the pthread mutex
> > used to control access to the log. This can happen on any thread
> > (pthreads doesn't even know which thread is the main or UI thread), so
> > long as it is completed before anything attempts to use the mutex.
> >
> > I will update this in a separate patch, but I need to check what to say
> > about Windows. Windows uses a Windows spinlock, and, for all I know, one
> > may have to create this on the UI thread.
> >
> > On 2013/03/18 16:32:55, joth wrote:
> >
> >> internally this calls LoggingLock::Init which has the following
> >>
> > rider:-
> >
> >  "LoggingLock::Init() should be called from the main thread before any
> >>
> > logging"
> >
> >  that may not strictly mean 'the main' thread, more 'a single thread
> >>
> > prior to
> >
> >> anything else using logging'. I can't tell from a quick look. Could
> >>
> > you check,
> >
> >> and perhaps update that comment? (probably in a separate patch)
> >>
> >
> >
> > https://codereview.chromium.**org/12700011/diff/1/content/**
> >
>
public/android/java/src/org/**chromium/content/app/**LibraryLoader.java<https://codereview.chromium.org/12700011/diff/1/content/public/android/java/src/org/chromium/content/app/LibraryLoader.java>
> > File
> > content/public/android/java/**src/org/chromium/content/app/**
> > LibraryLoader.java
> > (right):
> >
> > https://codereview.chromium.**org/12700011/diff/1/content/**
> > public/android/java/src/org/**chromium/content/app/**
> >
>
LibraryLoader.java#newcode20<https://codereview.chromium.org/12700011/diff/1/content/public/android/java/src/org/chromium/content/app/LibraryLoader.java#newcode20>
> > content/public/android/java/**src/org/chromium/content/app/**
> > LibraryLoader.java:20:
> > * advantage in doing so.
> > I have changed the comment to mention the Webview case.
> >
> > I don't think my code change actually makes a difference to the scope
> > for incorrect thread creation in the first stage of initialization.
> 
> 
> Previously the *only* native code called on the background thread was a
> very specific bit of android-only code to register one JNI method. (oh, and
> creating the AtExit manager. Hmm ok...)
> But with this patch we're now initializing several subsystems (logging,
> command line, trace event, plus all the breadth of JNI registration methods
> that sometimes get other code slipped into them too) so I definitely think
> there is an increased scope for accidentally starting threads now.
> 
> actually, for OS_ANDROID it would be good to have a global 'thread creation
> allowed' flag that defaults to false and we explicitly set to true when
> ContenMain.start() is called.
> 
> 

Ah OK I understand - you mean you've still left it as 2 separate methods anyway
so not changes the position w.r.t webview zygote.
Agreed! lgtm
I opened b/8414283 to track making further improvements for the webview case.

Thanks

> 
> 
> > I
> > think all C++ threads in Chromium are created by PlatformThread class,
> > so in theory one could create a ScopedDisallowCreateThreads that was
> > checked in PlatformThread.
> >
> >
> > On 2013/03/18 16:32:55, joth wrote:
> >
> >> There was an extra need: for webview we need to do this in two parts:
> >>
> > one that's
> >
> >> safe to do in the zygote process (specifically, than runs single
> >>
> > threaded and
> >
> >> does not create any threads) and then the second part that is run
> >>
> > per-app and
> >
> >> really starts chromium (launching threads etc).
> >> The existing delineation suits this very well.
> >> see AwBrowserProcess.loadLibrary vs. AwBrowserProcess.start
> >>
> >
> >
> >  But now I look, I can't see anywhere obvious in
> >>
> > LibraryLoadedOnMainThread() that
> >
> >> does create threads. It's always possible that someone adds code in
> >>
> > e.g.
> >
> >> logging::Init() that creates a thread though.... ideas if there some
> >>
> > way we can
> >
> >> stop this regressing in future? ScopedDisallowCreateThreads? :-)
> >>
> >
> > https://codereview.chromium.**org/12700011/diff/1/content/**
> > public/android/java/src/org/**chromium/content/app/**
> >
>
LibraryLoader.java#newcode22<https://codereview.chromium.org/12700011/diff/1/content/public/android/java/src/org/chromium/content/app/LibraryLoader.java#newcode22>
> > content/public/android/java/**src/org/chromium/content/app/**
> > LibraryLoader.java:22:
> > * must prevent two threads trying to do so simultaneously.
> > On 2013/03/18 16:32:55, joth wrote:
> >
> >> nit: prevent => handle. (prevent makes me look for an assert /
> >>
> > exception thrown)
> >
> > I have rewritten this paragraph to make this clearer.
> >
> >
> > https://codereview.chromium.**org/12700011/diff/1/content/**
> > public/android/java/src/org/**chromium/content/app/**
> >
>
LibraryLoader.java#newcode96<https://codereview.chromium.org/12700011/diff/1/content/public/android/java/src/org/chromium/content/app/LibraryLoader.java#newcode96>
> > content/public/android/java/**src/org/chromium/content/app/**
> > LibraryLoader.java:96:
> > synchronized(sLoadLock) {
> > On 2013/03/18 16:21:11, bulach wrote:
> >
> >> nit: space after synchronized
> >>
> >
> > Done.
> >
> >
> > https://codereview.chromium.**org/12700011/diff/1/content/**
> > public/android/java/src/org/**chromium/content/app/**
> >
>
LibraryLoader.java#newcode107<https://codereview.chromium.org/12700011/diff/1/content/public/android/java/src/org/chromium/content/app/LibraryLoader.java#newcode107>
> > content/public/android/java/**src/org/chromium/content/app/**
> > LibraryLoader.java:107:
> > if (!sLoaded) {
> > On 2013/03/18 16:21:11, bulach wrote:
> >
> >> nit: unindent this block 107-113
> >>
> >
> > Done.
> >
> >
>
https://codereview.chromium.**org/12700011/%3Chttps://codereview.chromium.org...>
> >

[commit-bot: I haz the power, 2013-03-19 09:35:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aberent@chromium.org/12700011/9001

[commit-bot: I haz the power, 2013-03-19 09:35:29]

Failed to apply patch for
content/public/android/java/src/org/chromium/content/app/SandboxedProcessService.java:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
content/public/android/java/src/org/chromium/content/app/SandboxedProcessService.java
  Hunk #1 FAILED at 129.
  1 out of 1 hunk FAILED -- saving rejects to file
content/public/android/java/src/org/chromium/content/app/SandboxedProcessService.java.rej

Patch:      
content/public/android/java/src/org/chromium/content/app/SandboxedProcessService.java
Index:
content/public/android/java/src/org/chromium/content/app/SandboxedProcessService.java
diff --git
a/content/public/android/java/src/org/chromium/content/app/SandboxedProcessService.java
b/content/public/android/java/src/org/chromium/content/app/SandboxedProcessService.java
index
2c5bd60c47218e1149e2e964f8a5d4e878ee89b5..b55713b974623015fa40daf02ecd967d3e7217c7
100644
---
a/content/public/android/java/src/org/chromium/content/app/SandboxedProcessService.java
+++
b/content/public/android/java/src/org/chromium/content/app/SandboxedProcessService.java
@@ -129,7 +129,7 @@ public class SandboxedProcessService extends Service {
                             mSandboxMainThread.wait();
                         }
                     }
-                    LibraryLoader.initializeOnMainThread(mCommandLineParams);
+                    LibraryLoader.initialize(mCommandLineParams);
                     synchronized (mSandboxMainThread) {
                         mLibraryInitialized = true;
                         mSandboxMainThread.notifyAll();

[commit-bot: I haz the power, 2013-03-19 10:38:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aberent@chromium.org/12700011/15001

[commit-bot: I haz the power, 2013-03-19 10:38:41]

Presubmit check for 12700011-15001 failed and returned exit status 1.

INFO:root:Found 3 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    content/app/android/library_loader_hooks.cc

Presubmit checks took 1.1s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[aberent, 2013-03-19 10:44:44]

Hi Yaron,

Can I have your LGTM, neither joth nor Marcus are owners of
content/apps/android.

On 2013/03/19 10:38:41, I haz the power (commit-bot) wrote:
> Presubmit check for 12700011-15001 failed and returned exit status 1.
> 
> INFO:root:Found 3 file(s).
> 
> Running presubmit commit checks ...
> Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for these files:
>     content/app/android/library_loader_hooks.cc
> 
> Presubmit checks took 1.1s to calculate.
> 
> Was the presubmit check useful? Please send feedback & hate mail to
> maruel@chromium.org!

[Yaron, 2013-03-19 16:53:01]

lgtm

[commit-bot: I haz the power, 2013-03-19 16:56:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aberent@chromium.org/12700011/15001

[commit-bot: I haz the power, 2013-03-19 20:43:58]

Change committed as 189093