Move PNaCl process startup back to the main thread.

Move PNaCl process startup back to the main thread.

Previously it was split between the helper thread and
the main thread. Way back in the day it was only the
helper thread, but there was concern about thread-safety
and part of the process moved to the main thread,
straddling two threads. Now we move all the way back to
the main thread only.

This removes the need to have "WaitFor*Start" in the helper
thread. Otherwise the helper thread would post work to
the main thread, and wait. That makes it very risky w.r.t.
deadlock, since the main thread will join() the helper
thread as part of plugin teardown. We had a bunch of
workarounds like a timeout when waiting on the condvar.

The bad thing about main thread loads is that it blocks
the renderer from doing work (like animation) as the nexe
loads. However, since we bounce work to the main thread
anyway, we block right now without this CL already.
In a later CL we can take advantage of the sel_main_chrome
load status hook to know when the nexe load is about done,
and only then call-back into the plugin when the blocking
is minimized. Hook added here:
https://codereview.chromium.org/1090233003/diff/100001/components/nacl/common/nacl_renderer_messages.h

SRPC client connection is still done on the helper thread.
I'm not sure of the requirement, but it will block
indefinitely if we do that in the main thread so
I left that in the helper thread.

There's still a bunch of pnacl_translate_thread mutexes
and condvars and the *_subprocess_active flags left
in the pnacl_translate_thread. Haven't moved the
activeness tracking out so that's left as is.

