New issue
Advanced search Search tips

Issue 855193 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Cc:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Bug



Sign in to add a comment

Make it possible for embedders to select the subprocesses supported by their main function

Project Member Reported by p...@chromium.org, Jun 21 2018

Issue description

Some embedders may want to split their subprocesses into multiple binaries in a similar way to how chrome.dll and chrome_child.dll are built on Windows. Right now, achieving this requires building multiple copies of //content/app for each binary. A cleaner way to do it would probably be to find a way to move the references to the subprocesses into the embedder's main function.
 

Comment 1 by p...@chromium.org, Jun 21 2018

Description: Show this description

Comment 2 by p...@chromium.org, Jun 21 2018

The basic way that I think this should work is:
- split ContentMainDelegate into multiple classes, one for each subprocess, each derived from a ProcessRunner base class
- the ProcessRunner for subprocess foo would generally live under //content/foo
- each derived class would implement a Run() function (declared as pure virtual in ProcessRunner) that implements the subprocess, and declare a pure virtual CreateContent*Client() function that embedders would implement
- each embedder creates a vector of ProcessRunners and passes it to ContentMain.

I have a work-in-progress CL here: https://chromium-review.googlesource.com/c/chromium/src/+/1110886

Comment 3 by p...@chromium.org, Jun 27 2018

> each embedder creates a vector of ProcessRunners and passes it to ContentMain.

Having given it some more thought I think we should do something slightly different than this. Since the common case is for ContentMain to support all subprocesses, the way that I'm currently doing things is to put the vector of ProcessRunners in ContentMainDelegate, and in general the ContentMainDelegate derived class would be responsible for filling it in. For ContentMainDelegates that might need to support different subsets of subprocesses (such as ChromeContentMainDelegate), I'm thinking that we would expose functions as part of the //chrome API that would do things like "add the browser subprocess to a ChromeContentMainDelegate" or "add the child subprocesses to a ChromeContentMainDelegate" -- then main functions would call the appropriate //chrome functions.

jam: I'd appreciate any feedback that you might have on the overall approach.

Comment 4 by jam@chromium.org, Jun 28 2018

is there a design doc for what this is about?

Comment 5 by p...@chromium.org, Jun 29 2018

There's some context in this internal doc: https://docs.google.com/document/d/1gattKwIg-8M9FHveJ5VRrnY-58I2GwAtduZSRvM1J9Q/edit
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 29 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5c0a0a104ddbef16788236d87647903460a21f9e

commit 5c0a0a104ddbef16788236d87647903460a21f9e
Author: Peter Collingbourne <pcc@chromium.org>
Date: Fri Jun 29 21:20:21 2018

content: Remove unused declaration.

Bug: 855193
Change-Id: Ic7ca91171215cc7054ca56af3ef6d496c2fcfb55
Reviewed-on: https://chromium-review.googlesource.com/1117862
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Peter Collingbourne <pcc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571634}
[modify] https://crrev.com/5c0a0a104ddbef16788236d87647903460a21f9e/content/app/content_main_runner_impl.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Jun 29 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/15bcab8bd3ac9eccfdc708841d4e945c8860ede0

commit 15bcab8bd3ac9eccfdc708841d4e945c8860ede0
Author: Peter Collingbourne <pcc@chromium.org>
Date: Fri Jun 29 21:38:38 2018

service_manager: Only start the service manager thread in the browser process.

The browser process is the only embedder process that needs a
service manager thread.

With this change we avoid spinning up an unnecessary I/O thread (and
presumably, once issue 729596 is fixed, a service manager) in
processes that are neither subprocesses nor the browser process,
such as the cloud print service process.

Bug: 855193
Change-Id: Iffae3405f6dd541ce898ac2033c9c10daefd0031
Reviewed-on: https://chromium-review.googlesource.com/1117473
Commit-Queue: Peter Collingbourne <pcc@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571640}
[modify] https://crrev.com/15bcab8bd3ac9eccfdc708841d4e945c8860ede0/services/service_manager/embedder/main.cc

Okay, https://chromium-review.googlesource.com/c/chromium/src/+/1110886 is now pretty much what I think the end state should look like. On Windows chrome.dll+chrome_child.dll sizes increase by 5KB and on Android libmonochrome.so 32+64 bit sizes shrink by 2KB (perhaps because with this change the BrowserMain function is statically unreachable on Android -- previously it was dynamically unreachable).

Should there be a more detailed design doc or is the CL enough? Assuming there are no objections I'll start splitting it up into smaller pieces and upstreaming it.
Project Member

Comment 9 by bugdroid1@chromium.org, Aug 1

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d9116474737152478e8183611c01c6ca732dec5c

commit d9116474737152478e8183611c01c6ca732dec5c
Author: Peter Collingbourne <pcc@chromium.org>
Date: Wed Aug 01 16:31:12 2018

content: Remove the "empty" content clients.

