Have each SandboxedProcessLauncherDelegate maintain a zygote.

Have each SandboxedProcessLauncherDelegate maintain a zygote.

To improve component updates of PPAPI plugins, Chrome needs multiple zygotes.
This will allow the PPAPI zygote to be recreated when a plugin is updated.
This CL allows Chrome to maintain a zygote for each process type by having
each SandboxedProcessLauncherDelegate maintain a class which can communicate
with its respective zygote. This CL will be followed up with work to allow
customization of zygotes, which will give Chrome the improve component update
experience.

BUG=569191
Committed: https://crrev.com/3c1e16b490255119b6f70f94d1716645e897b185
Cr-Commit-Position: refs/heads/master@{#370488}

Committed: https://crrev.com/afd49a83be5da2df6667a2afd14239d7783aff52
Cr-Commit-Position: refs/heads/master@{#371042}

[Greg K, 2015-12-21 21:18:49]

Description was changed from

==========
Have each SandboxedProcessLauncherDelegate maintain a zygote.

To improve component updates of PPAPI plugins, Chrome needs multiple zygotes.
This will allow the PPAPI zygote to be recreated when a plugin is updated.
This CL allows Chrome to maintain a zygote for each process type by having
each SandboxedProcessLauncherDelegate maintain a class which can communicate
with its respective zygote. This CL will be followed up with work to allow
customization of zygotes, which will give Chrome the improve component update
experience.

BUG=569191
==========

to

==========
Have each SandboxedProcessLauncherDelegate maintain a zygote.

To improve component updates of PPAPI plugins, Chrome needs multiple zygotes.
This will allow the PPAPI zygote to be recreated when a plugin is updated.
This CL allows Chrome to maintain a zygote for each process type by having
each SandboxedProcessLauncherDelegate maintain a class which can communicate
with its respective zygote. This CL will be followed up with work to allow
customization of zygotes, which will give Chrome the improve component update
experience.

BUG=569191
==========

[Greg K, 2015-12-21 21:18:49]

kerrnel@chromium.org changed reviewers:
+ mdempsky@chromium.org

[Greg K, 2015-12-21 21:19:08]

Matt, we can start looking at this first CL to move to a world with multiple
zygotes.

https://codereview.chromium.org/1532423003/diff/100001/content/browser/zygote...
File content/browser/zygote_host/zygote_communication_linux.cc (right):

https://codereview.chromium.org/1532423003/diff/100001/content/browser/zygote...
content/browser/zygote_host/zygote_communication_linux.cc:91: if (bytes_read !=
sizeof(sandbox_status_)) {
drop braces

https://codereview.chromium.org/1532423003/diff/100001/content/browser/zygote...
content/browser/zygote_host/zygote_communication_linux.cc:100: if
(ReadSandboxStatus() == -1)
add braces

https://codereview.chromium.org/1532423003/diff/100001/content/browser/zygote...
content/browser/zygote_host/zygote_communication_linux.cc:448: if
(have_read_sandbox_status_word_) {
drop braces

[mdempsky, 2015-12-22 21:21:33]

(I focused mostly on the code outside of zygote_host so far.)

https://codereview.chromium.org/1532423003/diff/100001/content/browser/browse...
File content/browser/browser_main_loop.cc (left):

https://codereview.chromium.org/1532423003/diff/100001/content/browser/browse...
content/browser/browser_main_loop.cc:1038:
ZygoteHostImpl::GetInstance()->TearDownAfterLastChild();
You might try landing just this removal first/separately.  I think we've
established that it's okay to remove and the trybots seem happy with it, but I
can imagine some weird CrOS or non-trybot tests might start complaining.  (And
if so, better to just have to revert/re-revert a simple one-line CL than your
entire CL.)

https://codereview.chromium.org/1532423003/diff/100001/content/browser/child_...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/1532423003/diff/100001/content/browser/child_...
content/browser/child_process_launcher.cc:203: zygote = nullptr;
Hm, so if you use

    using ZygoteHandle = std::nullptr_t;

as the stub typedef, then you could just write:

    ZygoteHandle zygote = nullptr;

here and use "nullptr" instead of "ZygoteHandle()" in
OnChildProcessStartedAndroid.  That seems a bit cleaner to me, what do you
think?

(Sorry, I know I suggested the empty struct in the first place. :-))

https://codereview.chromium.org/1532423003/diff/100001/content/browser/child_...
content/browser/child_process_launcher.cc:206: if (*zygote_handle == nullptr) {
I'd suggest adding a comment that mentions we're running on the PROCESS_LAUNCHER
thread here, so we don't need to worry about race conditions in the lazy
initialization.

https://codereview.chromium.org/1532423003/diff/100001/content/browser/child_...
content/browser/child_process_launcher.cc:343: zygote_(),
(And explicitly initialize to nullptr here if you change it.)

https://codereview.chromium.org/1532423003/diff/100001/content/browser/ppapi_...
File content/browser/ppapi_plugin_process_host.cc (right):

https://codereview.chromium.org/1532423003/diff/100001/content/browser/ppapi_...
content/browser/ppapi_plugin_process_host.cc:100: if (!is_broker_ &&
plugin_launcher.empty()) {
Nit: It's pretty negligible here, but in general I suggest trying to arrange if
statements to minimize how much code needs to be intended.  In this case, I
would write:

    if (is_broker_ || !plugin_launcher.empty())
      return nullptr;

    static ZygoteHandle zygote;
    return &zygote;

(Same goes for the other GetZygote functions too.)

https://codereview.chromium.org/1532423003/diff/100001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1532423003/diff/100001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:402: #if
!defined(OS_MACOSX) && !defined(OS_ANDROID)
We don't need the GetZygote function on OS X or Android anyway, so you could
also just completely #ifdef-out GetZygote on those platforms.  That would
actually be desirable too if it's not much trouble, because the linker can't do
dead code elimination on unused virtual functions (to my knowledge).

(Note: We do still need GetEnvironment and TakeIpcFd on OS X, but not on
Android.)

https://codereview.chromium.org/1532423003/diff/100001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:406:
ZygoteHostImpl::GetInstance()->SetRendererSandboxStatus(
This is kind of clunky, though I don't have a better concrete suggestion off
hand.

I'm wondering if we could make the ZygoteHandle visible externally as a static
RenderProcessHostImpl::zygote_ or something, and then other code could directly
use RenderProcessHostImpl::zygote_->GetSandboxStatus(), but that might have race
conditions.  Hrm.

I'd suggest at least adding a TODO(kerrnel) or TODO(mdempsky) to investigate
later.

https://codereview.chromium.org/1532423003/diff/100001/content/browser/zygote...
File content/browser/zygote_host/zygote_communication_linux.cc (right):

https://codereview.chromium.org/1532423003/diff/100001/content/browser/zygote...
content/browser/zygote_host/zygote_communication_linux.cc:1: // Copyright 2015
The Chromium Authors. All rights reserved.
(Are there any substantial differences in this code from the ZygoteHostImpl code
it replaces?)

https://codereview.chromium.org/1532423003/diff/100001/content/browser/zygote...
content/browser/zygote_host/zygote_communication_linux.cc:91: if (bytes_read !=
sizeof(sandbox_status_)) {
On 2015/12/21 21:19:08, Greg Kerr wrote:
> drop braces

FYI, in sandbox code (which sorta includes zygote code, since we own that too),
we tend to always use braces, even for single line bodies.

(The style guide doesn't lay out hard requirements here, so style preferences
vary about this across the source tree, unfortunately.)

[Greg K, 2016-01-05 19:22:59]

Patchset #8 (id:140001) has been deleted

[Greg K, 2016-01-05 19:23:09]

Patchset #8 (id:160001) has been deleted

[Greg K, 2016-01-05 19:23:58]

Patchset #8 (id:180001) has been deleted

[Greg K, 2016-01-05 19:24:10]

Patchset #8 (id:200001) has been deleted

[Greg K, 2016-01-05 21:42:13]

https://codereview.chromium.org/1532423003/diff/100001/content/browser/browse...
File content/browser/browser_main_loop.cc (left):

https://codereview.chromium.org/1532423003/diff/100001/content/browser/browse...
content/browser/browser_main_loop.cc:1038:
ZygoteHostImpl::GetInstance()->TearDownAfterLastChild();
On 2015/12/22 21:21:33, mdempsky wrote:
> You might try landing just this removal first/separately.  I think we've
> established that it's okay to remove and the trybots seem happy with it, but I
> can imagine some weird CrOS or non-trybot tests might start complaining.  (And
> if so, better to just have to revert/re-revert a simple one-line CL than your
> entire CL.)

Done.

https://codereview.chromium.org/1532423003/diff/100001/content/browser/child_...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/1532423003/diff/100001/content/browser/child_...
content/browser/child_process_launcher.cc:203: zygote = nullptr;
On 2015/12/22 21:21:33, mdempsky wrote:
> Hm, so if you use
> 
>     using ZygoteHandle = std::nullptr_t;
> 
> as the stub typedef, then you could just write:
> 
>     ZygoteHandle zygote = nullptr;
> 
> here and use "nullptr" instead of "ZygoteHandle()" in
> OnChildProcessStartedAndroid.  That seems a bit cleaner to me, what do you
> think?
> 
> (Sorry, I know I suggested the empty struct in the first place. :-))

Done.

https://codereview.chromium.org/1532423003/diff/100001/content/browser/child_...
content/browser/child_process_launcher.cc:206: if (*zygote_handle == nullptr) {
On 2015/12/22 21:21:33, mdempsky wrote:
> I'd suggest adding a comment that mentions we're running on the
PROCESS_LAUNCHER
> thread here, so we don't need to worry about race conditions in the lazy
> initialization.

Done.

https://codereview.chromium.org/1532423003/diff/100001/content/browser/child_...
content/browser/child_process_launcher.cc:343: zygote_(),
On 2015/12/22 21:21:33, mdempsky wrote:
> (And explicitly initialize to nullptr here if you change it.)

Done.

https://codereview.chromium.org/1532423003/diff/100001/content/browser/ppapi_...
File content/browser/ppapi_plugin_process_host.cc (right):

https://codereview.chromium.org/1532423003/diff/100001/content/browser/ppapi_...
content/browser/ppapi_plugin_process_host.cc:100: if (!is_broker_ &&
plugin_launcher.empty()) {
On 2015/12/22 21:21:33, mdempsky wrote:
> Nit: It's pretty negligible here, but in general I suggest trying to arrange
if
> statements to minimize how much code needs to be intended.  In this case, I
> would write:
> 
>     if (is_broker_ || !plugin_launcher.empty())
>       return nullptr;
> 
>     static ZygoteHandle zygote;
>     return &zygote;
> 
> (Same goes for the other GetZygote functions too.)

Done.

https://codereview.chromium.org/1532423003/diff/100001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1532423003/diff/100001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:402: #if
!defined(OS_MACOSX) && !defined(OS_ANDROID)
On 2015/12/22 21:21:33, mdempsky wrote:
> We don't need the GetZygote function on OS X or Android anyway, so you could
> also just completely #ifdef-out GetZygote on those platforms.  That would
> actually be desirable too if it's not much trouble, because the linker can't
do
> dead code elimination on unused virtual functions (to my knowledge).
> 
> (Note: We do still need GetEnvironment and TakeIpcFd on OS X, but not on
> Android.)

Done.

https://codereview.chromium.org/1532423003/diff/100001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:406:
ZygoteHostImpl::GetInstance()->SetRendererSandboxStatus(
On 2015/12/22 21:21:33, mdempsky wrote:
> This is kind of clunky, though I don't have a better concrete suggestion off
> hand.
> 
> I'm wondering if we could make the ZygoteHandle visible externally as a static
> RenderProcessHostImpl::zygote_ or something, and then other code could
directly
> use RenderProcessHostImpl::zygote_->GetSandboxStatus(), but that might have
race
> conditions.  Hrm.
> 
> I'd suggest at least adding a TODO(kerrnel) or TODO(mdempsky) to investigate
> later.

Done.

https://codereview.chromium.org/1532423003/diff/100001/content/browser/zygote...
File content/browser/zygote_host/zygote_communication_linux.cc (right):

https://codereview.chromium.org/1532423003/diff/100001/content/browser/zygote...
content/browser/zygote_host/zygote_communication_linux.cc:1: // Copyright 2015
The Chromium Authors. All rights reserved.
On 2015/12/22 21:21:33, mdempsky wrote:
> (Are there any substantial differences in this code from the ZygoteHostImpl
code
> it replaces?)

No, are you suggesting we retain the 2012 copyright notice since most of the
code is from then?

https://codereview.chromium.org/1532423003/diff/100001/content/browser/zygote...
content/browser/zygote_host/zygote_communication_linux.cc:91: if (bytes_read !=
sizeof(sandbox_status_)) {
On 2015/12/22 21:21:33, mdempsky wrote:
> On 2015/12/21 21:19:08, Greg Kerr wrote:
> > drop braces
> 
> FYI, in sandbox code (which sorta includes zygote code, since we own that
too),
> we tend to always use braces, even for single line bodies.
> 
> (The style guide doesn't lay out hard requirements here, so style preferences
> vary about this across the source tree, unfortunately.)

Acknowledged.

[mdempsky, 2016-01-05 22:41:57]

LGTM for content/browser/zygote_host

https://codereview.chromium.org/1532423003/diff/100001/content/browser/zygote...
File content/browser/zygote_host/zygote_communication_linux.cc (right):

https://codereview.chromium.org/1532423003/diff/100001/content/browser/zygote...
content/browser/zygote_host/zygote_communication_linux.cc:1: // Copyright 2015
The Chromium Authors. All rights reserved.
On 2016/01/05 21:42:13, Greg Kerr wrote:
> On 2015/12/22 21:21:33, mdempsky wrote:
> > (Are there any substantial differences in this code from the ZygoteHostImpl
> code
> > it replaces?)
> 
> No, are you suggesting we retain the 2012 copyright notice since most of the
> code is from then?

Oh, no.  Rietveld doesn't support file-scoped comments (AFAIK), so I just added
it to the first line instead.

I was just trying to gauge how closely I needed to look at the new file or if
there were any spots in particular you wanted me to look at.

https://codereview.chromium.org/1532423003/diff/220001/content/browser/zygote...
File content/browser/zygote_host/zygote_communication_linux.cc (right):

https://codereview.chromium.org/1532423003/diff/220001/content/browser/zygote...
content/browser/zygote_host/zygote_communication_linux.cc:55: #if
defined(OS_POSIX)
This shouldn't be necessary since Linux is always OS_POSIX?

https://codereview.chromium.org/1532423003/diff/220001/content/browser/zygote...
File content/browser/zygote_host/zygote_communication_linux.h (right):

https://codereview.chromium.org/1532423003/diff/220001/content/browser/zygote...
content/browser/zygote_host/zygote_communication_linux.h:73: // Set to true when
this is initialiazed successfuly.
Typo: "initialized" and "successfully"

https://codereview.chromium.org/1532423003/diff/220001/content/browser/zygote...
content/browser/zygote_host/zygote_communication_linux.h:74: bool init_;
I'd put this next to the other bools below so we have less wasted space from
alignment padding of the member variables.

https://codereview.chromium.org/1532423003/diff/220001/content/browser/zygote...
content/browser/zygote_host/zygote_communication_linux.h:82: // The listen of
running zygote children.
Typo: "list"

https://codereview.chromium.org/1532423003/diff/220001/content/browser/zygote...
File content/browser/zygote_host/zygote_host_impl_linux.h (right):

https://codereview.chromium.org/1532423003/diff/220001/content/browser/zygote...
content/browser/zygote_host/zygote_host_impl_linux.h:8: #include <map>
No need for <map> here.

Oh, you should add <sys/types.h> for pid_t and "base/process/process_handle.h"
for base::ProcessHandle though.

[Greg K, 2016-01-06 18:20:41]

kerrnel@chromium.org changed reviewers:
+ avi@chromium.org, mseaborn@chromium.org, thestig@chromium.org

[Greg K, 2016-01-06 18:20:41]

mseaborn@chromium.org: Please review changes in 
nacl

thestig@chromium.org: Please review changes in 
chome/

avi@chromium.org: Please review changes in 
content/

Thanks!

- Greg

https://codereview.chromium.org/1532423003/diff/220001/content/browser/zygote...
File content/browser/zygote_host/zygote_communication_linux.cc (right):

https://codereview.chromium.org/1532423003/diff/220001/content/browser/zygote...
content/browser/zygote_host/zygote_communication_linux.cc:55: #if
defined(OS_POSIX)
On 2016/01/05 22:41:57, mdempsky wrote:
> This shouldn't be necessary since Linux is always OS_POSIX?

Done.

https://codereview.chromium.org/1532423003/diff/220001/content/browser/zygote...
File content/browser/zygote_host/zygote_communication_linux.h (right):

https://codereview.chromium.org/1532423003/diff/220001/content/browser/zygote...
content/browser/zygote_host/zygote_communication_linux.h:73: // Set to true when
this is initialiazed successfuly.
On 2016/01/05 22:41:57, mdempsky wrote:
> Typo: "initialized" and "successfully"

Done.

https://codereview.chromium.org/1532423003/diff/220001/content/browser/zygote...
content/browser/zygote_host/zygote_communication_linux.h:74: bool init_;
On 2016/01/05 22:41:57, mdempsky wrote:
> I'd put this next to the other bools below so we have less wasted space from
> alignment padding of the member variables.

Done.

https://codereview.chromium.org/1532423003/diff/220001/content/browser/zygote...
content/browser/zygote_host/zygote_communication_linux.h:82: // The listen of
running zygote children.
On 2016/01/05 22:41:57, mdempsky wrote:
> Typo: "list"

Done.

https://codereview.chromium.org/1532423003/diff/220001/content/browser/zygote...
File content/browser/zygote_host/zygote_host_impl_linux.h (right):

https://codereview.chromium.org/1532423003/diff/220001/content/browser/zygote...
content/browser/zygote_host/zygote_host_impl_linux.h:8: #include <map>
On 2016/01/05 22:41:57, mdempsky wrote:
> No need for <map> here.
> 
> Oh, you should add <sys/types.h> for pid_t and "base/process/process_handle.h"
> for base::ProcessHandle though.

Done.

[Avi (use Gerrit), 2016-01-06 19:07:56]

Mostly IWYU failures.

https://codereview.chromium.org/1532423003/diff/260001/content/browser/zygote...
File content/browser/zygote_host/zygote_communication_linux.cc (right):

https://codereview.chromium.org/1532423003/diff/260001/content/browser/zygote...
content/browser/zygote_host/zygote_communication_linux.cc:46: if (memcmp(buf,
expect_msg, expect_len) != 0)
#include <string.h>

https://codereview.chromium.org/1532423003/diff/260001/content/browser/zygote...
File content/browser/zygote_host/zygote_communication_linux.h (right):

https://codereview.chromium.org/1532423003/diff/260001/content/browser/zygote...
content/browser/zygote_host/zygote_communication_linux.h:27: //
base::kNullProcessHandle;
Weird wrapping; please re-wrap.

https://codereview.chromium.org/1532423003/diff/260001/content/public/common/...
File content/public/common/sandboxed_process_launcher_delegate.h (right):

https://codereview.chromium.org/1532423003/diff/260001/content/public/common/...
content/public/common/sandboxed_process_launcher_delegate.h:29: using
ZygoteHandle = std::nullptr_t;
#include <cstddef>

[Lei Zhang, 2016-01-06 19:23:46]

On 2016/01/06 18:20:41, Greg Kerr wrote:
> mailto:thestig@chromium.org: Please review changes in 
> chome/

chrome/ lgtm

[Greg K, 2016-01-06 22:24:21]

https://codereview.chromium.org/1532423003/diff/260001/content/browser/zygote...
File content/browser/zygote_host/zygote_communication_linux.cc (right):

https://codereview.chromium.org/1532423003/diff/260001/content/browser/zygote...
content/browser/zygote_host/zygote_communication_linux.cc:46: if (memcmp(buf,
expect_msg, expect_len) != 0)
On 2016/01/06 19:07:55, Avi wrote:
> #include <string.h>

Done.

https://codereview.chromium.org/1532423003/diff/260001/content/browser/zygote...
File content/browser/zygote_host/zygote_communication_linux.h (right):

https://codereview.chromium.org/1532423003/diff/260001/content/browser/zygote...
content/browser/zygote_host/zygote_communication_linux.h:27: //
base::kNullProcessHandle;
On 2016/01/06 19:07:55, Avi wrote:
> Weird wrapping; please re-wrap.

Done.

https://codereview.chromium.org/1532423003/diff/260001/content/public/common/...
File content/public/common/sandboxed_process_launcher_delegate.h (right):

https://codereview.chromium.org/1532423003/diff/260001/content/public/common/...
content/public/common/sandboxed_process_launcher_delegate.h:29: using
ZygoteHandle = std::nullptr_t;
On 2016/01/06 19:07:56, Avi wrote:
> #include <cstddef>

Done.

[Avi (use Gerrit), 2016-01-07 01:23:40]

:) LGTM

[Greg K, 2016-01-07 18:02:08]

kerrnel@chromium.org changed reviewers:
- mseaborn@chromium.org

[Greg K, 2016-01-07 18:03:36]

kerrnel@chromium.org changed reviewers:
+ mseaborn@chromium.org

[Mark Seaborn, 2016-01-11 22:42:39]

https://codereview.chromium.org/1532423003/diff/280001/components/nacl/browse...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/1532423003/diff/280001/components/nacl/browse...
components/nacl/browser/nacl_process_host.cc:186: static content::ZygoteHandle
zygote;
Does this mean the zygote process is launched on demand?  Doesn't that break
auto-update via Linux package managers (as described in
http://neugierig.org/software/chromium/notes/2011/08/zygote.html)?

[mdempsky, 2016-01-11 22:50:28]

https://codereview.chromium.org/1532423003/diff/280001/components/nacl/browse...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/1532423003/diff/280001/components/nacl/browse...
components/nacl/browser/nacl_process_host.cc:186: static content::ZygoteHandle
zygote;
On 2016/01/11 22:42:39, Mark Seaborn wrote:
> Does this mean the zygote process is launched on demand?

Yes.  I'm not convinced that's a problem per se (see below), but perhaps we
should try to launch all of the zygote processes at startup for now, and change
to on-demand for non-critical zygotes in a separate CL.

> Doesn't that break
> auto-update via Linux package managers (as described in
> http://neugierig.org/software/chromium/notes/2011/08/zygote.html)

It does.  However, at least my anecdotal experience with Chrome on Linux in
recent history is that we don't handle Chrome being updated on disk underneath
us very gracefully anyway.  Whenever the hot-dog-update icon appears for me,
Chrome becomes super laggy and crashy anyway, and I *have* to reboot to get even
basic things like Gmail and YouTube usable again.

[Greg K, 2016-01-15 01:40:12]

Patchset #12 (id:300001) has been deleted

[Greg K, 2016-01-15 01:40:29]

Patchset #12 (id:320001) has been deleted

[Greg K, 2016-01-15 01:40:49]

Patchset #12 (id:340001) has been deleted

[Greg K, 2016-01-15 18:23:00]

Hi all,

I had to make additional changes to ensure that the zygotes are loaded right at
startup time, per Mark's concerns. This protects Chrome in case  its binaries
are updated while its running. 

Can all the reviewers take a look at the most recent delta and make sure they're
still OK with the changes?

Thanks!

- Greg

[Lei Zhang, 2016-01-15 19:55:48]

On 2016/01/15 18:23:00, Greg Kerr wrote:
> Can all the reviewers take a look at the most recent delta and make sure
they're
> still OK with the changes?

chrome/ lgtm still stands for patch set 12.

[Avi (use Gerrit), 2016-01-15 19:57:14]

still lgtm

[mdempsky, 2016-01-15 20:29:42]

lgtm with nits

https://codereview.chromium.org/1532423003/diff/360001/components/nacl/browse...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/1532423003/diff/360001/components/nacl/browse...
components/nacl/browser/nacl_process_host.cc:160: content::ZygoteHandle zygote;
Google C++ style guide encourages prefixing globals with g_.

Also, I know these are file-scoped, but let's still call them g_nacl_zygote,
g_ppapi_zygote, etc. just to make it a bit clearer.

https://codereview.chromium.org/1532423003/diff/360001/content/browser/ppapi_...
File content/browser/ppapi_plugin_process_host.cc (right):

https://codereview.chromium.org/1532423003/diff/360001/content/browser/ppapi_...
content/browser/ppapi_plugin_process_host.cc:210: zygote = new
ZygoteCommunication();
Use ZygoteHost::CreateZygote().  (Here and the next two EarlyZygoteLaunch
functions too.)

https://codereview.chromium.org/1532423003/diff/360001/content/public/browser...
File content/public/browser/zygote_host_linux.h (right):

https://codereview.chromium.org/1532423003/diff/360001/content/public/browser...
content/public/browser/zygote_host_linux.h:17: #if defined(OS_LINUX)
I think these should go into a new content/public/browser/zygote_handle.h
header, otherwise there will be a compile error due to duplicate type
definitions if you try to include zygote_host_linux.h and
sandboxed_process_launcher_delegate.h in the same .cc file.

Also, this #if condition needs to be defined(OS_POSIX) && !defined(OS_MACOSX) &&
!defined(OS_ANDROID) like all the other Zygote-related stuff.

[mdempsky, 2016-01-16 05:24:26]

(Don't forget to send "Done" emails after uploading new patch sets that respond
to feedback so reviewers know to take another look.  Rietveld doesn't
automatically send email notifications when you upload new patch sets.)

https://codereview.chromium.org/1532423003/diff/380001/content/public/browser...
File content/public/browser/zygote_handle_linux.h (right):

https://codereview.chromium.org/1532423003/diff/380001/content/public/browser...
content/public/browser/zygote_handle_linux.h:15: namespace zygote_handle {
This is really content/OWNERS' call, but I would get rid of this.  I don't think
there's any precedent for sub-namespaces within content, and you're not at risk
of "CreateZygote" colliding with any other symbols in namespace content.

https://codereview.chromium.org/1532423003/diff/380001/content/public/common/...
File content/public/common/zygote_alias.h (right):

https://codereview.chromium.org/1532423003/diff/380001/content/public/common/...
content/public/common/zygote_alias.h:5: #ifndef
CONTENT_PUBLIC_COMMON_ZYGOTE_ALIAS_H_
Nit: The style guide convention is that the header that defines FooBar should be
named foo_bar.h.  This file is primarily about defining ZygoteHandle, so it
should be named zygote_handle.h.

[Greg K, 2016-01-19 19:34:19]

https://codereview.chromium.org/1532423003/diff/360001/components/nacl/browse...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/1532423003/diff/360001/components/nacl/browse...
components/nacl/browser/nacl_process_host.cc:160: content::ZygoteHandle zygote;
On 2016/01/15 20:29:41, mdempsky wrote:
> Google C++ style guide encourages prefixing globals with g_.
> 
> Also, I know these are file-scoped, but let's still call them g_nacl_zygote,
> g_ppapi_zygote, etc. just to make it a bit clearer.

Done.

https://codereview.chromium.org/1532423003/diff/360001/content/browser/ppapi_...
File content/browser/ppapi_plugin_process_host.cc (right):

https://codereview.chromium.org/1532423003/diff/360001/content/browser/ppapi_...
content/browser/ppapi_plugin_process_host.cc:210: zygote = new
ZygoteCommunication();
On 2016/01/15 20:29:42, mdempsky wrote:
> Use ZygoteHost::CreateZygote().  (Here and the next two EarlyZygoteLaunch
> functions too.)

Done.

https://codereview.chromium.org/1532423003/diff/360001/content/public/browser...
File content/public/browser/zygote_host_linux.h (right):

https://codereview.chromium.org/1532423003/diff/360001/content/public/browser...
content/public/browser/zygote_host_linux.h:17: #if defined(OS_LINUX)
On 2016/01/15 20:29:42, mdempsky wrote:
> I think these should go into a new content/public/browser/zygote_handle.h
> header, otherwise there will be a compile error due to duplicate type
> definitions if you try to include zygote_host_linux.h and
> sandboxed_process_launcher_delegate.h in the same .cc file.
> 
> Also, this #if condition needs to be defined(OS_POSIX) && !defined(OS_MACOSX)
&&
> !defined(OS_ANDROID) like all the other Zygote-related stuff.

Done.

https://codereview.chromium.org/1532423003/diff/380001/content/public/browser...
File content/public/browser/zygote_handle_linux.h (right):

https://codereview.chromium.org/1532423003/diff/380001/content/public/browser...
content/public/browser/zygote_handle_linux.h:15: namespace zygote_handle {
On 2016/01/16 05:24:26, mdempsky wrote:
> This is really content/OWNERS' call, but I would get rid of this.  I don't
think
> there's any precedent for sub-namespaces within content, and you're not at
risk
> of "CreateZygote" colliding with any other symbols in namespace content.

Done.

https://codereview.chromium.org/1532423003/diff/380001/content/public/common/...
File content/public/common/zygote_alias.h (right):

https://codereview.chromium.org/1532423003/diff/380001/content/public/common/...
content/public/common/zygote_alias.h:5: #ifndef
CONTENT_PUBLIC_COMMON_ZYGOTE_ALIAS_H_
On 2016/01/16 05:24:26, mdempsky wrote:
> Nit: The style guide convention is that the header that defines FooBar should
be
> named foo_bar.h.  This file is primarily about defining ZygoteHandle, so it
> should be named zygote_handle.h.

Done.

[mdempsky, 2016-01-19 19:39:31]

lgtm for zygote OWNERS

avi: Can you double check that the content/public changes still LGTY?

[Avi (use Gerrit), 2016-01-19 19:42:41]

still lgtm

[Mark Seaborn, 2016-01-19 21:06:43]

https://codereview.chromium.org/1532423003/diff/400001/components/nacl/browse...
File components/nacl/browser/nacl_process_host.h (right):

https://codereview.chromium.org/1532423003/diff/400001/components/nacl/browse...
components/nacl/browser/nacl_process_host.h:96: #if defined(OS_POSIX) &&
!defined(OS_ANDROID) && !defined(OS_MACOSX)
NaCl isn't enabled on Android so you probably don't need the OS_ANDROID check
here (and it could be misleading to have it).  Same in other cases that are
behind !defined(DISABLE_NACL).

https://codereview.chromium.org/1532423003/diff/400001/content/browser/child_...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/1532423003/diff/400001/content/browser/child_...
content/browser/child_process_launcher.cc:208: if (*zygote_handle == nullptr) {
So there is still this code for launching the zygote process on demand, then? 
Is that still used (e.g. for plugins)?

Does this leave a small race condition?  chrome/browser/chrome_browser_main.cc
uses PostTask() to run nacl::NaClProcessHost::EarlyZygoteLaunch().  If that task
hasn't run by the time we try to use the zygote, then one zygote would get
launched here, and then a second zygote would get launched by
EarlyZygoteLaunch() (or the DCHECK in EarlyZygoteLaunch() would fail).

Maybe EarlyZygoteLaunch() should check whether the zygote has been launched
already?

Or maybe this would never be an issue in practice because anything that launches
a NaCl process would occur from a task run by PostTask() that is queued later
than EarlyZygoteLaunch()?

[mdempsky, 2016-01-19 21:26:43]

https://codereview.chromium.org/1532423003/diff/400001/content/browser/child_...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/1532423003/diff/400001/content/browser/child_...
content/browser/child_process_launcher.cc:208: if (*zygote_handle == nullptr) {
On 2016/01/19 21:06:43, Mark Seaborn wrote:
> So there is still this code for launching the zygote process on demand, then? 
> Is that still used (e.g. for plugins)?

It's not currently used, but we'll start using it once/if we switch to lazily
starting zygotes.

> Does this leave a small race condition?

My understanding is it shouldn't be racy: EarlyZygoteLaunch and
LaunchOnLauncherThread are both posted to the PROCESS_LAUNCHER thread, which
should ensure they're ordered.


kerrnel: This does make me realize we should use CreateZygote() here too, and
that CreateZygote() should include
DCHECK_CURRENTLY_ON(BrowserThread::PROCESS_LAUNCHER);

[Mark Seaborn, 2016-01-19 22:16:38]

On 2016/01/19 21:26:43, mdempsky wrote:
>
https://codereview.chromium.org/1532423003/diff/400001/content/browser/child_...
> File content/browser/child_process_launcher.cc (right):
> 
>
https://codereview.chromium.org/1532423003/diff/400001/content/browser/child_...
> content/browser/child_process_launcher.cc:208: if (*zygote_handle == nullptr)
{
> On 2016/01/19 21:06:43, Mark Seaborn wrote:
> > So there is still this code for launching the zygote process on demand,
then? 
> > Is that still used (e.g. for plugins)?
> 
> It's not currently used, but we'll start using it once/if we switch to lazily
> starting zygotes.

OK.  You might want to remove the check for the sake of clarity, if it's dead
code at the moment, and add it back at the point when it is needed.

> > Does this leave a small race condition?
> 
> My understanding is it shouldn't be racy: EarlyZygoteLaunch and
> LaunchOnLauncherThread are both posted to the PROCESS_LAUNCHER thread, which
> should ensure they're ordered.

OK, thanks, LGTM.

[Greg K, 2016-01-19 22:20:16]

https://codereview.chromium.org/1532423003/diff/400001/components/nacl/browse...
File components/nacl/browser/nacl_process_host.h (right):

https://codereview.chromium.org/1532423003/diff/400001/components/nacl/browse...
components/nacl/browser/nacl_process_host.h:96: #if defined(OS_POSIX) &&
!defined(OS_ANDROID) && !defined(OS_MACOSX)
On 2016/01/19 21:06:43, Mark Seaborn wrote:
> NaCl isn't enabled on Android so you probably don't need the OS_ANDROID check
> here (and it could be misleading to have it).  Same in other cases that are
> behind !defined(DISABLE_NACL).

Done.

https://codereview.chromium.org/1532423003/diff/400001/content/browser/child_...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/1532423003/diff/400001/content/browser/child_...
content/browser/child_process_launcher.cc:208: if (*zygote_handle == nullptr) {
On 2016/01/19 21:26:43, mdempsky wrote:
> On 2016/01/19 21:06:43, Mark Seaborn wrote:
> > So there is still this code for launching the zygote process on demand,
then? 
> > Is that still used (e.g. for plugins)?
> 
> It's not currently used, but we'll start using it once/if we switch to lazily
> starting zygotes.
> 
> > Does this leave a small race condition?
> 
> My understanding is it shouldn't be racy: EarlyZygoteLaunch and
> LaunchOnLauncherThread are both posted to the PROCESS_LAUNCHER thread, which
> should ensure they're ordered.
> 
> 
> kerrnel: This does make me realize we should use CreateZygote() here too, and
> that CreateZygote() should include
> DCHECK_CURRENTLY_ON(BrowserThread::PROCESS_LAUNCHER);

I talked to Matt, except for NaCl, the zygotes are currently launched in the
main thread initialization, which should be complete before the PROCESS_LAUNCHER
thread is receiving any requests to create rendererers, etc.

[Greg K, 2016-01-20 18:10:33]

The CQ bit was checked by kerrnel@chromium.org

[Greg K, 2016-01-20 18:10:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, avi@chromium.org, mdempsky@chromium.org
Link to the patchset: https://codereview.chromium.org/1532423003/#ps420001
(title: "Cleanup the NaCl defines.")

[Greg K, 2016-01-20 18:11:00]

The CQ bit was unchecked by kerrnel@chromium.org

[Greg K, 2016-01-20 19:25:23]

Patchset #15 (id:420001) has been deleted

[Greg K, 2016-01-20 20:07:28]

The CQ bit was checked by kerrnel@chromium.org

[Greg K, 2016-01-20 20:07:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, mseaborn@chromium.org, avi@chromium.org,
mdempsky@chromium.org
Link to the patchset: https://codereview.chromium.org/1532423003/#ps440001
(title: "Cleanup the NaCl defines.")

[commit-bot: I haz the power, 2016-01-20 20:09:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1532423003/440001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1532423003/440001

[commit-bot: I haz the power, 2016-01-20 21:11:40]

Description was changed from

==========
Have each SandboxedProcessLauncherDelegate maintain a zygote.

To improve component updates of PPAPI plugins, Chrome needs multiple zygotes.
This will allow the PPAPI zygote to be recreated when a plugin is updated.
This CL allows Chrome to maintain a zygote for each process type by having
each SandboxedProcessLauncherDelegate maintain a class which can communicate
with its respective zygote. This CL will be followed up with work to allow
customization of zygotes, which will give Chrome the improve component update
experience.

BUG=569191
==========

to

==========
Have each SandboxedProcessLauncherDelegate maintain a zygote.

To improve component updates of PPAPI plugins, Chrome needs multiple zygotes.
This will allow the PPAPI zygote to be recreated when a plugin is updated.
This CL allows Chrome to maintain a zygote for each process type by having
each SandboxedProcessLauncherDelegate maintain a class which can communicate
with its respective zygote. This CL will be followed up with work to allow
customization of zygotes, which will give Chrome the improve component update
experience.

BUG=569191
==========

[commit-bot: I haz the power, 2016-01-20 21:11:41]

Committed patchset #15 (id:440001)

[commit-bot: I haz the power, 2016-01-20 21:12:44]

Description was changed from

==========
Have each SandboxedProcessLauncherDelegate maintain a zygote.

To improve component updates of PPAPI plugins, Chrome needs multiple zygotes.
This will allow the PPAPI zygote to be recreated when a plugin is updated.
This CL allows Chrome to maintain a zygote for each process type by having
each SandboxedProcessLauncherDelegate maintain a class which can communicate
with its respective zygote. This CL will be followed up with work to allow
customization of zygotes, which will give Chrome the improve component update
experience.

BUG=569191
==========

to

==========
Have each SandboxedProcessLauncherDelegate maintain a zygote.

To improve component updates of PPAPI plugins, Chrome needs multiple zygotes.
This will allow the PPAPI zygote to be recreated when a plugin is updated.
This CL allows Chrome to maintain a zygote for each process type by having
each SandboxedProcessLauncherDelegate maintain a class which can communicate
with its respective zygote. This CL will be followed up with work to allow
customization of zygotes, which will give Chrome the improve component update
experience.

BUG=569191

Committed: https://crrev.com/3c1e16b490255119b6f70f94d1716645e897b185
Cr-Commit-Position: refs/heads/master@{#370488}
==========

[commit-bot: I haz the power, 2016-01-20 21:12:45]

Patchset 15 (id:??) landed as
https://crrev.com/3c1e16b490255119b6f70f94d1716645e897b185
Cr-Commit-Position: refs/heads/master@{#370488}

[Greg K, 2016-01-21 20:26:46]

A revert of this CL (patchset #15 id:440001) has been created in
https://codereview.chromium.org/1617213002/ by kerrnel@chromium.org.

The reason for reverting is: Revert "Have each SandboxedProcessLauncherDelegate
maintain a zygote."

This reverts commit 3c1e16b490255119b6f70f94d1716645e897b185.
This broke the Site Isolation FYI bot failure:
https://build.chromium.org/p/chromium.fyi/builders/Site%20Isolation%20Linux/b...

To re-land the CL, that failure needs to be diagnosed and corrected..

[Greg K, 2016-01-21 21:17:08]

Description was changed from

==========
Have each SandboxedProcessLauncherDelegate maintain a zygote.

To improve component updates of PPAPI plugins, Chrome needs multiple zygotes.
This will allow the PPAPI zygote to be recreated when a plugin is updated.
This CL allows Chrome to maintain a zygote for each process type by having
each SandboxedProcessLauncherDelegate maintain a class which can communicate
with its respective zygote. This CL will be followed up with work to allow
customization of zygotes, which will give Chrome the improve component update
experience.

BUG=569191

Committed: https://crrev.com/3c1e16b490255119b6f70f94d1716645e897b185
Cr-Commit-Position: refs/heads/master@{#370488}
==========

to

==========
Have each SandboxedProcessLauncherDelegate maintain a zygote.

To improve component updates of PPAPI plugins, Chrome needs multiple zygotes.
This will allow the PPAPI zygote to be recreated when a plugin is updated.
This CL allows Chrome to maintain a zygote for each process type by having
each SandboxedProcessLauncherDelegate maintain a class which can communicate
with its respective zygote. This CL will be followed up with work to allow
customization of zygotes, which will give Chrome the improve component update
experience.

BUG=569191

Committed: https://crrev.com/3c1e16b490255119b6f70f94d1716645e897b185
Cr-Commit-Position: refs/heads/master@{#370488}
==========

[Greg K, 2016-01-22 00:55:49]

Patchset #16 (id:460001) has been deleted

[Greg K, 2016-01-22 00:57:55]

Patchset #16 (id:480001) has been deleted

[Greg K, 2016-01-22 18:00:17]

Matthew, will you please look at the delta for this most recent patch set? It
fixes the SUID sandbox initialization failure by making sure that the
sandbox_binary_ path is setup before zgyotes are initialized. It also corrects
two other small issues that I noticed when combing through the CL (an incorrect
header guard and a missing UMA log).

Thanks,

Greg

[mdempsky, 2016-01-22 18:46:47]

lgtm

lgtm

[Greg K, 2016-01-22 21:09:52]

The CQ bit was checked by kerrnel@chromium.org

[Greg K, 2016-01-22 21:09:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, mseaborn@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1532423003/#ps500001
(title: "Correctly initialize the sandbox_binary_ command before launching the
zygote.")

[commit-bot: I haz the power, 2016-01-22 21:10:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1532423003/500001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1532423003/500001

[commit-bot: I haz the power, 2016-01-22 21:16:23]

Description was changed from

==========
Have each SandboxedProcessLauncherDelegate maintain a zygote.

To improve component updates of PPAPI plugins, Chrome needs multiple zygotes.
This will allow the PPAPI zygote to be recreated when a plugin is updated.
This CL allows Chrome to maintain a zygote for each process type by having
each SandboxedProcessLauncherDelegate maintain a class which can communicate
with its respective zygote. This CL will be followed up with work to allow
customization of zygotes, which will give Chrome the improve component update
experience.

BUG=569191

Committed: https://crrev.com/3c1e16b490255119b6f70f94d1716645e897b185
Cr-Commit-Position: refs/heads/master@{#370488}
==========

to

==========
Have each SandboxedProcessLauncherDelegate maintain a zygote.

To improve component updates of PPAPI plugins, Chrome needs multiple zygotes.
This will allow the PPAPI zygote to be recreated when a plugin is updated.
This CL allows Chrome to maintain a zygote for each process type by having
each SandboxedProcessLauncherDelegate maintain a class which can communicate
with its respective zygote. This CL will be followed up with work to allow
customization of zygotes, which will give Chrome the improve component update
experience.

BUG=569191

Committed: https://crrev.com/3c1e16b490255119b6f70f94d1716645e897b185
Cr-Commit-Position: refs/heads/master@{#370488}
==========

[commit-bot: I haz the power, 2016-01-22 21:16:24]

Committed patchset #16 (id:500001)

[commit-bot: I haz the power, 2016-01-22 21:17:13]

Description was changed from

==========
Have each SandboxedProcessLauncherDelegate maintain a zygote.

To improve component updates of PPAPI plugins, Chrome needs multiple zygotes.
This will allow the PPAPI zygote to be recreated when a plugin is updated.
This CL allows Chrome to maintain a zygote for each process type by having
each SandboxedProcessLauncherDelegate maintain a class which can communicate
with its respective zygote. This CL will be followed up with work to allow
customization of zygotes, which will give Chrome the improve component update
experience.

BUG=569191

Committed: https://crrev.com/3c1e16b490255119b6f70f94d1716645e897b185
Cr-Commit-Position: refs/heads/master@{#370488}
==========

to

==========
Have each SandboxedProcessLauncherDelegate maintain a zygote.

To improve component updates of PPAPI plugins, Chrome needs multiple zygotes.
This will allow the PPAPI zygote to be recreated when a plugin is updated.
This CL allows Chrome to maintain a zygote for each process type by having
each SandboxedProcessLauncherDelegate maintain a class which can communicate
with its respective zygote. This CL will be followed up with work to allow
customization of zygotes, which will give Chrome the improve component update
experience.

BUG=569191

Committed: https://crrev.com/3c1e16b490255119b6f70f94d1716645e897b185
Cr-Commit-Position: refs/heads/master@{#370488}

Committed: https://crrev.com/afd49a83be5da2df6667a2afd14239d7783aff52
Cr-Commit-Position: refs/heads/master@{#371042}
==========

[commit-bot: I haz the power, 2016-01-22 21:17:14]

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

[Greg K, 2016-01-28 19:19:02]

A revert of this CL (patchset #16 id:500001) has been created in
https://codereview.chromium.org/1640123005/ by kerrnel@chromium.org.

The reason for reverting is: This change has caused several performance
concerns, and those needs to be addressed before we ship this. Because my focus
is shifting to other goals for the time being, I am reverting this until it can
be sped up and re-landed. .

[Lei Zhang, 2017-04-27 22:57:33]

On 2016/01/28 19:19:02, Greg K wrote:
> A revert of this CL (patchset #16 id:500001) has been created in
> https://codereview.chromium.org/1640123005/ by mailto:kerrnel@chromium.org.
> 
> The reason for reverting is: This change has caused several performance
> concerns, and those needs to be addressed before we ship this. Because my
focus
> is shifting to other goals for the time being, I am reverting this until it
can
> be sped up and re-landed. .

It looks like this second revert never landed, and we "temporarily" landed
https://codereview.chromium.org/1643533006 instead. That leaves a bunch of
unused EarlyZygoteLaunch() methods from this CL. Can I delete those?

[Greg K, 2017-04-27 22:59:14]

jamescook@ just posted a CL to delete them as well:
https://codereview.chromium.org/2848603003/

Is this is the same issue?

Regards,

Greg

On Thu, Apr 27, 2017 at 3:57 PM,  <thestig@chromium.org> wrote:
> On 2016/01/28 19:19:02, Greg K wrote:
>> A revert of this CL (patchset #16 id:500001) has been created in
>> https://codereview.chromium.org/1640123005/ by
>> mailto:kerrnel@chromium.org.
>>
>> The reason for reverting is: This change has caused several performance
>> concerns, and those needs to be addressed before we ship this. Because my
> focus
>> is shifting to other goals for the time being, I am reverting this until
>> it
> can
>> be sped up and re-landed. .
>
> It looks like this second revert never landed, and we "temporarily" landed
> https://codereview.chromium.org/1643533006 instead. That leaves a bunch of
> unused EarlyZygoteLaunch() methods from this CL. Can I delete those?
>
> https://codereview.chromium.org/1532423003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Lei Zhang, 2017-04-27 23:00:08]

On 2017/04/27 22:59:14, Greg K wrote:
> jamescook@ just posted a CL to delete them as well:
> https://codereview.chromium.org/2848603003/
> 
> Is this is the same issue?

Yes. Wow. How did we notice it at the same time? :)