Initialize FontConfigIPC when zygote is disabled so font fallback works.

Initialize FontConfigIPC when zygote is disabled so font fallback works.

Normally FontConfigIPC is set from zygote_main_linux, but this can't
happen if zygote is disabled, either from the --no-zygote flag or the
--renderer-cmd-prefix flag.

This is a problem because the renderer's font code uses the linux
sandbox IPC logic even if --no-sandbox is set. This code mostly works
but fallback font loading fails because when FontConfigIPC isn't set,
SkFontConfigInterfaceDirect is used instead, and it doesn't know how
to deal with fontconfiginterfaceids. This causes the last resort
fallback font to always be chosen, even when a more appropriate font
is available.

To fix this, simply initialize FontConfigIPC even when zygote is
disabled. This lets the existing code work, and allows fallback fonts
to be loaded.

Manually tested with the test-case in http://crbug.com/712779

BUG=712779
Review-Url: https://codereview.chromium.org/2833973003
Cr-Commit-Position: refs/heads/master@{#472553}
Committed: https://chromium.googlesource.com/chromium/src/+/525974cdba654d1611d169a34397304334797175

[zoeclifford, 2017-04-21 22:54:26]

Description was changed from

==========
Initialize FontConfigIPC when zygote is disabled so font fallback works.

Normally FontConfigIPC is set from zygote_main_linux, but this can't
happen if zygote is disabled, either from the --no-zygote flag or the
--renderer-cmd-prefix flag.

This is a problem because the renderer's font code uses the linux
sandbox IPC logic even if --no-sandbox is set. This code mostly works
but fallback font loading fails because when FontConfigIPC isn't set,
SkFontConfigInterfaceDirect is used instead, and it doesn't know how
to deal with fontconfiginterfaceids.

To fix this, simply initialize FontConfigIPC even when zygote is
disabled. This lets the existing code work, and allows fallback fonts
to be loaded.

Manually tested with the test-case in http://crbug.com/712779

BUG=712779
==========

to

==========
Initialize FontConfigIPC when zygote is disabled so font fallback works.

Normally FontConfigIPC is set from zygote_main_linux, but this can't
happen if zygote is disabled, either from the --no-zygote flag or the
--renderer-cmd-prefix flag.

This is a problem because the renderer's font code uses the linux
sandbox IPC logic even if --no-sandbox is set. This code mostly works
but fallback font loading fails because when FontConfigIPC isn't set,
SkFontConfigInterfaceDirect is used instead, and it doesn't know how
to deal with fontconfiginterfaceids. This causes the last resort
fallback font to always be chosen, even when a more appropriate font
is available.

To fix this, simply initialize FontConfigIPC even when zygote is
disabled. This lets the existing code work, and allows fallback fonts
to be loaded.

Manually tested with the test-case in http://crbug.com/712779

BUG=712779
==========

[Sami, 2017-04-27 18:14:17]

skyostil@chromium.org changed reviewers:
+ dgozman@chromium.org

[Sami, 2017-04-27 18:15:46]

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

[Sami, 2017-04-27 18:15:47]

Dmitry, would you be willing to review this small startup change?

[dgozman, 2017-04-27 19:49:21]

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

[dgozman, 2017-04-27 19:49:21]

I'm definitely not the best reviewer. Over to jam@.

[zoeclifford, 2017-05-04 20:53:18]

Making sure this was actually sent out for review.

[jam, 2017-05-05 00:21:22]

can you add a test for this?

https://codereview.chromium.org/2833973003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2833973003/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1657: // A non-empty
RendererCmdPrefix implies that Zygote is disabled.
this is only when --no-zygote is specified right?

if so, i think no need to run this line. i.e. just keep line 1805 which will
forward it to the renderer

[zoeclifford, 2017-05-05 21:35:48]

On 2017/05/05 00:21:22, jam wrote:
> can you add a test for this?

Any ideas on a good way to test this? The only thing I can think of is to modify
SandboxIPCHandler to be more testable.
It would be nice if there was a way to run functions on the renderer process,
but AFAICT that's not possible since the bug only happens in multi-process mode.
If SandboxIPCHandler seems like a good place to plug in test logic, then I
should have a test ready on Monday.

https://codereview.chromium.org/2833973003/diff/20001/content/browser/rendere...
> content/browser/renderer_host/render_process_host_impl.cc:1657: // A non-empty
> RendererCmdPrefix implies that Zygote is disabled.
> this is only when --no-zygote is specified right?
> 
> if so, i think no need to run this line. i.e. just keep line 1805 which will
> forward it to the renderer

Actually no, --renderer-cmd-prefix on it's own implies that there is no zygote.
See [1][2]. --no-zygote was added fairly recently for Chromium Headless use
cases, I'm not sure if the duplicate behavior was an oversight or not.

[1]
https://cs.chromium.org/chromium/src/content/browser/browser_main_loop.cc?rcl...
[2]
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_pro...

[jam, 2017-05-05 22:25:43]

On 2017/05/05 21:35:48, zoeclifford wrote:
> On 2017/05/05 00:21:22, jam wrote:
> > can you add a test for this?
> 
> Any ideas on a good way to test this? The only thing I can think of is to
modify
> SandboxIPCHandler to be more testable.
> It would be nice if there was a way to run functions on the renderer process,
> but AFAICT that's not possible since the bug only happens in multi-process
mode.
> If SandboxIPCHandler seems like a good place to plug in test logic, then I
> should have a test ready on Monday.

You could write a browser test in content/browser that adds a command line to
disable the zygote, i.e. see
https://cs.chromium.org/search/?q=file:src/content+file:browsertest+setupcomm...

[zoeclifford, 2017-05-05 22:28:50]

On 2017/05/05 22:25:43, jam wrote:
> On 2017/05/05 21:35:48, zoeclifford wrote:
> > On 2017/05/05 00:21:22, jam wrote:
> > > can you add a test for this?
> > 
> > Any ideas on a good way to test this? The only thing I can think of is to
> modify
> > SandboxIPCHandler to be more testable.
> > It would be nice if there was a way to run functions on the renderer
process,
> > but AFAICT that's not possible since the bug only happens in multi-process
> mode.
> > If SandboxIPCHandler seems like a good place to plug in test logic, then I
> > should have a test ready on Monday.
> 
> You could write a browser test in content/browser that adds a command line to
> disable the zygote, i.e. see
>
https://cs.chromium.org/search/?q=file:src/content+file:browsertest+setupcomm...

Ah yeah, I'm planning on doing that, and I've actually done that before.
But disabling zygote isn't the hard part, but testing that the Linux IPC font
fallback code works. That's why I was thinking of making the SandboxIPCHandler
code a little more "mockable" or injectable, so that I can check if FontOpen
messages are actually reaching the browser side.

[jam, 2017-05-09 15:12:38]

On 2017/05/05 22:28:50, zoeclifford wrote:
> On 2017/05/05 22:25:43, jam wrote:
> > On 2017/05/05 21:35:48, zoeclifford wrote:
> > > On 2017/05/05 00:21:22, jam wrote:
> > > > can you add a test for this?
> > > 
> > > Any ideas on a good way to test this? The only thing I can think of is to
> > modify
> > > SandboxIPCHandler to be more testable.
> > > It would be nice if there was a way to run functions on the renderer
> process,
> > > but AFAICT that's not possible since the bug only happens in multi-process
> > mode.
> > > If SandboxIPCHandler seems like a good place to plug in test logic, then I
> > > should have a test ready on Monday.
> > 
> > You could write a browser test in content/browser that adds a command line
to
> > disable the zygote, i.e. see
> >
>
https://cs.chromium.org/search/?q=file:src/content+file:browsertest+setupcomm...
> 
> Ah yeah, I'm planning on doing that, and I've actually done that before.
> But disabling zygote isn't the hard part, but testing that the Linux IPC font
> fallback code works. That's why I was thinking of making the SandboxIPCHandler
> code a little more "mockable" or injectable, so that I can check if FontOpen
> messages are actually reaching the browser side.

Can you write on an html canvas and do pixel checks from JS?

[jam, 2017-05-09 15:12:40]

On 2017/05/05 22:28:50, zoeclifford wrote:
> On 2017/05/05 22:25:43, jam wrote:
> > On 2017/05/05 21:35:48, zoeclifford wrote:
> > > On 2017/05/05 00:21:22, jam wrote:
> > > > can you add a test for this?
> > > 
> > > Any ideas on a good way to test this? The only thing I can think of is to
> > modify
> > > SandboxIPCHandler to be more testable.
> > > It would be nice if there was a way to run functions on the renderer
> process,
> > > but AFAICT that's not possible since the bug only happens in multi-process
> > mode.
> > > If SandboxIPCHandler seems like a good place to plug in test logic, then I
> > > should have a test ready on Monday.
> > 
> > You could write a browser test in content/browser that adds a command line
to
> > disable the zygote, i.e. see
> >
>
https://cs.chromium.org/search/?q=file:src/content+file:browsertest+setupcomm...
> 
> Ah yeah, I'm planning on doing that, and I've actually done that before.
> But disabling zygote isn't the hard part, but testing that the Linux IPC font
> fallback code works. That's why I was thinking of making the SandboxIPCHandler
> code a little more "mockable" or injectable, so that I can check if FontOpen
> messages are actually reaching the browser side.

Can you write on an html canvas and do pixel checks from JS?

[zoeclifford, 2017-05-09 22:05:54]

On 2017/05/09 15:12:40, jam wrote:
> On 2017/05/05 22:28:50, zoeclifford wrote:
> > On 2017/05/05 22:25:43, jam wrote:
> > > On 2017/05/05 21:35:48, zoeclifford wrote:
> > > > On 2017/05/05 00:21:22, jam wrote:
> > > > > can you add a test for this?
> > > > 
> > > > Any ideas on a good way to test this? The only thing I can think of is
to
> > > modify
> > > > SandboxIPCHandler to be more testable.
> > > > It would be nice if there was a way to run functions on the renderer
> > process,
> > > > but AFAICT that's not possible since the bug only happens in
multi-process
> > > mode.
> > > > If SandboxIPCHandler seems like a good place to plug in test logic, then
I
> > > > should have a test ready on Monday.
> > > 
> > > You could write a browser test in content/browser that adds a command line
> to
> > > disable the zygote, i.e. see
> > >
> >
>
https://cs.chromium.org/search/?q=file:src/content+file:browsertest+setupcomm...
> > 
> > Ah yeah, I'm planning on doing that, and I've actually done that before.
> > But disabling zygote isn't the hard part, but testing that the Linux IPC
font
> > fallback code works. That's why I was thinking of making the
SandboxIPCHandler
> > code a little more "mockable" or injectable, so that I can check if FontOpen
> > messages are actually reaching the browser side.
> 
> Can you write on an html canvas and do pixel checks from JS?

It's possible.
Issues are:
1. How to ensure that the fallback font code will always work the same on
different systems
2. How to prevent the test from being sensitive to small changes in painting
logic

#1 can be done by calling into ui/gfx/test/fontconfig_util_linux.h
#2 can probably be done by comparing two canvases against eachother (One with
explicit family text, one with fallback text), instead of comparing one canvas
against a golden copy.

