Clean up Linux zygote creation code

Clean up Linux zygote creation code

The browser code contains partial support for using multiple zygote
processes, which exists to support Flash component updates. However,
at present the browser process only creates a single zygote, on startup,
and uses that for everything. Clean up the code to make that more clear.

* Remove probably-unused lazy zygote creation code.
* Always use ZygoteHandle instead of ZygoteHandle*. ZygoteHandle is always
a pointer under the hood.
* Add CHECKs to validate the assumption there is only one zygote.

This came up while investigating a zygote check failure on Chrome OS.
I'm concerned that the lazy-launch code could be starting a zygote during
browser process shutdown. See crbug.com/692227

BUG=692227, 569191
TEST=manually load web pages, extensions and nacl apps to trigger child
process creation, verify they load properly

Review-Url: https://codereview.chromium.org/2850903002
Cr-Commit-Position: refs/heads/master@{#468331}
Committed: https://chromium.googlesource.com/chromium/src/+/ca8595ada8cf0a185b7abc5d112c7e9dcd66e3bb

[James Cook, 2017-04-28 19:49:02]

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

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

Dry run: CQ is trying da patch.

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

[James Cook, 2017-04-28 20:07:51]

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

[James Cook, 2017-04-28 20:07:52]

kerrnel, please take a look.

https://codereview.chromium.org/2850903002/diff/1/content/browser/child_proce...
File content/browser/child_process_launcher_helper_linux.cc (left):

https://codereview.chromium.org/2850903002/diff/1/content/browser/child_proce...
content/browser/child_process_launcher_helper_linux.cc:75: if (*zygote_handle ==
nullptr) {
This bit is a little subtle, and is the thing I'm worried could be causing the
zygote check failure I saw.

In the old code, delegate_->GetZygote() can return:
* null, which means "don't use a zygote"
* a pointer to a non-null ZygoteHandle (aka ZygoteCommunication*), which means
"use this zygote for launching". I think right now that ZygoteHandle is always
the "generic zygote"
* a pointer to a null ZygoteHandle, which means "lazily create a zygote for this
process". I don't think that is currently used. However, if it was used it could
result in extra zygotes being launched, which could cause the problem I saw.

Since the current intent of the code is to use a single zygote, I changed
GetZygote() to always return the handle, so the return value only has 2 possible
meanings.

https://codereview.chromium.org/2850903002/diff/1/content/browser/zygote_host...
File content/browser/zygote_host/zygote_handle_linux.cc (right):

https://codereview.chromium.org/2850903002/diff/1/content/browser/zygote_host...
content/browser/zygote_host/zygote_handle_linux.cc:12: // Intentionally leaked.
The old code leaks the ZygoteCommunication object. Is that intentional?

https://codereview.chromium.org/2850903002/diff/1/content/public/common/sandb...
File content/public/common/sandboxed_process_launcher_delegate.cc (right):

https://codereview.chromium.org/2850903002/diff/1/content/public/common/sandb...
content/public/common/sandboxed_process_launcher_delegate.cc:33: return nullptr;
I could also make this return GetGenericZygote().

[Greg K, 2017-04-28 20:51:40]

On 2017/04/28 20:07:52, James Cook wrote:
> kerrnel, please take a look.
> 
>
https://codereview.chromium.org/2850903002/diff/1/content/browser/child_proce...
> File content/browser/child_process_launcher_helper_linux.cc (left):
> 
>
https://codereview.chromium.org/2850903002/diff/1/content/browser/child_proce...
> content/browser/child_process_launcher_helper_linux.cc:75: if (*zygote_handle
==
> nullptr) {
> This bit is a little subtle, and is the thing I'm worried could be causing the
> zygote check failure I saw.
> 
> In the old code, delegate_->GetZygote() can return:
> * null, which means "don't use a zygote"
> * a pointer to a non-null ZygoteHandle (aka ZygoteCommunication*), which means
> "use this zygote for launching". I think right now that ZygoteHandle is always
> the "generic zygote"
> * a pointer to a null ZygoteHandle, which means "lazily create a zygote for
this
> process". I don't think that is currently used. However, if it was used it
could
> result in extra zygotes being launched, which could cause the problem I saw.
> 
> Since the current intent of the code is to use a single zygote, I changed
> GetZygote() to always return the handle, so the return value only has 2
possible
> meanings.
> 
>
https://codereview.chromium.org/2850903002/diff/1/content/browser/zygote_host...
> File content/browser/zygote_host/zygote_handle_linux.cc (right):
> 
>
https://codereview.chromium.org/2850903002/diff/1/content/browser/zygote_host...
> content/browser/zygote_host/zygote_handle_linux.cc:12: // Intentionally
leaked.
> The old code leaks the ZygoteCommunication object. Is that intentional?
> 
>
https://codereview.chromium.org/2850903002/diff/1/content/public/common/sandb...
> File content/public/common/sandboxed_process_launcher_delegate.cc (right):
> 
>
https://codereview.chromium.org/2850903002/diff/1/content/public/common/sandb...
> content/public/common/sandboxed_process_launcher_delegate.cc:33: return
nullptr;
> I could also make this return GetGenericZygote().

 Thanks for doing some much needed cleanup. I think this looks good, but can
jln@ or mdempsky@ also give it a pass?

[commit-bot: I haz the power, 2017-04-28 21:20:51]

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

[commit-bot: I haz the power, 2017-04-28 21:20:52]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-04-28 21:39:11]

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

[James Cook, 2017-04-28 21:39:12]

mdempsky, can you take a look? Thanks.

[mdempsky, 2017-04-28 22:13:45]

lgtm

Like you identified, the pointer-to-handle pattern was used so we could
centralize the code for lazily initializing zygotes. That functionality isn't
currently needed (and could be implemented differently if necessary), so it
seems fine to eliminate the indirection.

I'd be surprised if it makes a difference though: excepting memory corruption,
the only possible ZygoteHandle* values were nullptr and
&GetGenericZygote::zygote.

https://codereview.chromium.org/2850903002/diff/1/content/browser/zygote_host...
File content/browser/zygote_host/zygote_handle_linux.cc (right):

https://codereview.chromium.org/2850903002/diff/1/content/browser/zygote_host...
content/browser/zygote_host/zygote_handle_linux.cc:12: // Intentionally leaked.
On 2017/04/28 20:07:52, James Cook wrote:
> The old code leaks the ZygoteCommunication object. Is that intentional?

By "leaks" do you mean that it's not destroyed when the browser process exits?
If so, I think it's just for simplicity. We need to make sure the
ZygoteCommunication object outlives any delegates that might want to use it.

I don't think it "leaks" in the sense that we created it and lose any reference
to it: it's still accessible via GetGenericZygote.

https://codereview.chromium.org/2850903002/diff/1/content/public/common/sandb...
File content/public/common/sandboxed_process_launcher_delegate.cc (right):

https://codereview.chromium.org/2850903002/diff/1/content/public/common/sandb...
content/public/common/sandboxed_process_launcher_delegate.cc:33: return nullptr;
On 2017/04/28 20:07:52, James Cook wrote:
> I could also make this return GetGenericZygote().

I believe some of the launcher delegate subclasses do not want to use zygotes,
so you'd have to override GetZygote in those to return nullptr instead. I'd
suggest leaving as is.

[James Cook, 2017-04-28 22:16:39]

jamescook@chromium.org changed reviewers:
+ jam@chromium.org

[James Cook, 2017-04-28 22:16:39]

jam, can I get OWNERS?

mdempsky, thanks for the quick review.

[jam, 2017-05-01 15:34:44]

lgtm

[James Cook, 2017-05-01 15:36:29]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2017-05-01 15:36:48]

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-05-01 16:58:39]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1493652989989740, "parent_rev":
"11faf12d156f25a7d8cbe0c87b44d19090e56542", "commit_rev":
"ca8595ada8cf0a185b7abc5d112c7e9dcd66e3bb"}

