Moved logic for mapping child process FDs for ICU and V8 into child_process_launcher.cc

Moved logic for mapping child process FDs for ICU and V8 into child_process_launcher.cc

Used to be defined in each app's ContentBrowserClient, but since
content/ is the one that receives the FDs, it makes sense that it should
be the one to send them.

This also removes ChildProcessLauncher::AppendMappedFileCommandLineSwitches
as it is no longer needed.

BUG=394502
Committed: https://crrev.com/228414fc8870f88f11ada7512e88ea6999890f56
Cr-Commit-Position: refs/heads/master@{#334702}

[agrieve, 2015-06-11 14:59:21]

agrieve@chromium.org changed reviewers:
+ jam@chromium.org, jungshik@google.com, rmcilroy@chromium.org,
thestig@chromium.org

[jam, 2015-06-11 16:11:34]

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

[jam, 2015-06-11 16:11:34]

Thanks

+jochen for the v8 bits

https://codereview.chromium.org/1182443003/diff/20001/content/browser/child_p...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/1182443003/diff/20001/content/browser/child_p...
content/browser/child_process_launcher.cc:149: files_to_register->Share(
should be behind OS_ANDROID

btw it would be good to run trybots before sending for review, as it would catch
stuff like this :)

[agrieve, 2015-06-11 17:30:35]

https://codereview.chromium.org/1182443003/diff/20001/content/browser/child_p...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/1182443003/diff/20001/content/browser/child_p...
content/browser/child_process_launcher.cc:149: files_to_register->Share(
On 2015/06/11 16:11:34, jam wrote:
> should be behind OS_ANDROID
> 
> btw it would be good to run trybots before sending for review, as it would
catch
> stuff like this :)

Done (and kicked off a try job, good advice!)

[michaelbai, 2015-06-11 19:13:03]

michaelbai@chromium.org changed reviewers:
+ michaelbai@chromium.org

[michaelbai, 2015-06-11 19:13:04]

Came across this, is it suitable to carry on all GlobalDescriptors?

Register ICU and V8 as GlobalDescriptors, then carry on all GlobalDescriptors to
child process?

[agrieve, 2015-06-11 19:28:14]

On 2015/06/11 19:13:04, michaelbai wrote:
> Came across this, is it suitable to carry on all GlobalDescriptors?
> 
> Register ICU and V8 as GlobalDescriptors, then carry on all GlobalDescriptors
to
> child process?

Maybe. Might want to restrict which descriptors get passed based on the type of
subprocess though? Might make sense to store FDs and regions in
GlobalDescriptors rather than in local statics within icu_util and
v8_initializer. WDYT (anyone)?

[agrieve, 2015-06-12 01:26:44]

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

[commit-bot: I haz the power, 2015-06-12 01:27:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1182443003/160001

[commit-bot: I haz the power, 2015-06-12 01:27:22]

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

[commit-bot: I haz the power, 2015-06-12 01:27:23]

Dry run: No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[jochen (gone - plz use gerrit), 2015-06-12 08:34:36]

https://codereview.chromium.org/1182443003/diff/160001/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/1182443003/diff/160001/content/browser/DEPS#n...
content/browser/DEPS:12: "+gin",
content/browser doesn't pull in gin as a build dependency, no?

[agrieve, 2015-06-12 13:39:16]

https://codereview.chromium.org/1182443003/diff/160001/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/1182443003/diff/160001/content/browser/DEPS#n...
content/browser/DEPS:12: "+gin",
On 2015/06/12 08:34:36, jochen wrote:
> content/browser doesn't pull in gin as a build dependency, no?

It didn't (which is why I had to add this line). Wasn't sure if it was from a
design decision not to, or just that it hadn't used it yet. gin certainly is
used in both app/ and renderer/, but if there is a better spot within content/
to put the FD mapping code, I'm open to suggestions.

[agrieve, 2015-06-12 19:41:25]

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

[commit-bot: I haz the power, 2015-06-12 19:41:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1182443003/200001

[commit-bot: I haz the power, 2015-06-12 19:41:43]

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

[commit-bot: I haz the power, 2015-06-12 19:41:45]

Dry run: No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[agrieve, 2015-06-12 19:42:32]

On 2015/06/12 13:39:16, agrieve wrote:
> https://codereview.chromium.org/1182443003/diff/160001/content/browser/DEPS
> File content/browser/DEPS (right):
> 
>
https://codereview.chromium.org/1182443003/diff/160001/content/browser/DEPS#n...
> content/browser/DEPS:12: "+gin",
> On 2015/06/12 08:34:36, jochen wrote:
> > content/browser doesn't pull in gin as a build dependency, no?
> 
> It didn't (which is why I had to add this line). Wasn't sure if it was from a
> design decision not to, or just that it hadn't used it yet. gin certainly is
> used in both app/ and renderer/, but if there is a better spot within content/
> to put the FD mapping code, I'm open to suggestions.

I believe I've worked through all of the CQ failures (sorry for not doing so
first). No major changes though. PTAL.

[rmcilroy, 2015-06-12 22:09:25]

A couple of driveby comments.

https://codereview.chromium.org/1182443003/diff/200001/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/1182443003/diff/200001/content/browser/DEPS#n...
content/browser/DEPS:12: "+gin",
Probably best to make this as specific as possible (we don't really want gin/v8
being a dependency of browser). Could you make it:
gin/v8_initialization.h

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

https://codereview.chromium.org/1182443003/diff/200001/content/browser/child_...
content/browser/child_process_launcher.cc:151:
&regions[kV8NativesDataDescriptor]));
Please add a DCHECK that the natives_of is valid (I know the function already
asserts, but just in case this changes in the future).

