android: Fix java launcher thread

android: Fix java launcher thread

This is a follow up fix for refs/heads/master@{#460509} which is not
correct:
* content::android::LauncherThread never starts the thread, so
  GetMessageLoop just returns null.
* This of course causes an actual native thread to be started for
  launcher thread in  BrowserMainLoop::CreateThreads.

Fix does this:
* content::android::LauncherThread calls Start on construction so
  GetMessageLoop no longer returns null.
* Make sure Start is not called on BrowserProcessSubThread for launcher
  thread. This matches behavior in BrowserMainLoop::InitializeMainThread
  for the UI thread.

Luckily we have not removed any thread safety code in this time, so
looks like this mistake has not caused huge issues.

BUG=701442
Review-Url: https://codereview.chromium.org/2808343004
Cr-Commit-Position: refs/heads/master@{#464466}
Committed: https://chromium.googlesource.com/chromium/src/+/311458dc726c28f8d091aa496ca6ce70778269ef

[boliu, 2017-04-12 18:30:52]

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

[commit-bot: I haz the power, 2017-04-12 18:31:35]

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

[boliu, 2017-04-12 18:31:41]

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

[boliu, 2017-04-12 18:31:42]

PTAL

Found this when I was adding thread asserts and realized native calls doesn't
assert on the java thread: https://codereview.chromium.org/2809293005/

oops

[gab, 2017-04-12 19:29:06]

Re. * Make sure Start is not called on BrowserProcessSubThread for launcher
  thread. This matches behavior in BrowserMainLoop::InitializeMainThread
  for the UI thread.

How's that ensured by this CL?

[commit-bot: I haz the power, 2017-04-12 19:29:27]

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

[commit-bot: I haz the power, 2017-04-12 19:29:28]

Dry run: This issue passed the CQ dry run.

[boliu, 2017-04-12 19:43:43]

On 2017/04/12 19:29:06, gab wrote:
> Re. * Make sure Start is not called on BrowserProcessSubThread for launcher
>   thread. This matches behavior in BrowserMainLoop::InitializeMainThread
>   for the UI thread.
> 
> How's that ensured by this CL?

The change in BrowserMainLoop to not call StartWithOptions.

Without that change, there's a DCHECK in base thread.cc that would have caught
double start:
'[10797:10797:0412/180323.291675:97244901791:FATAL:thread.cc(80)] Check failed:
!message_loop_.

because message loop is already set.

[boliu, 2017-04-12 19:53:27]

On 2017/04/12 19:43:43, boliu wrote:
> On 2017/04/12 19:29:06, gab wrote:
> > Re. * Make sure Start is not called on BrowserProcessSubThread for launcher
> >   thread. This matches behavior in BrowserMainLoop::InitializeMainThread
> >   for the UI thread.
> > 
> > How's that ensured by this CL?
> 
> The change in BrowserMainLoop to not call StartWithOptions.
> 
> Without that change, there's a DCHECK in base thread.cc that would have caught
> double start:
> '[10797:10797:0412/180323.291675:97244901791:FATAL:thread.cc(80)] Check
failed:
> !message_loop_.
> 
> because message loop is already set.

Already set by BrowserThreadImpl:
https://cs.chromium.org/chromium/src/content/browser/browser_thread_impl.cc?r...

[boliu, 2017-04-12 22:30:44]

Description was changed from

==========
android: Fix java launcher thread

This is a follow up fix for refs/heads/master@{#460509} which is not
correct:
* content::android::LauncherThread never starts the thread, so
  GetMessageLoop just returns null.
* This of course causes an actual native thread to be started for
  launcher thread in  BrowserMainLoop::CreateThreads.

Fix does this:
* content::android::LauncherThread calls Start on construction so
  GetMessageLoop no longer returns null.
* Make sure Start is not called on BrowserProcessSubThread for launcher
  thread. This matches behavior in BrowserMainLoop::InitializeMainThread
  for the UI thread.

Luckily we have not removed any thread safety code in this time, so
looks like this mistake has not caused huge issues.

BUG=689758
==========

to

==========
android: Fix java launcher thread

This is a follow up fix for refs/heads/master@{#460509} which is not
correct:
* content::android::LauncherThread never starts the thread, so
  GetMessageLoop just returns null.
* This of course causes an actual native thread to be started for
  launcher thread in  BrowserMainLoop::CreateThreads.

Fix does this:
* content::android::LauncherThread calls Start on construction so
  GetMessageLoop no longer returns null.
* Make sure Start is not called on BrowserProcessSubThread for launcher
  thread. This matches behavior in BrowserMainLoop::InitializeMainThread
  for the UI thread.

Luckily we have not removed any thread safety code in this time, so
looks like this mistake has not caused huge issues.

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

[gab, 2017-04-13 15:39:12]

lgtm w/ comment

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

https://codereview.chromium.org/2808343004/diff/1/content/browser/browser_mai...
content/browser/browser_main_loop.cc:1130: if (!message_loop &&
!(*thread_to_start)->StartWithOptions(options))
Ah I see, hadn't parsed that this wasn't about adding a conditional for the LOG
but rather preventing StartWithOptions(), add a comment above like:

// Start the thread if an existing |message_loop| wasn't provided.

[gab, 2017-04-13 15:40:04]

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

https://codereview.chromium.org/2808343004/diff/1/content/browser/browser_mai...
content/browser/browser_main_loop.cc:1071: message_loop =
android::LauncherThread::GetMessageLoop();
DCHECK(message_loop); here maybe?

[boliu, 2017-04-13 16:30:05]

thanks!

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

https://codereview.chromium.org/2808343004/diff/1/content/browser/browser_mai...
content/browser/browser_main_loop.cc:1071: message_loop =
android::LauncherThread::GetMessageLoop();
On 2017/04/13 15:40:04, gab wrote:
> DCHECK(message_loop); here maybe?

Done.

https://codereview.chromium.org/2808343004/diff/1/content/browser/browser_mai...
content/browser/browser_main_loop.cc:1130: if (!message_loop &&
!(*thread_to_start)->StartWithOptions(options))
On 2017/04/13 15:39:12, gab wrote:
> Ah I see, hadn't parsed that this wasn't about adding a conditional for the
LOG
> but rather preventing StartWithOptions(), add a comment above like:
> 
> // Start the thread if an existing |message_loop| wasn't provided.

Done.

[boliu, 2017-04-13 16:30:26]

The CQ bit was checked by boliu@chromium.org

[boliu, 2017-04-13 16:30:26]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2808343004/#ps20001
(title: "review comments")

[commit-bot: I haz the power, 2017-04-13 16:31:00]

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-04-13 18:01:42]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492101026314450,
"parent_rev": "049f4387ba7724fa3c5ac1117a438fabb504f563", "commit_rev":
"311458dc726c28f8d091aa496ca6ce70778269ef"}

[commit-bot: I haz the power, 2017-04-13 18:11:27]

Description was changed from

==========
android: Fix java launcher thread

This is a follow up fix for refs/heads/master@{#460509} which is not
correct:
* content::android::LauncherThread never starts the thread, so
  GetMessageLoop just returns null.
* This of course causes an actual native thread to be started for
  launcher thread in  BrowserMainLoop::CreateThreads.

Fix does this:
* content::android::LauncherThread calls Start on construction so
  GetMessageLoop no longer returns null.
* Make sure Start is not called on BrowserProcessSubThread for launcher
  thread. This matches behavior in BrowserMainLoop::InitializeMainThread
  for the UI thread.

Luckily we have not removed any thread safety code in this time, so
looks like this mistake has not caused huge issues.

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

to

==========
android: Fix java launcher thread

This is a follow up fix for refs/heads/master@{#460509} which is not
correct:
* content::android::LauncherThread never starts the thread, so
  GetMessageLoop just returns null.
* This of course causes an actual native thread to be started for
  launcher thread in  BrowserMainLoop::CreateThreads.

Fix does this:
* content::android::LauncherThread calls Start on construction so
  GetMessageLoop no longer returns null.
* Make sure Start is not called on BrowserProcessSubThread for launcher
  thread. This matches behavior in BrowserMainLoop::InitializeMainThread
  for the UI thread.

Luckily we have not removed any thread safety code in this time, so
looks like this mistake has not caused huge issues.

BUG=701442

Review-Url: https://codereview.chromium.org/2808343004
Cr-Commit-Position: refs/heads/master@{#464466}
Committed:
https://chromium.googlesource.com/chromium/src/+/311458dc726c28f8d091aa496ca6...
==========

[commit-bot: I haz the power, 2017-04-13 18:11:28]

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