[commit-bot: I haz the power, 2017-05-01 16:58:50]

Description was changed from

==========
Clean up Linux zygote creation code

The browser code contains partial support for using multiple zygote
processes, which exists to support Flash component updates. However,
at present the browser process only creates a single zygote, on startup,
and uses that for everything. Clean up the code to make that more clear.

* Remove probably-unused lazy zygote creation code.
* Always use ZygoteHandle instead of ZygoteHandle*. ZygoteHandle is always
a pointer under the hood.
* Add CHECKs to validate the assumption there is only one zygote.

This came up while investigating a zygote check failure on Chrome OS.
I'm concerned that the lazy-launch code could be starting a zygote during
browser process shutdown. See crbug.com/692227

BUG=692227,569191
TEST=manually load web pages, extensions and nacl apps to trigger child
process creation, verify they load properly
==========

to

==========
Clean up Linux zygote creation code

The browser code contains partial support for using multiple zygote
processes, which exists to support Flash component updates. However,
at present the browser process only creates a single zygote, on startup,
and uses that for everything. Clean up the code to make that more clear.

* Remove probably-unused lazy zygote creation code.
* Always use ZygoteHandle instead of ZygoteHandle*. ZygoteHandle is always
a pointer under the hood.
* Add CHECKs to validate the assumption there is only one zygote.

This came up while investigating a zygote check failure on Chrome OS.
I'm concerned that the lazy-launch code could be starting a zygote during
browser process shutdown. See crbug.com/692227

BUG=692227,569191
TEST=manually load web pages, extensions and nacl apps to trigger child
process creation, verify they load properly

Review-Url: https://codereview.chromium.org/2850903002
Cr-Commit-Position: refs/heads/master@{#468331}
Committed:
https://chromium.googlesource.com/chromium/src/+/ca8595ada8cf0a185b7abc5d112c...
==========

[commit-bot: I haz the power, 2017-05-01 16:58:50]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/ca8595ada8cf0a185b7abc5d112c...