But is there a reason to prefer pixel tests over adding a test observer class to
SandboxIPCHandler to look for FontOpenRequests from test code? This latter
approach still seems more straightforward to me, since I don't want to test that
fonts are drawn in a particular way, but that the right font is chosen to be
drawn.

[jam, 2017-05-10 00:09:13]

On 2017/05/09 22:05:54, zoeclifford wrote:
> On 2017/05/09 15:12:40, jam wrote:
> > On 2017/05/05 22:28:50, zoeclifford wrote:
> > > On 2017/05/05 22:25:43, jam wrote:
> > > > On 2017/05/05 21:35:48, zoeclifford wrote:
> > > > > On 2017/05/05 00:21:22, jam wrote:
> > > > > > can you add a test for this?
> > > > > 
> > > > > Any ideas on a good way to test this? The only thing I can think of is
> to
> > > > modify
> > > > > SandboxIPCHandler to be more testable.
> > > > > It would be nice if there was a way to run functions on the renderer
> > > process,
> > > > > but AFAICT that's not possible since the bug only happens in
> multi-process
> > > > mode.
> > > > > If SandboxIPCHandler seems like a good place to plug in test logic,
then
> I
> > > > > should have a test ready on Monday.
> > > > 
> > > > You could write a browser test in content/browser that adds a command
line
> > to
> > > > disable the zygote, i.e. see
> > > >
> > >
> >
>
https://cs.chromium.org/search/?q=file:src/content+file:browsertest+setupcomm...
> > > 
> > > Ah yeah, I'm planning on doing that, and I've actually done that before.
> > > But disabling zygote isn't the hard part, but testing that the Linux IPC
> font
> > > fallback code works. That's why I was thinking of making the
> SandboxIPCHandler
> > > code a little more "mockable" or injectable, so that I can check if
FontOpen
> > > messages are actually reaching the browser side.
> > 
> > Can you write on an html canvas and do pixel checks from JS?
> 
> It's possible.
> Issues are:
> 1. How to ensure that the fallback font code will always work the same on
> different systems
> 2. How to prevent the test from being sensitive to small changes in painting
> logic
> 
> #1 can be done by calling into ui/gfx/test/fontconfig_util_linux.h
> #2 can probably be done by comparing two canvases against eachother (One with
> explicit family text, one with fallback text), instead of comparing one canvas
> against a golden copy.
> 
> But is there a reason to prefer pixel tests over adding a test observer class
to
> SandboxIPCHandler to look for FontOpenRequests from test code? This latter
> approach still seems more straightforward to me, since I don't want to test
that
> fonts are drawn in a particular way, but that the right font is chosen to be
> drawn.