https://codereview.chromium.org/1182443003/diff/200001/content/browser/child_...
content/browser/child_process_launcher.cc:171: *cmd_line, child_process_id,
files_to_register.get());
Nit - I'd move this call above the #ifdef V8_... but that's mostly personal
preference.

https://codereview.chromium.org/1182443003/diff/200001/gin/v8_initializer.cc
File gin/v8_initializer.cc (right):

https://codereview.chromium.org/1182443003/diff/200001/gin/v8_initializer.cc#...
gin/v8_initializer.cc:302: base::PlatformFile
V8Initializer::OpenSnapshotFileForChildProcesses(
How about GetOpenSnapshotFileForChildProcess (to make it clear this doesn't open
the file each time).

[jam, 2015-06-13 00:14:59]

lgtm on my end, deferring to others' outstanding comments.

thanks for cleaning this up

[Lei Zhang, 2015-06-13 00:17:30]

chrome/ lgtm

[agrieve, 2015-06-15 13:38:21]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-06-15 13:38:22]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1182443003/#ps240001
(title: " ")

[commit-bot: I haz the power, 2015-06-15 13:38:37]

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

[commit-bot: I haz the power, 2015-06-15 14:06:11]

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

[commit-bot: I haz the power, 2015-06-15 14:06:13]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[jochen (gone - plz use gerrit), 2015-06-15 14:51:20]

On 2015/06/12 at 13:39:16, agrieve wrote:
> https://codereview.chromium.org/1182443003/diff/160001/content/browser/DEPS
> File content/browser/DEPS (right):
> 
>
https://codereview.chromium.org/1182443003/diff/160001/content/browser/DEPS#n...
> content/browser/DEPS:12: "+gin",
> On 2015/06/12 08:34:36, jochen wrote:
> > content/browser doesn't pull in gin as a build dependency, no?
> 
> It didn't (which is why I had to add this line). Wasn't sure if it was from a
design decision not to, or just that it hadn't used it yet. gin certainly is
used in both app/ and renderer/, but if there is a better spot within content/
to put the FD mapping code, I'm open to suggestions.

you'll need to add a dependency from content/browser (both in
content_browser.gypi and from content/browser/BUILD.gn on gin). Adding the DEPS
rule is not enough

[agrieve, 2015-06-15 15:03:06]

On 2015/06/15 14:51:20, jochen wrote:
> On 2015/06/12 at 13:39:16, agrieve wrote:
> > https://codereview.chromium.org/1182443003/diff/160001/content/browser/DEPS
> > File content/browser/DEPS (right):
> > 
> >
>
https://codereview.chromium.org/1182443003/diff/160001/content/browser/DEPS#n...
> > content/browser/DEPS:12: "+gin",
> > On 2015/06/12 08:34:36, jochen wrote:
> > > content/browser doesn't pull in gin as a build dependency, no?
> > 
> > It didn't (which is why I had to add this line). Wasn't sure if it was from
a
> design decision not to, or just that it hadn't used it yet. gin certainly is
> used in both app/ and renderer/, but if there is a better spot within content/
> to put the FD mapping code, I'm open to suggestions.
> 
> you'll need to add a dependency from content/browser (both in
> content_browser.gypi and from content/browser/BUILD.gn on gin). Adding the
DEPS
> rule is not enough