BUG=473474
Committed: https://crrev.com/a26ccdf90ca5f7464df921c72d18ec52d5e27d09
Cr-Commit-Position: refs/heads/master@{#329409}

[jvoung (off chromium), 2015-05-07 00:55:55]

jvoung@chromium.org changed reviewers:
+ dschuff@chromium.org

[jvoung (off chromium), 2015-05-07 00:56:38]

Still a bunch of cleanup to do, but wdyt about moving this to the main thread?

[Derek Schuff, 2015-05-07 19:51:26]

On 2015/05/07 00:56:38, jvoung wrote:
> Still a bunch of cleanup to do, but wdyt about moving this to the main thread?

yeah it looks like we are blocking the main thread anyway for the helper modules
at least. does that include the main nexe too?

[Derek Schuff, 2015-05-07 19:52:12]

On 2015/05/07 19:51:26, Derek Schuff wrote:
> On 2015/05/07 00:56:38, jvoung wrote:
> > Still a bunch of cleanup to do, but wdyt about moving this to the main
thread?
> 
> yeah it looks like we are blocking the main thread anyway for the helper
modules
> at least. does that include the main nexe too?

but in general if it doesn't make things any worse, and it simplifies the code,
and we have a way forward that we actually like, then i'm all for it.

[jvoung (off chromium), 2015-05-07 20:09:39]

On 2015/05/07 19:51:26, Derek Schuff wrote:
> On 2015/05/07 00:56:38, jvoung wrote:
> > Still a bunch of cleanup to do, but wdyt about moving this to the main
thread?
> 
> yeah it looks like we are blocking the main thread anyway for the helper
modules
> at least. does that include the main nexe too?

Yeah I think the main nexe has always been doing StartNexe() from the main
thread so would have blocked too. I'm hoping the sel_main_chrome hook ->
renderer callback will help minimize blocking for both helpers and main nexes
later on.

[jvoung (off chromium), 2015-05-07 22:52:26]

ok did a bit of cleanup -- PTAL

[Derek Schuff, 2015-05-08 22:45:59]

https://codereview.chromium.org/1128943003/diff/60001/components/nacl/rendere...
File components/nacl/renderer/plugin/plugin.cc (right):

https://codereview.chromium.org/1128943003/diff/60001/components/nacl/rendere...
components/nacl/renderer/plugin/plugin.cc:117: // TODO(jvoung): This operations
blocks. That's bad because this is the
this operation, or these operations

https://codereview.chromium.org/1128943003/diff/60001/components/nacl/rendere...
File components/nacl/renderer/plugin/pnacl_coordinator.cc (right):

https://codereview.chromium.org/1128943003/diff/60001/components/nacl/rendere...
components/nacl/renderer/plugin/pnacl_coordinator.cc:417: void
PnaclCoordinator::LoadCompiler(int32_t pp_error) {
is LoadCompiler ever used as a callback? does it actually need a pp_error now?

https://codereview.chromium.org/1128943003/diff/60001/components/nacl/rendere...
File components/nacl/renderer/plugin/pnacl_translate_thread.cc (right):

https://codereview.chromium.org/1128943003/diff/60001/components/nacl/rendere...
components/nacl/renderer/plugin/pnacl_translate_thread.cc:116: // Invoke llc
followed by ld off the main thread.  This allows use of
move or remove this comment.

https://codereview.chromium.org/1128943003/diff/60001/components/nacl/rendere...
components/nacl/renderer/plugin/pnacl_translate_thread.cc:150: // Tear down the
previous thread.
why tear down and fork again instead of reusing the thread?

https://codereview.chromium.org/1128943003/diff/60001/components/nacl/rendere...
components/nacl/renderer/plugin/pnacl_translate_thread.cc:195: if
(!compiler_subprocess_active_)
is it intentional to return with the mutex still locked?

https://codereview.chromium.org/1128943003/diff/60001/components/nacl/rendere...
components/nacl/renderer/plugin/pnacl_translate_thread.cc:199: // Wait till we
are in the helper thread to do the blocking
maybe say "now that we are in the helper thread, we can do blah blah"

https://codereview.chromium.org/1128943003/diff/60001/components/nacl/rendere...
components/nacl/renderer/plugin/pnacl_translate_thread.cc:346: // Wait till we
are in the helper thread to do the blocking
here too as above

https://codereview.chromium.org/1128943003/diff/60001/components/nacl/rendere...
File components/nacl/renderer/plugin/pnacl_translate_thread.h (right):

https://codereview.chromium.org/1128943003/diff/60001/components/nacl/rendere...
components/nacl/renderer/plugin/pnacl_translate_thread.h:113: // ensure the
subprocesses don't get shutdown more than once.
the comment should have a little more detail about e.g. if (some of) these
variables be touched by a particular thread or with the lock

[jvoung (off chromium), 2015-05-08 23:59:32]

https://codereview.chromium.org/1128943003/diff/60001/components/nacl/rendere...
File components/nacl/renderer/plugin/plugin.cc (right):

https://codereview.chromium.org/1128943003/diff/60001/components/nacl/rendere...
components/nacl/renderer/plugin/plugin.cc:117: // TODO(jvoung): This operations
blocks. That's bad because this is the
On 2015/05/08 22:45:59, Derek Schuff wrote:
> this operation, or these operations

Done.

https://codereview.chromium.org/1128943003/diff/60001/components/nacl/rendere...
File components/nacl/renderer/plugin/pnacl_coordinator.cc (right):

https://codereview.chromium.org/1128943003/diff/60001/components/nacl/rendere...
components/nacl/renderer/plugin/pnacl_coordinator.cc:417: void
PnaclCoordinator::LoadCompiler(int32_t pp_error) {
On 2015/05/08 22:45:59, Derek Schuff wrote:
> is LoadCompiler ever used as a callback? does it actually need a pp_error now?

No -- and I added the check of temp_nexe_file_->Open()'s return value earlier.
Removed the pp_error param.

https://codereview.chromium.org/1128943003/diff/60001/components/nacl/rendere...
File components/nacl/renderer/plugin/pnacl_translate_thread.cc (right):

https://codereview.chromium.org/1128943003/diff/60001/components/nacl/rendere...
components/nacl/renderer/plugin/pnacl_translate_thread.cc:116: // Invoke llc
followed by ld off the main thread.  This allows use of
On 2015/05/08 22:45:59, Derek Schuff wrote:
> move or remove this comment.

Done.

https://codereview.chromium.org/1128943003/diff/60001/components/nacl/rendere...
components/nacl/renderer/plugin/pnacl_translate_thread.cc:150: // Tear down the
previous thread.
On 2015/05/08 22:45:59, Derek Schuff wrote:
> why tear down and fork again instead of reusing the thread?

Ideally we would have a "PostTask" e.g., if we use base/threading. We can't use
base/threading because of conflicts on macro definitions (CHECK, and logging
macros, etc.).

Otherwise, this seemed easier to reason about than having the thread
CallOnMainThread, sleep on a condvar and wait till it can start linking (or
until it's asked to abort).

Preference?

https://codereview.chromium.org/1128943003/diff/60001/components/nacl/rendere...
components/nacl/renderer/plugin/pnacl_translate_thread.cc:195: if
(!compiler_subprocess_active_)
On 2015/05/08 22:45:59, Derek Schuff wrote:
> is it intentional to return with the mutex still locked?

Nope =)

Done.

https://codereview.chromium.org/1128943003/diff/60001/components/nacl/rendere...
components/nacl/renderer/plugin/pnacl_translate_thread.cc:199: // Wait till we
are in the helper thread to do the blocking
On 2015/05/08 22:45:59, Derek Schuff wrote:
> maybe say "now that we are in the helper thread, we can do blah blah"

Done.

https://codereview.chromium.org/1128943003/diff/60001/components/nacl/rendere...
components/nacl/renderer/plugin/pnacl_translate_thread.cc:346: // Wait till we
are in the helper thread to do the blocking
On 2015/05/08 22:45:59, Derek Schuff wrote:
> here too as above

Done.

https://codereview.chromium.org/1128943003/diff/60001/components/nacl/rendere...
File components/nacl/renderer/plugin/pnacl_translate_thread.h (right):

https://codereview.chromium.org/1128943003/diff/60001/components/nacl/rendere...
components/nacl/renderer/plugin/pnacl_translate_thread.h:113: // ensure the
subprocesses don't get shutdown more than once.
On 2015/05/08 22:45:59, Derek Schuff wrote:
> the comment should have a little more detail about e.g. if (some of) these
> variables be touched by a particular thread or with the lock

Done. Still a little iffy on the accesses that happen without the lock, but
those were there before...

[Derek Schuff, 2015-05-09 00:18:32]

lgtm

https://codereview.chromium.org/1128943003/diff/60001/components/nacl/rendere...
File components/nacl/renderer/plugin/pnacl_translate_thread.cc (right):

https://codereview.chromium.org/1128943003/diff/60001/components/nacl/rendere...
components/nacl/renderer/plugin/pnacl_translate_thread.cc:150: // Tear down the
previous thread.
This does seem simpler, and probably not too bad for performance compared to the
rest of the stuff that's going on.

[jvoung (off chromium), 2015-05-09 04:30:53]

The CQ bit was checked by jvoung@chromium.org

[commit-bot: I haz the power, 2015-05-09 04:32:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1128943003/80001

[commit-bot: I haz the power, 2015-05-09 12:15:39]

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

[commit-bot: I haz the power, 2015-05-09 12:15:40]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  cast_shell_android on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[jvoung (off chromium), 2015-05-12 00:13:10]

FYI, rebased after ananta's change landed first:
https://codereview.chromium.org/1137833003/

This CL also had removed the CloseFileHandle when LoadHelperNaClModule failed,
but that seemed like it could go in as a separate change and could tag the fix
for the separate bug.

[jvoung (off chromium), 2015-05-12 15:15:20]

The CQ bit was checked by jvoung@chromium.org

[jvoung (off chromium), 2015-05-12 15:15:20]

The patchset sent to the CQ was uploaded after l-g-t-m from dschuff@chromium.org
Link to the patchset: https://codereview.chromium.org/1128943003/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2015-05-12 15:15:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1128943003/100001

[commit-bot: I haz the power, 2015-05-12 15:19:40]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-05-12 15:20:20]

Patchset 6 (id:??) landed as
https://crrev.com/a26ccdf90ca5f7464df921c72d18ec52d5e27d09
Cr-Commit-Position: refs/heads/master@{#329409}