android: Java-based launcher thread

android: Java-based launcher thread

A major part of android's ChildProcessLauncher is implemented in Java
so having an easy access to the launcher thread is important.

Refactor JavaHandlerThread in base so that that java part can be created
first, and then connected with native peer after consutrction. This is
needed because the launcher thread can be used before native library is
loaded.

Add LauncherThread in content which is a simple wrapper around
JavaHandlerThread. Then for android, override the launcher thread
message loop. Note this means the launcher thread will no longer be
joined on shutdown, but this is not a problem in practice since android
never does clean shutdowns.

Convert two cases of random background threads in ChildProcessLauncher
to use LauncherThread. There are more in the future.

BUG=701442
Review-Url: https://codereview.chromium.org/2774363003
Cr-Commit-Position: refs/heads/master@{#460509}
Committed: https://chromium.googlesource.com/chromium/src/+/61687ec5b7b5dce1c9c86ae46f552c0c34473555

[boliu, 2017-03-28 04:15:15]

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

[commit-bot: I haz the power, 2017-03-28 04:15:34]

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-03-28 04:23:55]

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

[commit-bot: I haz the power, 2017-03-28 04:23:55]

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...)

[boliu, 2017-03-28 05:05:35]

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

[boliu, 2017-03-28 05:05:36]

+agreive for an overall first look.. and also a question about base tests.

So I want to write a test for creating the java side of the thread first, and
then connect it with the native side. Which test would I do? I could put it in
unit tests, but I think it'll be the first unit test to call into java? I also
see base/android/javatests, but afaict they aren't running on bots? And then
there's robolectric tests, but I don't think those can use native code.

[boliu, 2017-03-28 18:00:42]

On 2017/03/28 05:05:36, boliu wrote:
> +agreive for an overall first look.. and also a question about base tests.
> 
> So I want to write a test for creating the java side of the thread first, and
> then connect it with the native side. Which test would I do? I could put it in
> unit tests, but I think it'll be the first unit test to call into java? I also
> see base/android/javatests, but afaict they aren't running on bots? And then
> there's robolectric tests, but I don't think those can use native code.

Oh, base_javatests is built as part of content_shell_test. Gonna use that then.

[agrieve, 2017-03-28 19:59:33]

Just nits for base/ Whatever you decide, lgtm.

https://codereview.chromium.org/2774363003/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/JavaHandlerThread.java (right):