Done. thanks!

[jochen (gone - plz use gerrit), 2015-06-15 15:21:57]

since chrome/browser already depends on gin (through net_with_v8), this lgtm

[agrieve, 2015-06-16 00:29:57]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-06-16 00:29:58]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
thestig@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1182443003/#ps340001
(title: "winbuild1")

[commit-bot: I haz the power, 2015-06-16 00:31:31]

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

[commit-bot: I haz the power, 2015-06-16 00:44:43]

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

[commit-bot: I haz the power, 2015-06-16 00:44:45]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[agrieve, 2015-06-16 14:46:29]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-06-16 14:46:30]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
jam@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1182443003/#ps360001
(title: "fix ios and win2")

[commit-bot: I haz the power, 2015-06-16 14:46:52]

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

[commit-bot: I haz the power, 2015-06-16 14:57:29]

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

[commit-bot: I haz the power, 2015-06-16 14:57:30]

Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[agrieve, 2015-06-16 16:04:00]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-06-16 16:04:01]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
jam@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1182443003/#ps380001
(title: "fix lin")

[commit-bot: I haz the power, 2015-06-16 16:04:24]

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

[commit-bot: I haz the power, 2015-06-16 18:33:52]

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

[commit-bot: I haz the power, 2015-06-16 18:33:54]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[agrieve, 2015-06-16 18:52:49]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2015-06-16 18:55:06]

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

[commit-bot: I haz the power, 2015-06-16 21:40:06]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2015-06-16 21:41:05]