These don't appear to be necessary: almost all clients return non-null
pointers from their CreateContent*Client functions, so we can use the
content clients directly. The one exception is the chromecast utility
client whose use of that functionality seems unintentional.

Bug: 855193
Change-Id: I9b835de11dfe37799b30bba7921513438a0d5a9e
Reviewed-on: https://chromium-review.googlesource.com/1157659
Reviewed-by: Luke Halliwell <halliwell@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Peter Collingbourne <pcc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579831}
[modify] https://crrev.com/d9116474737152478e8183611c01c6ca732dec5c/chromecast/utility/cast_content_utility_client_simple.cc
[modify] https://crrev.com/d9116474737152478e8183611c01c6ca732dec5c/content/app/content_main_runner_impl.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Aug 1

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/24c818c9a9915bd0350d2149c3dc367694d89303

commit 24c818c9a9915bd0350d2149c3dc367694d89303
Author: Peter Collingbourne <pcc@chromium.org>
Date: Wed Aug 01 20:38:36 2018

extensions: Remove virtual functions that are never overridden.

Bug: 855193
Change-Id: I2e30bd2e6188a7695b469955c896dd5900fb1793
Reviewed-on: https://chromium-review.googlesource.com/1157545
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Commit-Queue: Peter Collingbourne <pcc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579924}
[modify] https://crrev.com/24c818c9a9915bd0350d2149c3dc367694d89303/extensions/shell/app/shell_main_delegate.cc
[modify] https://crrev.com/24c818c9a9915bd0350d2149c3dc367694d89303/extensions/shell/app/shell_main_delegate.h

Project Member

Comment 11 by bugdroid1@chromium.org, Aug 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/381beab379e4243ece5610862be54367b2ed4dd6

commit 381beab379e4243ece5610862be54367b2ed4dd6
Author: Peter Collingbourne <pcc@chromium.org>
Date: Thu Aug 02 19:42:51 2018

chrome: Call SetUpBundleOverrides from ChromeMain.

It looks like we always need to call SetUpBundleOverrides no matter
what the process type is, so we can just call it from ChromeMain. This
will save us from needing to call this function from browser-specific
code in a later change.

While here, also remove a dead declaration.

Bug: 855193
Change-Id: I449d92ef9c7d80d066d4cfefb3015014bd407cc1
Reviewed-on: https://chromium-review.googlesource.com/1159335
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580285}
[modify] https://crrev.com/381beab379e4243ece5610862be54367b2ed4dd6/chrome/app/chrome_main.cc
[modify] https://crrev.com/381beab379e4243ece5610862be54367b2ed4dd6/chrome/app/chrome_main_delegate.cc
[modify] https://crrev.com/381beab379e4243ece5610862be54367b2ed4dd6/chrome/app/chrome_main_mac.h

Project Member

Comment 12 by bugdroid1@chromium.org, Aug 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7a07960a925b2b0c2e8dc5e9b49f349fb5bd3e7a

commit 7a07960a925b2b0c2e8dc5e9b49f349fb5bd3e7a
Author: Kunihiko Sakamoto <ksakamoto@chromium.org>
Date: Fri Aug 03 05:19:12 2018

Revert "chrome: Call SetUpBundleOverrides from ChromeMain."

This reverts commit 381beab379e4243ece5610862be54367b2ed4dd6.

Reason for revert: Suspected of breaking browser_tests on Mac. crbug.com/870579

Original change's description:
> chrome: Call SetUpBundleOverrides from ChromeMain.
> 
> It looks like we always need to call SetUpBundleOverrides no matter
> what the process type is, so we can just call it from ChromeMain. This
> will save us from needing to call this function from browser-specific
> code in a later change.
> 
> While here, also remove a dead declaration.
> 
> Bug: 855193
> Change-Id: I449d92ef9c7d80d066d4cfefb3015014bd407cc1
> Reviewed-on: https://chromium-review.googlesource.com/1159335
> Reviewed-by: Robert Sesek <rsesek@chromium.org>
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Commit-Queue: Avi Drissman <avi@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#580285}

TBR=avi@chromium.org,pcc@chromium.org,rsesek@chromium.org,mark@chromium.org

Change-Id: I32f905ff8f82306a85fbc9798652b8c99aa46009
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 855193,870579
Reviewed-on: https://chromium-review.googlesource.com/1161722
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580456}
[modify] https://crrev.com/7a07960a925b2b0c2e8dc5e9b49f349fb5bd3e7a/chrome/app/chrome_main.cc
[modify] https://crrev.com/7a07960a925b2b0c2e8dc5e9b49f349fb5bd3e7a/chrome/app/chrome_main_delegate.cc
[modify] https://crrev.com/7a07960a925b2b0c2e8dc5e9b49f349fb5bd3e7a/chrome/app/chrome_main_mac.h

Project Member

Comment 13 by bugdroid1@chromium.org, Aug 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/65ef9c4596407c5a063d4adae8baf6bc67aee131