I defer to the owners of that code how it should be tested :) I just suggested
it as one possible way.

[Sami, 2017-05-10 11:03:47]

skyostil@chromium.org changed reviewers:
+ jln@google.com, jorgelo@chromium.org

[Sami, 2017-05-10 11:05:19]

+linux sandboxing owners

My non-owner opinion is that refactoring SandboxIPCHandler to be more testable
is probably the most practical solution here since writing fast and non-flaky
pixel tests isn't always easy.

[zoeclifford, 2017-05-12 18:22:28]

On 2017/05/10 11:05:19, Sami wrote:
> +linux sandboxing owners
> 
> My non-owner opinion is that refactoring SandboxIPCHandler to be more testable
> is probably the most practical solution here since writing fast and non-flaky
> pixel tests isn't always easy.

Added a browser test.

[Sami, 2017-05-15 11:14:37]

Non-owner lgtm, thanks for adding the test!

https://codereview.chromium.org/2833973003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/sandbox_ipc_linux.cc (right):

https://codereview.chromium.org/2833973003/diff/40001/content/browser/rendere...
content/browser/renderer_host/sandbox_ipc_linux.cc:263: if (g_test_observer)
nit: Please add curly braces

[jam, 2017-05-15 14:00:34]

lgtm

[zoeclifford, 2017-05-15 19:19:48]