Patchset 20 (id:??) landed as
https://crrev.com/228414fc8870f88f11ada7512e88ea6999890f56
Cr-Commit-Position: refs/heads/master@{#334702}

[Marijn Kruisselbrink, 2015-06-16 22:26:39]

A revert of this CL (patchset #20 id:380001) has been created in
https://codereview.chromium.org/1187213002/ by mek@chromium.org.

The reason for reverting is: Adds new static initializers in
http://build.chromium.org/p/chromium/buildstatus?builder=Linux%20x64&number=4979

# icu_util.cc cc::VertexShaderQuadAA::VertexShaderQuadAA()
# icu_util.cc base::MemoryMappedFile::MemoryMappedFile()
# icu_util.cc base::i18n::(anonymous namespace)::g_icudtl_region
# icu_util.cc base::i18n::(anonymous namespace)::g_icudtl_mapped_file
# icu_util.cc operator new(unsigned long)
.

[Marijn Kruisselbrink, 2015-06-16 22:27:56]

On 2015/06/16 at 22:26:39, Marijn Kruisselbrink wrote:
> A revert of this CL (patchset #20 id:380001) has been created in
https://codereview.chromium.org/1187213002/ by mek@chromium.org.
> 
> The reason for reverting is: Adds new static initializers in
http://build.chromium.org/p/chromium/buildstatus?builder=Linux%20x64&number=4979
> 
> # icu_util.cc cc::VertexShaderQuadAA::VertexShaderQuadAA()
> # icu_util.cc base::MemoryMappedFile::MemoryMappedFile()
> # icu_util.cc base::i18n::(anonymous namespace)::g_icudtl_region
> # icu_util.cc base::i18n::(anonymous namespace)::g_icudtl_mapped_file
> # icu_util.cc operator new(unsigned long)
> .

And a few in v8_initializer.cc as well:
# v8_initializer.cc cc::VertexShaderQuadAA::VertexShaderQuadAA()
# v8_initializer.cc gin::(anonymous namespace)::g_natives_region
# v8_initializer.cc gin::(anonymous namespace)::g_snapshot_region

[agrieve, 2015-06-17 01:01:42]

On 2015/06/16 22:27:56, Marijn Kruisselbrink wrote:
> On 2015/06/16 at 22:26:39, Marijn Kruisselbrink wrote:
> > A revert of this CL (patchset #20 id:380001) has been created in
> https://codereview.chromium.org/1187213002/ by mailto:mek@chromium.org.
> > 
> > The reason for reverting is: Adds new static initializers in
>
http://build.chromium.org/p/chromium/buildstatus?builder=Linux%20x64&number=4979
> > 
> > # icu_util.cc cc::VertexShaderQuadAA::VertexShaderQuadAA()
> > # icu_util.cc base::MemoryMappedFile::MemoryMappedFile()
> > # icu_util.cc base::i18n::(anonymous namespace)::g_icudtl_region
> > # icu_util.cc base::i18n::(anonymous namespace)::g_icudtl_mapped_file
> > # icu_util.cc operator new(unsigned long)
> > .
> 
> And a few in v8_initializer.cc as well:
> # v8_initializer.cc cc::VertexShaderQuadAA::VertexShaderQuadAA()
> # v8_initializer.cc gin::(anonymous namespace)::g_natives_region
> # v8_initializer.cc gin::(anonymous namespace)::g_snapshot_region

Marijn (or anyone else) - anyone know how I can test that my next attempt won't
break this builder again? I don't see "Linux x64" it in the list of trybots in
the code review tool, and a code search for "sizes.py" doesn't turn up the
script that failed.

[Lei Zhang, 2015-06-17 01:09:06]

On 2015/06/17 01:01:42, agrieve wrote:
> On 2015/06/16 22:27:56, Marijn Kruisselbrink wrote:
> > On 2015/06/16 at 22:26:39, Marijn Kruisselbrink wrote:
> > > A revert of this CL (patchset #20 id:380001) has been created in
> > https://codereview.chromium.org/1187213002/ by mailto:mek@chromium.org.
> > > 
> > > The reason for reverting is: Adds new static initializers in
> >
>
http://build.chromium.org/p/chromium/buildstatus?builder=Linux%20x64&number=4979
> > > 
> > > # icu_util.cc cc::VertexShaderQuadAA::VertexShaderQuadAA()
> > > # icu_util.cc base::MemoryMappedFile::MemoryMappedFile()
> > > # icu_util.cc base::i18n::(anonymous namespace)::g_icudtl_region
> > > # icu_util.cc base::i18n::(anonymous namespace)::g_icudtl_mapped_file
> > > # icu_util.cc operator new(unsigned long)
> > > .
> > 
> > And a few in v8_initializer.cc as well:
> > # v8_initializer.cc cc::VertexShaderQuadAA::VertexShaderQuadAA()
> > # v8_initializer.cc gin::(anonymous namespace)::g_natives_region
> > # v8_initializer.cc gin::(anonymous namespace)::g_snapshot_region
> 
> Marijn (or anyone else) - anyone know how I can test that my next attempt
won't
> break this builder again? I don't see "Linux x64" it in the list of trybots in
> the code review tool, and a code search for "sizes.py" doesn't turn up the
> script that failed.

On Linux, do a release build with GYP_DEFINES="component=static_library", then
use tools/linux/dump-static-initializers.py to print the static initializers.

[agrieve, 2015-06-17 01:13:44]

On 2015/06/17 01:09:06, Lei Zhang wrote:
> On 2015/06/17 01:01:42, agrieve wrote:
> > On 2015/06/16 22:27:56, Marijn Kruisselbrink wrote:
> > > On 2015/06/16 at 22:26:39, Marijn Kruisselbrink wrote:
> > > > A revert of this CL (patchset #20 id:380001) has been created in
> > > https://codereview.chromium.org/1187213002/ by mailto:mek@chromium.org.
> > > > 
> > > > The reason for reverting is: Adds new static initializers in
> > >
> >
>
http://build.chromium.org/p/chromium/buildstatus?builder=Linux%20x64&number=4979
> > > > 
> > > > # icu_util.cc cc::VertexShaderQuadAA::VertexShaderQuadAA()
> > > > # icu_util.cc base::MemoryMappedFile::MemoryMappedFile()
> > > > # icu_util.cc base::i18n::(anonymous namespace)::g_icudtl_region
> > > > # icu_util.cc base::i18n::(anonymous namespace)::g_icudtl_mapped_file
> > > > # icu_util.cc operator new(unsigned long)
> > > > .
> > > 
> > > And a few in v8_initializer.cc as well:
> > > # v8_initializer.cc cc::VertexShaderQuadAA::VertexShaderQuadAA()
> > > # v8_initializer.cc gin::(anonymous namespace)::g_natives_region
> > > # v8_initializer.cc gin::(anonymous namespace)::g_snapshot_region
> > 
> > Marijn (or anyone else) - anyone know how I can test that my next attempt
> won't
> > break this builder again? I don't see "Linux x64" it in the list of trybots
in
> > the code review tool, and a code search for "sizes.py" doesn't turn up the
> > script that failed.
> 
> On Linux, do a release build with GYP_DEFINES="component=static_library", then
> use tools/linux/dump-static-initializers.py to print the static initializers.

Thanks!