commit 65ef9c4596407c5a063d4adae8baf6bc67aee131
Author: Peter Collingbourne <pcc@chromium.org>
Date: Fri Aug 03 16:54:16 2018

content: Move function that initializes field trials to child.

A future change will move the calls to this function into the
individual subprocess implementations.

Also simplify the API by leaking the field trial list instead
of requiring the caller to keep it on the stack. This
is similar to what we do in the browser process (see
ChromeBrowserMainParts::SetupFieldTrials).

Bug: 855193
Change-Id: Idf5fbd18d8a6b6f5831d7d5ef63293c22d60d127
Reviewed-on: https://chromium-review.googlesource.com/1159740
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Peter Collingbourne <pcc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580572}
[modify] https://crrev.com/65ef9c4596407c5a063d4adae8baf6bc67aee131/content/app/content_main_runner_impl.cc
[modify] https://crrev.com/65ef9c4596407c5a063d4adae8baf6bc67aee131/content/child/BUILD.gn
[add] https://crrev.com/65ef9c4596407c5a063d4adae8baf6bc67aee131/content/child/field_trial.cc
[add] https://crrev.com/65ef9c4596407c5a063d4adae8baf6bc67aee131/content/child/field_trial.h

Project Member

Comment 14 by bugdroid1@chromium.org, Aug 7

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ddeb4c0cccc2d8154c98f730808c95dde006e4ff

commit ddeb4c0cccc2d8154c98f730808c95dde006e4ff
Author: Peter Collingbourne <pcc@chromium.org>
Date: Tue Aug 07 21:29:30 2018

content: Inline MachBroker::ChildSendTaskPortToParent into callers.

This function lives in //content/browser despite being intended to
be used by child processes. The function body is trivial and can be
inlined into every caller. This lets us remove one dependency from
app to browser.

Bug: 855193
Change-Id: Ia9558b4298275a5638500ebcd65361073be838b9
Reviewed-on: https://chromium-review.googlesource.com/1159394
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Peter Collingbourne <pcc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581342}
[modify] https://crrev.com/ddeb4c0cccc2d8154c98f730808c95dde006e4ff/content/app/content_main_runner_impl.cc
[modify] https://crrev.com/ddeb4c0cccc2d8154c98f730808c95dde006e4ff/content/browser/mach_broker_mac.mm
[modify] https://crrev.com/ddeb4c0cccc2d8154c98f730808c95dde006e4ff/content/browser/mach_broker_mac_unittest.cc
[modify] https://crrev.com/ddeb4c0cccc2d8154c98f730808c95dde006e4ff/content/common/content_constants_internal.cc
[modify] https://crrev.com/ddeb4c0cccc2d8154c98f730808c95dde006e4ff/content/common/content_constants_internal.h

Project Member

Comment 15 by bugdroid1@chromium.org, Aug 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/29ee01681f295842fec78c0eb5db04b7a1b56270

commit 29ee01681f295842fec78c0eb5db04b7a1b56270
Author: Peter Collingbourne <pcc@chromium.org>
Date: Fri Aug 10 01:41:27 2018

extensions: De-duplicate ShellCrApplication initialization.

This will allow ShellMainDelegate and TestShellMainDelegate to share
an implementation of the browser subprocess later.

Bug: 855193
Change-Id: Id4ef662bd96b606f19b4ecedda15a8a1bbed979b
Reviewed-on: https://chromium-review.googlesource.com/1168647
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Commit-Queue: Peter Collingbourne <pcc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582003}
[modify] https://crrev.com/29ee01681f295842fec78c0eb5db04b7a1b56270/extensions/shell/BUILD.gn
[modify] https://crrev.com/29ee01681f295842fec78c0eb5db04b7a1b56270/extensions/shell/app/DEPS
[modify] https://crrev.com/29ee01681f295842fec78c0eb5db04b7a1b56270/extensions/shell/app/shell_main_delegate.h
[rename] https://crrev.com/29ee01681f295842fec78c0eb5db04b7a1b56270/extensions/shell/app/shell_main_delegate_mac.mm
[modify] https://crrev.com/29ee01681f295842fec78c0eb5db04b7a1b56270/extensions/shell/browser/DEPS
[modify] https://crrev.com/29ee01681f295842fec78c0eb5db04b7a1b56270/extensions/shell/browser/shell_browser_main_parts.cc
[delete] https://crrev.com/8b8659b15ab630d1e92d20a97b7d3c2d24e295e6/extensions/shell/browser/shell_browser_main_parts_mac.h
[delete] https://crrev.com/8b8659b15ab630d1e92d20a97b7d3c2d24e295e6/extensions/shell/browser/shell_browser_main_parts_mac.mm
[modify] https://crrev.com/29ee01681f295842fec78c0eb5db04b7a1b56270/extensions/shell/test/test_shell_main_delegate.h

Sign in to add a comment