On 2017/05/15 11:14:37, Sami wrote:
> Non-owner lgtm, thanks for adding the test!
> 
>
https://codereview.chromium.org/2833973003/diff/40001/content/browser/rendere...
> File content/browser/renderer_host/sandbox_ipc_linux.cc (right):
> 
>
https://codereview.chromium.org/2833973003/diff/40001/content/browser/rendere...
> content/browser/renderer_host/sandbox_ipc_linux.cc:263: if (g_test_observer)
> nit: Please add curly braces

curly braces added. UChar include fixed.

[zoeclifford, 2017-05-15 19:22:19]

zoeclifford@chromium.org changed reviewers:
+ jln@chromium.org
- jln@google.com

[Jorge Lucangeli Obes, 2017-05-16 14:56:05]

sandbox_ipc_linux lgtm

[zoeclifford, 2017-05-16 16:01:28]

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

[commit-bot: I haz the power, 2017-05-16 16:02:54]

Dry run: 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-16 16:17:21]

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

[commit-bot: I haz the power, 2017-05-16 16:17:22]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[zoeclifford, 2017-05-16 19:51:05]

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

[commit-bot: I haz the power, 2017-05-16 19:52:35]

Dry run: 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-16 21:38:47]

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

[commit-bot: I haz the power, 2017-05-16 21:38:48]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[zoeclifford, 2017-05-17 16:48:22]

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