https://codereview.chromium.org/2774363003/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/JavaHandlerThread.java:37: public Looper
getLooper() {
super nit: Might make more sense to expose this by having JavaHandlerThread
extend HandlerThread. Normally I prefer composition, but from the name I think
one would expect JavaHandlerThread instanceof HandlerThread.

https://codereview.chromium.org/2774363003/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/JavaHandlerThread.java:42: if (mStarted)
return;
nit: I think you could do away with mStarted by checking:

mThread.getState() != Thread.State.NEW

https://codereview.chromium.org/2774363003/diff/1/content/browser/browser_mai...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2774363003/diff/1/content/browser/browser_mai...
content/browser/browser_main_loop.cc:1095: message_loop =
android::LauncherThread::GetLauncherMessageLoop();
nit: might be nicer to drop the double "Launcher":
android::LauncherThread::GetMessageLoop()

https://codereview.chromium.org/2774363003/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/LauncherThread.java
(right):

https://codereview.chromium.org/2774363003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/LauncherThread.java:19:
synchronized (sInstanceLock) {
There's no reason to use this class without creating the singleton, so it would
be simpler to just have:

private static LauncherThread sInstance = new LauncherThread()

Going even further, there's are no non-static methods here, so you might as well
just avoid the object altogether and do:

private static final sThread = ...
private static final sHandler = ...
static {
  sThread.maybeStart();
}

[boliu, 2017-03-28 23:30:52]

https://codereview.chromium.org/2774363003/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/JavaHandlerThread.java (right):

https://codereview.chromium.org/2774363003/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/JavaHandlerThread.java:37: public Looper
getLooper() {
On 2017/03/28 19:59:32, agrieve wrote:
> super nit: Might make more sense to expose this by having JavaHandlerThread
> extend HandlerThread. Normally I prefer composition, but from the name I think
> one would expect JavaHandlerThread instanceof HandlerThread.

I think composition is still better, to hide things. Eg I deliberately didn't
expose stop in java, so only native code can explicitly stop it.

I'm sure there are arguments for others :p

https://codereview.chromium.org/2774363003/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/JavaHandlerThread.java:42: if (mStarted)
return;
On 2017/03/28 19:59:32, agrieve wrote:
> nit: I think you could do away with mStarted by checking:
> 
> mThread.getState() != Thread.State.NEW

Done.

https://codereview.chromium.org/2774363003/diff/1/content/browser/browser_mai...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2774363003/diff/1/content/browser/browser_mai...
content/browser/browser_main_loop.cc:1095: message_loop =
android::LauncherThread::GetLauncherMessageLoop();
On 2017/03/28 19:59:32, agrieve wrote:
> nit: might be nicer to drop the double "Launcher":
> android::LauncherThread::GetMessageLoop()

Done.

https://codereview.chromium.org/2774363003/diff/1/content/public/android/java...
File
content/public/android/java/src/org/chromium/content/browser/LauncherThread.java
(right):

https://codereview.chromium.org/2774363003/diff/1/content/public/android/java...
content/public/android/java/src/org/chromium/content/browser/LauncherThread.java:19:
synchronized (sInstanceLock) {
On 2017/03/28 19:59:32, agrieve wrote:
> There's no reason to use this class without creating the singleton, so it
would
> be simpler to just have:
> 
> private static LauncherThread sInstance = new LauncherThread()
> 
> Going even further, there's are no non-static methods here, so you might as
well
> just avoid the object altogether and do:
> 
> private static final sThread = ...
> private static final sHandler = ...
> static {
>   sThread.maybeStart();
> }

Done.

[boliu, 2017-03-28 23:32:32]

boliu@chromium.org changed reviewers:
+ gab@chromium.org, jam@chromium.org

[boliu, 2017-03-28 23:32:33]

PTAL

agrieve: Added tests in base too
+gab for stamping non-android base changes
+jam for non-android parts of content (browser_main_loop,
browser_process_sub_thread)

[boliu, 2017-03-28 23:33:26]

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

[commit-bot: I haz the power, 2017-03-28 23:34:13]

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

[boliu, 2017-03-28 23:36:58]

The CQ bit was unchecked by boliu@chromium.org

[agrieve, 2017-03-29 00:05:56]

base tests lgtm

https://codereview.chromium.org/2774363003/diff/20001/base/test/android/java/...
File base/test/android/java/src/org/chromium/base/JavaHandlerThreadTest.java
(right):

https://codereview.chromium.org/2774363003/diff/20001/base/test/android/java/...
base/test/android/java/src/org/chromium/base/JavaHandlerThreadTest.java:34:
handler.post(runnable);
nit: might be good to add a comment about what this tests (maybe just tests that
the thread is indeed running?)

[boliu, 2017-03-29 01:48:49]

https://codereview.chromium.org/2774363003/diff/20001/base/test/android/java/...
File base/test/android/java/src/org/chromium/base/JavaHandlerThreadTest.java
(right):

https://codereview.chromium.org/2774363003/diff/20001/base/test/android/java/...
base/test/android/java/src/org/chromium/base/JavaHandlerThreadTest.java:34:
handler.post(runnable);
On 2017/03/29 00:05:56, agrieve wrote:
> nit: might be good to add a comment about what this tests (maybe just tests
that
> the thread is indeed running?)

This is meant to test that JavaHandlerThread can be used without issue without
connecting to native peer. And I guess the easiest way to test it is post a task
to it and make sure it executes.

Expanded the comment at the top

[jam, 2017-03-29 14:48:58]

lgtm

[gab, 2017-03-29 17:19:53]

https://codereview.chromium.org/2774363003/diff/40001/base/message_loop/messa...
File base/message_loop/message_loop_unittest.cc (right):

https://codereview.chromium.org/2774363003/diff/40001/base/message_loop/messa...
base/message_loop/message_loop_unittest.cc:102: if (init_java_first) {
I'm not clear on difference? The "JavaHandlerThreadForTesting from
AbortDontRunMoreTasks" changes something?

https://codereview.chromium.org/2774363003/diff/40001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2774363003/diff/40001/content/browser/browser...
content/browser/browser_main_loop.cc:1004: // updated away from its default.
Document |message_loop| here.

https://codereview.chromium.org/2774363003/diff/40001/content/browser/browser...
content/browser/browser_main_loop.cc:1073: redirect_nonUInonIO_browser_threads =
false;
That's a large hammer since this bool is set outside the loop and as such this
alters behaviour of all other threads in this loop, let's tweak logic to only
ignore redirection for PROCESS_LAUNCHER thread?

https://codereview.chromium.org/2774363003/diff/40001/content/browser/browser...
content/browser/browser_main_loop.cc:1132: : new BrowserProcessSubThread(id));
This will alter shutdown behavior of PROCESS_LAUNCHER thread on Android. When
BrowserThreadImpl is passed an external message loop, it no longer joins
remaining tasks on shutdown [1] as Thread::Stop() no-ops (2). This might not be
a problem but something you should be aware of and document in this CL's
description. FWIW, I don't think joining process launch tasks is necessary, but
if the process launching code happens to be using anything registered on
AtExitManager (e.g. non-leaky Singleton/LazyInstances), it's possible you get
use-after-frees if your custom background thread uses those after Chrome's main
thread has unwound (so once again, document that in CL description so it's easy
to come back to this if crashes start popping up -- do we even have a crash
dashboard for Android?)

[1]
https://cs.chromium.org/chromium/src/content/browser/browser_thread_impl.cc?t...

[2]
https://cs.chromium.org/chromium/src/base/threading/thread.cc?type=cs&q=using...

[boliu, 2017-03-29 18:01:57]

Description was changed from

==========
android: Java-based launcher thread

A major part of android's ChildProcessLauncher is implemented in Java
so having an easy access to the launcher thread is important.

Refactor JavaHandlerThread in base so that that java part can be created
first, and then connected with native peer after consutrction. This is
needed because the launcher thread can be used before native library is
loaded.

Add LauncherThread in content which is a simple wrapper around
JavaHandlerThread. Then for android, override the launcher thread
message loop.

Convert two cases of random background threads in ChildProcessLauncher
to use LauncherThread. There are more in the future.

BUG=701442
==========

to

==========
android: Java-based launcher thread

A major part of android's ChildProcessLauncher is implemented in Java
so having an easy access to the launcher thread is important.

Refactor JavaHandlerThread in base so that that java part can be created
first, and then connected with native peer after consutrction. This is
needed because the launcher thread can be used before native library is
loaded.

Add LauncherThread in content which is a simple wrapper around
JavaHandlerThread. Then for android, override the launcher thread
message loop. Note this means the launcher thread will no longer be
joined on shutdown, but this is not a problem in practice since android
never does clean shutdowns.

Convert two cases of random background threads in ChildProcessLauncher
to use LauncherThread. There are more in the future.

BUG=701442
==========

[boliu, 2017-03-29 18:02:40]

gab ptal

sorry had a rebase in between patch sets, but additional diff not too terrible

https://codereview.chromium.org/2774363003/diff/40001/base/message_loop/messa...
File base/message_loop/message_loop_unittest.cc (right):

https://codereview.chromium.org/2774363003/diff/40001/base/message_loop/messa...
base/message_loop/message_loop_unittest.cc:102: if (init_java_first) {
On 2017/03/29 17:19:53, gab wrote:
> I'm not clear on difference? The "JavaHandlerThreadForTesting from
> AbortDontRunMoreTasks" changes something?

It calls a different constructor, which constructs the thread in a different
way, ie create the java object and test it can be used as a first, before
native.

Added Create methods so they can be named better to make this easier to read.
wdyt?

https://codereview.chromium.org/2774363003/diff/40001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2774363003/diff/40001/content/browser/browser...
content/browser/browser_main_loop.cc:1004: // updated away from its default.
On 2017/03/29 17:19:53, gab wrote:
> Document |message_loop| here.

Done.

https://codereview.chromium.org/2774363003/diff/40001/content/browser/browser...
content/browser/browser_main_loop.cc:1073: redirect_nonUInonIO_browser_threads =
false;
On 2017/03/29 17:19:53, gab wrote:
> That's a large hammer since this bool is set outside the loop and as such this
> alters behaviour of all other threads in this loop, let's tweak logic to only
> ignore redirection for PROCESS_LAUNCHER thread?

Oh good catch! Added a bool in the loop instead

https://codereview.chromium.org/2774363003/diff/40001/content/browser/browser...
content/browser/browser_main_loop.cc:1132: : new BrowserProcessSubThread(id));
On 2017/03/29 17:19:53, gab wrote:
> This will alter shutdown behavior of PROCESS_LAUNCHER thread on Android. When
> BrowserThreadImpl is passed an external message loop, it no longer joins
> remaining tasks on shutdown [1] as Thread::Stop() no-ops (2). This might not
be
> a problem but something you should be aware of and document in this CL's
> description. FWIW, I don't think joining process launch tasks is necessary,
but
> if the process launching code happens to be using anything registered on
> AtExitManager (e.g. non-leaky Singleton/LazyInstances), it's possible you get
> use-after-frees if your custom background thread uses those after Chrome's
main
> thread has unwound (so once again, document that in CL description so it's
easy
> to come back to this if crashes start popping up -- do we even have a crash
> dashboard for Android?)
> 
> [1]
>
https://cs.chromium.org/chromium/src/content/browser/browser_thread_impl.cc?t...
> 
> [2]
>
https://cs.chromium.org/chromium/src/base/threading/thread.cc?type=cs&q=using...

Noted. Thankfully, android never does clean shutdown, so don't need to think
about this too much. Added comments though

[gab, 2017-03-29 18:14:04]

lgtm w/ comments

I guess this will make it harder to deprecate PROCESS_LAUNCHER thread in favor
of explicit calls to TaskScheduler but that's something for us to figure out
when we get there...

https://codereview.chromium.org/2774363003/diff/40001/base/message_loop/messa...
File base/message_loop/message_loop_unittest.cc (right):

https://codereview.chromium.org/2774363003/diff/40001/base/message_loop/messa...
base/message_loop/message_loop_unittest.cc:102: if (init_java_first) {
On 2017/03/29 18:02:39, boliu wrote:
> On 2017/03/29 17:19:53, gab wrote:
> > I'm not clear on difference? The "JavaHandlerThreadForTesting from
> > AbortDontRunMoreTasks" changes something?
> 
> It calls a different constructor, which constructs the thread in a different
> way, ie create the java object and test it can be used as a first, before
> native.
> 
> Added Create methods so they can be named better to make this easier to read.
> wdyt?

Ah I see, yeah two constructors doing different things sure was subtle, explicit
create methods are indeed much better,

https://codereview.chromium.org/2774363003/diff/60001/base/test/android/java_...
File base/test/android/java_handler_thread_for_testing.h (right):

https://codereview.chromium.org/2774363003/diff/60001/base/test/android/java_...
base/test/android/java_handler_thread_for_testing.h:44: static
std::unique_ptr<JavaHandlerThreadForTesting> CreateJavaFirst(
Add comments

https://codereview.chromium.org/2774363003/diff/60001/base/test/android/java_...
base/test/android/java_handler_thread_for_testing.h:46:
~JavaHandlerThreadForTesting() override;
Destructor goes first since these are no longer constructors

https://codereview.chromium.org/2774363003/diff/60001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2774363003/diff/60001/content/browser/browser...
content/browser/browser_main_loop.cc:1013: bool
redirect_nonUInonIO_browser_threads =
bool redirect_thread = redirect_nonUInonIO_browser_threads;

(i.e. this individual bool only applies to this thread and should be named as
such -- also removes need to rename top-level bool)

https://codereview.chromium.org/2774363003/diff/60001/content/browser/browser...
content/browser/browser_main_loop.cc:1082: // concerns is not a problem in
practice.
s/is/are/

[boliu, 2017-03-29 18:30:07]

https://codereview.chromium.org/2774363003/diff/60001/base/test/android/java_...
File base/test/android/java_handler_thread_for_testing.h (right):

https://codereview.chromium.org/2774363003/diff/60001/base/test/android/java_...
base/test/android/java_handler_thread_for_testing.h:44: static
std::unique_ptr<JavaHandlerThreadForTesting> CreateJavaFirst(
On 2017/03/29 18:14:04, gab wrote:
> Add comments

Done.

https://codereview.chromium.org/2774363003/diff/60001/base/test/android/java_...
base/test/android/java_handler_thread_for_testing.h:46:
~JavaHandlerThreadForTesting() override;
On 2017/03/29 18:14:04, gab wrote:
> Destructor goes first since these are no longer constructors

Done.

https://codereview.chromium.org/2774363003/diff/60001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2774363003/diff/60001/content/browser/browser...
content/browser/browser_main_loop.cc:1013: bool
redirect_nonUInonIO_browser_threads =
On 2017/03/29 18:14:04, gab wrote:
> bool redirect_thread = redirect_nonUInonIO_browser_threads;
> 
> (i.e. this individual bool only applies to this thread and should be named as
> such -- also removes need to rename top-level bool)

Done, but now needs to rename all the usage of the old var in the switch block
:p

https://codereview.chromium.org/2774363003/diff/60001/content/browser/browser...
content/browser/browser_main_loop.cc:1082: // concerns is not a problem in
practice.
On 2017/03/29 18:14:04, gab wrote:
> s/is/are/

Done.

[boliu, 2017-03-29 18:30:15]

The CQ bit was checked by boliu@chromium.org

[boliu, 2017-03-29 18:30:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
agrieve@chromium.org, jam@chromium.org, gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2774363003/#ps80001
(title: "gab review 2")

[commit-bot: I haz the power, 2017-03-29 18:31:24]

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

[gab, 2017-03-29 18:38:06]

https://codereview.chromium.org/2774363003/diff/60001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2774363003/diff/60001/content/browser/browser...
content/browser/browser_main_loop.cc:1086: if
(redirect_nonUInonIO_browser_threads) {
Actually can't we just change this to

if (redirect_nonUInonIO_browser_threads && !message_loop) {
  ...

instead of having a duplicate of the bool merely to skip this block?

[boliu, 2017-03-29 18:39:59]

The CQ bit was unchecked by boliu@chromium.org

[boliu, 2017-03-29 18:42:20]

https://codereview.chromium.org/2774363003/diff/60001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2774363003/diff/60001/content/browser/browser...
content/browser/browser_main_loop.cc:1086: if
(redirect_nonUInonIO_browser_threads) {
On 2017/03/29 18:38:06, gab wrote:
> Actually can't we just change this to
> 
> if (redirect_nonUInonIO_browser_threads && !message_loop) {
>   ...
> 
> instead of having a duplicate of the bool merely to skip this block?

Hmm, then in theory, that should be changed in all the case blocks, which is no
better than the current thing imo

[gab, 2017-03-29 18:46:53]

https://codereview.chromium.org/2774363003/diff/60001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2774363003/diff/60001/content/browser/browser...
content/browser/browser_main_loop.cc:1086: if
(redirect_nonUInonIO_browser_threads) {
On 2017/03/29 18:42:20, boliu wrote:
> On 2017/03/29 18:38:06, gab wrote:
> > Actually can't we just change this to
> > 
> > if (redirect_nonUInonIO_browser_threads && !message_loop) {
> >   ...
> > 
> > instead of having a duplicate of the bool merely to skip this block?
> 
> Hmm, then in theory, that should be changed in all the case blocks, which is
no
> better than the current thing imo

Hmmm ok sure.

[boliu, 2017-03-29 18:47:25]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2017-03-29 18:48:23]

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-03-29 20:09:38]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490813245811170,
"parent_rev": "477d57e0cb3d510ac379ed3c525741379cd39193", "commit_rev":
"61687ec5b7b5dce1c9c86ae46f552c0c34473555"}

[commit-bot: I haz the power, 2017-03-29 20:10:24]

Description was changed from

==========
android: Java-based launcher thread

A major part of android's ChildProcessLauncher is implemented in Java
so having an easy access to the launcher thread is important.

Refactor JavaHandlerThread in base so that that java part can be created
first, and then connected with native peer after consutrction. This is
needed because the launcher thread can be used before native library is
loaded.

Add LauncherThread in content which is a simple wrapper around
JavaHandlerThread. Then for android, override the launcher thread
message loop. Note this means the launcher thread will no longer be
joined on shutdown, but this is not a problem in practice since android
never does clean shutdowns.

Convert two cases of random background threads in ChildProcessLauncher
to use LauncherThread. There are more in the future.

BUG=701442
==========

to

==========
android: Java-based launcher thread

A major part of android's ChildProcessLauncher is implemented in Java
so having an easy access to the launcher thread is important.

Refactor JavaHandlerThread in base so that that java part can be created
first, and then connected with native peer after consutrction. This is
needed because the launcher thread can be used before native library is
loaded.

Add LauncherThread in content which is a simple wrapper around
JavaHandlerThread. Then for android, override the launcher thread
message loop. Note this means the launcher thread will no longer be
joined on shutdown, but this is not a problem in practice since android
never does clean shutdowns.

Convert two cases of random background threads in ChildProcessLauncher
to use LauncherThread. There are more in the future.

BUG=701442

Review-Url: https://codereview.chromium.org/2774363003
Cr-Commit-Position: refs/heads/master@{#460509}
Committed:
https://chromium.googlesource.com/chromium/src/+/61687ec5b7b5dce1c9c86ae46f55...
==========

[commit-bot: I haz the power, 2017-03-29 20:10:25]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/61687ec5b7b5dce1c9c86ae46f55...