[commit-bot: I haz the power, 2017-05-17 16:49:16]

Dry run: CQ is trying da patch.

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

[zoeclifford, 2017-05-17 17:40:49]

On 2017/05/16 14:56:05, Jorge Lucangeli Obes wrote:
> sandbox_ipc_linux lgtm

I had to modify the test for TSan to be happy about populating test maps from
the IPC thread. If I understand correctly this was because the test thread and
IPC thread accesses were unsequenced in C++ memory model, even though one always
happens before the other since the test waits for renderer process to complete
navigation and the font opening happens before that.

Does this seem like what was happening? Does the updated test LGTM?

[commit-bot: I haz the power, 2017-05-17 18:26:51]

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

[commit-bot: I haz the power, 2017-05-17 18:26:53]

Dry run: This issue passed the CQ dry run.

[Jorge Lucangeli Obes (Google), 2017-05-17 20:16:12]

jorgelo@google.com changed reviewers:
+ jorgelo@google.com

[Jorge Lucangeli Obes (Google), 2017-05-17 20:16:13]

PS6 looks reasonable to me.

lgtm

[Jorge Lucangeli Obes, 2017-05-17 20:16:54]

Sorry, I keep replying from my @google address.

lgtm

[zoeclifford, 2017-05-17 20:17:07]

The CQ bit was checked by zoeclifford@chromium.org

[zoeclifford, 2017-05-17 20:17:07]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2833973003/#ps100001
(title: "Make test TSan clean.")

[Jorge Lucangeli Obes, 2017-05-17 20:17:10]

jorgelo@chromium.org changed reviewers:
- jorgelo@google.com

[commit-bot: I haz the power, 2017-05-17 20:18:39]

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-17 20:31:10]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495052227315010,
"parent_rev": "1fbb68b7dbf4977fce75ae74a8c1683ec26d5474", "commit_rev":
"525974cdba654d1611d169a34397304334797175"}

[commit-bot: I haz the power, 2017-05-17 20:31:21]

Description was changed from

==========
Initialize FontConfigIPC when zygote is disabled so font fallback works.

Normally FontConfigIPC is set from zygote_main_linux, but this can't
happen if zygote is disabled, either from the --no-zygote flag or the
--renderer-cmd-prefix flag.

This is a problem because the renderer's font code uses the linux
sandbox IPC logic even if --no-sandbox is set. This code mostly works
but fallback font loading fails because when FontConfigIPC isn't set,
SkFontConfigInterfaceDirect is used instead, and it doesn't know how
to deal with fontconfiginterfaceids. This causes the last resort
fallback font to always be chosen, even when a more appropriate font
is available.

To fix this, simply initialize FontConfigIPC even when zygote is
disabled. This lets the existing code work, and allows fallback fonts
to be loaded.

Manually tested with the test-case in http://crbug.com/712779

BUG=712779
==========

to

==========
Initialize FontConfigIPC when zygote is disabled so font fallback works.

Normally FontConfigIPC is set from zygote_main_linux, but this can't
happen if zygote is disabled, either from the --no-zygote flag or the
--renderer-cmd-prefix flag.

This is a problem because the renderer's font code uses the linux
sandbox IPC logic even if --no-sandbox is set. This code mostly works
but fallback font loading fails because when FontConfigIPC isn't set,
SkFontConfigInterfaceDirect is used instead, and it doesn't know how
to deal with fontconfiginterfaceids. This causes the last resort
fallback font to always be chosen, even when a more appropriate font
is available.

To fix this, simply initialize FontConfigIPC even when zygote is
disabled. This lets the existing code work, and allows fallback fonts
to be loaded.

Manually tested with the test-case in http://crbug.com/712779

BUG=712779

Review-Url: https://codereview.chromium.org/2833973003
Cr-Commit-Position: refs/heads/master@{#472553}
Committed:
https://chromium.googlesource.com/chromium/src/+/525974cdba654d1611d169a34397...
==========

[commit-bot: I haz the power, 2017-05-17 20:31:23]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/525974cdba654d1611d169a34397...