Support DirectWrite with sandbox on

Support DirectWrite with sandbox on

This seems to be enough to get DirectWrite working with the sandbox on.
There's two parts:

1. Warmup for three things: the dwrite.dll, creating a font, and
accessing glyph metrics.

2. A renderer sandbox policy allowing readonly fonts directory access.

Manually tested working on Win 8.1, Win 7 Pro SP1 with QFE 2670838,
and Win Home Basic RTM (no SPs or QFEs).

Blink side here: https://codereview.chromium.org/210243004 but can land
independently after this change.

R=brettw@chromium.org, cpu@chromium.org, jam@chromium.org, jschuh@chromium.org, darin@chromium.org
BUG=333029
TEST=Run with --enable-direct-write and http://wikipedia.org/

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259434

[scottmg, 2014-03-21 23:04:05]

https://codereview.chromium.org/209163002/diff/1/content/common/sandbox_win.cc
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/209163002/diff/1/content/common/sandbox_win.c...
content/common/sandbox_win.cc:312: // XXX: This is in the wrong place, it should
only apply to the renderer.
This needs to be made renderer-only rather than in generic.

https://codereview.chromium.org/209163002/diff/1/content/renderer/renderer_ma...
File content/renderer/renderer_main_platform_delegate_win.cc (right):

https://codereview.chromium.org/209163002/diff/1/content/renderer/renderer_ma...
content/renderer/renderer_main_platform_delegate_win.cc:86:
gBeforeSandboxSkFontMgr = SkFontMgr_New_DirectWrite();  // Leaked.
Still working on simplifying this; we can call the DWrite APIs directly and make
this simpler, I think.

[bungeman-chromium, 2014-03-21 23:13:28]

not lgtm

If you feel like responding to everyone who has issues like
https://code.google.com/p/chromium/issues/detail?id=82957 then whatever. But my
design is intentionally not doing this in order to avoid these problems. In fact
my design is intentionally designed to solve this specific issue on Mac.

[scottmg, 2014-03-21 23:17:12]

On 2014/03/21 23:13:28, bungeman2 wrote:
> not lgtm
> 
> If you feel like responding to everyone who has issues like
> https://code.google.com/p/chromium/issues/detail?id=82957 then whatever. But
my
> design is intentionally not doing this in order to avoid these problems. In
fact
> my design is intentionally designed to solve this specific issue on Mac.

Aha, I see. I don't know how to reference non-system installed fonts on Windows
or via HTML/CSS. Is there some test I could use for that on Windows?

[bungeman-chromium, 2014-03-21 23:37:34]

On 2014/03/21 23:17:12, scottmg wrote:
> On 2014/03/21 23:13:28, bungeman2 wrote:
> > not lgtm
> > 
> > If you feel like responding to everyone who has issues like
> > https://code.google.com/p/chromium/issues/detail?id=82957 then whatever. But
> my
> > design is intentionally not doing this in order to avoid these problems. In
> fact
> > my design is intentionally designed to solve this specific issue on Mac.
> 
> Aha, I see. I don't know how to reference non-system installed fonts on
Windows
> or via HTML/CSS. Is there some test I could use for that on Windows?

Here's the other thing (now that I remember). I believe this will incur huge
start up cost (per renderer process), because DirectWrite will block wait for a
connection to the FontCache service (it has some large fixed time out). This
will cause DirectWrite to build a process local cache at great expense. See
https://bugzilla.mozilla.org/show_bug.cgi?id=602792 .

DirectWrite makes no promises about where the font data might come from. Also,
our back-end doesn't enforce that the font collection used is the system font
collection in any event. I'll see about making DirectWrite use a font in a
different directory. However, looks like someone's already done this. See
http://www.marshwiggle.net/regfont/ .

[Will Harris, 2014-03-21 23:55:47]

some sandbox policy location ideas

https://codereview.chromium.org/209163002/diff/1/content/common/sandbox_win.cc
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/209163002/diff/1/content/common/sandbox_win.c...
content/common/sandbox_win.cc:312: // XXX: This is in the wrong place, it should
only apply to the renderer.
On 2014/03/21 23:04:06, scottmg wrote:
> This needs to be made renderer-only rather than in generic.

suggested location below around 657

https://codereview.chromium.org/209163002/diff/1/content/common/sandbox_win.c...
content/common/sandbox_win.cc:657: 
maybe put that policy here

[jschuh, 2014-03-22 00:21:43]

On 2014/03/21 23:37:34, bungeman2 wrote:
> On 2014/03/21 23:17:12, scottmg wrote:
> > On 2014/03/21 23:13:28, bungeman2 wrote:
> > > not lgtm
> > > 
> > > If you feel like responding to everyone who has issues like
> > > https://code.google.com/p/chromium/issues/detail?id=82957 then whatever.
But
> > my
> > > design is intentionally not doing this in order to avoid these problems.
In
> > fact
> > > my design is intentionally designed to solve this specific issue on Mac.
> > 
> > Aha, I see. I don't know how to reference non-system installed fonts on
> Windows
> > or via HTML/CSS. Is there some test I could use for that on Windows?
> 
> Here's the other thing (now that I remember). I believe this will incur huge
> start up cost (per renderer process), because DirectWrite will block wait for
a
> connection to the FontCache service (it has some large fixed time out). This
> will cause DirectWrite to build a process local cache at great expense. See
> https://bugzilla.mozilla.org/show_bug.cgi?id=602792 .
> 
> DirectWrite makes no promises about where the font data might come from. Also,
> our back-end doesn't enforce that the font collection used is the system font
> collection in any event. I'll see about making DirectWrite use a font in a
> different directory. However, looks like someone's already done this. See
> http://www.marshwiggle.net/regfont/ .

Those font registrations don't persist beyond the lifetime of the window
manager. They're intended pretty much for registering custom fonts for use
within your own application. I wouldn't consider that a use case worth
supporting.

[bungeman-chromium, 2014-03-22 02:58:41]

On 2014/03/22 00:21:43, Justin Schuh wrote:
> On 2014/03/21 23:37:34, bungeman2 wrote:
> > On 2014/03/21 23:17:12, scottmg wrote:
> > > On 2014/03/21 23:13:28, bungeman2 wrote:
> > > > not lgtm
> > > > 
> > > > If you feel like responding to everyone who has issues like
> > > > https://code.google.com/p/chromium/issues/detail?id=82957 then whatever.
> But
> > > my
> > > > design is intentionally not doing this in order to avoid these problems.
> In
> > > fact
> > > > my design is intentionally designed to solve this specific issue on Mac.
> > > 
> > > Aha, I see. I don't know how to reference non-system installed fonts on
> > Windows
> > > or via HTML/CSS. Is there some test I could use for that on Windows?
> > 
> > Here's the other thing (now that I remember). I believe this will incur huge
> > start up cost (per renderer process), because DirectWrite will block wait
for
> a
> > connection to the FontCache service (it has some large fixed time out). This
> > will cause DirectWrite to build a process local cache at great expense. See
> > https://bugzilla.mozilla.org/show_bug.cgi?id=602792 .
> > 
> > DirectWrite makes no promises about where the font data might come from.
Also,
> > our back-end doesn't enforce that the font collection used is the system
font
> > collection in any event. I'll see about making DirectWrite use a font in a
> > different directory. However, looks like someone's already done this. See
> > http://www.marshwiggle.net/regfont/ .
> 
> Those font registrations don't persist beyond the lifetime of the window
> manager. They're intended pretty much for registering custom fonts for use
> within your own application. I wouldn't consider that a use case worth
> supporting.

I'm sorry you feel this way, but I was pointing out this one open source project
which does font activation on Windows because that's what a number of people
actually use (with start up scripts). For many people this is the only way for
them to have their own fonts, as they are not administrator and they have not
been given access to the fonts folder. This is particularly not for registering
fonts for one's own application, as GDI already allows one to create a font from
data for a process (which is how we handle web fonts with GDI). On the other
hand, this same basic mechanism is how some rather high end products (for
example Suitcase Fusion 5 for Windows) work. With the current GDI
implementation, fonts activated with these products work. I and many users would
prefer not to see a regression when it isn't necessary. It is easy to be
dismissive of these users since not many on the Chromium team have even heard of
font activation products, but for many web designers these products are how they
test what various kinds of users with various installed fonts might see. On Mac
this issue gets reported every so often by someone savvy enough to realize
what's going on, but most such reporters are now quite weary of Chrome ever
fixing the issue, since the Mac bug is quite old. I would dislike perpetuating
this issue onto other platforms without necessity. In addition, my proposed
mechanism is a large (and possibly necessary) step toward resolving the Mac bug.

[jschuh, 2014-03-22 05:56:40]

On 2014/03/22 02:58:41, bungeman2 wrote:
> On 2014/03/22 00:21:43, Justin Schuh wrote:
> > On 2014/03/21 23:37:34, bungeman2 wrote:
> > > On 2014/03/21 23:17:12, scottmg wrote:
> > > > On 2014/03/21 23:13:28, bungeman2 wrote:
> > > > > not lgtm
> > > > > 
> > > > > If you feel like responding to everyone who has issues like
> > > > > https://code.google.com/p/chromium/issues/detail?id=82957 then
whatever.
> > But
> > > > my
> > > > > design is intentionally not doing this in order to avoid these
problems.
> > In
> > > > fact
> > > > > my design is intentionally designed to solve this specific issue on
Mac.
> > > > 
> > > > Aha, I see. I don't know how to reference non-system installed fonts on
> > > Windows
> > > > or via HTML/CSS. Is there some test I could use for that on Windows?
> > > 
> > > Here's the other thing (now that I remember). I believe this will incur
huge
> > > start up cost (per renderer process), because DirectWrite will block wait
> for
> > a
> > > connection to the FontCache service (it has some large fixed time out).
This
> > > will cause DirectWrite to build a process local cache at great expense.
See
> > > https://bugzilla.mozilla.org/show_bug.cgi?id=602792 .

That bug refers to the scenario where the font manager isn't already running,
which is not the case for individual renderer launches. The approach in this CL
should actually have better performance overall, because the sandbox broker
layer is much lighter and faster than the IPCs in the previous patch.


> > > DirectWrite makes no promises about where the font data might come from.
> Also,
> > > our back-end doesn't enforce that the font collection used is the system
> font
> > > collection in any event. I'll see about making DirectWrite use a font in a
> > > different directory. However, looks like someone's already done this. See
> > > http://www.marshwiggle.net/regfont/ .
> > 
> > Those font registrations don't persist beyond the lifetime of the window
> > manager. They're intended pretty much for registering custom fonts for use
> > within your own application. I wouldn't consider that a use case worth
> > supporting.
> 
> I'm sorry you feel this way, but I was pointing out this one open source
project
> which does font activation on Windows because that's what a number of people
> actually use (with start up scripts). For many people this is the only way for
> them to have their own fonts, as they are not administrator and they have not
> been given access to the fonts folder. This is particularly not for
registering
> fonts for one's own application, as GDI already allows one to create a font
from
> data for a process (which is how we handle web fonts with GDI). On the other
> hand, this same basic mechanism is how some rather high end products (for
> example Suitcase Fusion 5 for Windows) work. With the current GDI
> implementation, fonts activated with these products work. I and many users
would
> prefer not to see a regression when it isn't necessary.

Introducing a security regression in the IPC layer is the more concerning issue,
and the reason why the other patch isn't viable. Whereas the strategy in this CL
is safer, simpler, and more performant. I appreciate your concerns regarding
custom font managers, but that's a narrow corner case that doesn't justify
putting our entire user population at risk. And if we see user feedback on it,
then the approach in this CL makes it straight-forward to detect the additional
font directories and add them via sandbox policy.


> It is easy to be
> dismissive of these users since not many on the Chromium team have even heard
of
> font activation products, but for many web designers these products are how
they
> test what various kinds of users with various installed fonts might see. On
Mac
> this issue gets reported every so often by someone savvy enough to realize
> what's going on, but most such reporters are now quite weary of Chrome ever
> fixing the issue, since the Mac bug is quite old. I would dislike perpetuating
> this issue onto other platforms without necessity. In addition, my proposed
> mechanism is a large (and possibly necessary) step toward resolving the Mac
bug.

I appreciate the goal, but the solution can't introduce this kind of attack
surface outside the sandbox. The current situation on Linux is a known
performance and security issue, and fixing it is a work in progress. We can't
introduce the same problem on all our other platforms.

[bungeman-skia, 2014-03-22 18:59:43]

On 2014/03/22 05:56:40, Justin Schuh wrote:
> On 2014/03/22 02:58:41, bungeman2 wrote:
> > On 2014/03/22 00:21:43, Justin Schuh wrote:
> > > On 2014/03/21 23:37:34, bungeman2 wrote:
> > > > On 2014/03/21 23:17:12, scottmg wrote:
> > > > > On 2014/03/21 23:13:28, bungeman2 wrote:
> > > > > > not lgtm
> > > > > > 
> > > > > > If you feel like responding to everyone who has issues like
> > > > > > https://code.google.com/p/chromium/issues/detail?id=82957 then
> whatever.
> > > But
> > > > > my
> > > > > > design is intentionally not doing this in order to avoid these
> problems.
> > > In
> > > > > fact
> > > > > > my design is intentionally designed to solve this specific issue on
> Mac.
> > > > > 
> > > > > Aha, I see. I don't know how to reference non-system installed fonts
on
> > > > Windows
> > > > > or via HTML/CSS. Is there some test I could use for that on Windows?
> > > > 
> > > > Here's the other thing (now that I remember). I believe this will incur
> huge
> > > > start up cost (per renderer process), because DirectWrite will block
wait
> > for
> > > a
> > > > connection to the FontCache service (it has some large fixed time out).
> This
> > > > will cause DirectWrite to build a process local cache at great expense.
> See
> > > > https://bugzilla.mozilla.org/show_bug.cgi?id=602792 .

I can see that you are unswayed by arguments about correctness and user
concerns, but have you addressed the second (and possibly larger) concern about
significantly slowing renderer start-up on blocking? This was a major issue for
Firefox, and I would not like to see it repeated without need.

I am unsure about your argument about attack surface. As far as I can tell, the
Linux font IPC uses almost exactly this model.

[jschuh, 2014-03-22 19:27:56]

On 2014/03/22 18:59:43, bungeman1 wrote:
> I can see that you are unswayed by arguments about correctness and user
> concerns,

I'm trying very hard to assume positive intent, but the tone and veiled
accusations are really hurting the discussion. I'd ask that you take a moment to
consider how you're framing things before you respond. Because the user's
concerns are clearly the primary consideration here, but those concerns must be
balanced, and one narrow perspective cannot trump everything else.

More importantly, it's still entirely possible to address the issues you've
raised without foisting potentially dangerous code outside the sandbox. I've
already explained how that can be addressed if needed in a follow-up patch on
Windows, and I'm sure there are similar patterns on Mac. It's just a matter of
having a friendly conversation with the engineers who are familiar with how
these components work, and how they interact with the sandbox architectures on a
given platform.


> but have you addressed the second (and possibly larger) concern about
> significantly slowing renderer start-up on blocking? This was a major issue
for
> Firefox, and I would not like to see it repeated without need.

Yes, in my previous response:

"""That bug refers to the scenario where the font manager isn't already running,
which is not the case for individual renderer launches. The approach in this CL
should actually have better performance overall, because the sandbox broker
layer is much lighter and faster than the IPCs in the previous patch."""



> I am unsure about your argument about attack surface. As far as I can tell,
the
> Linux font IPC uses almost exactly this model.

Once again, I addressed this in my previous response:

"""The current situation on Linux is a known
performance and security issue, and fixing it is a work in progress. We can't
introduce the same problem on all our other platforms."""

I can also state unequivocally that the current Linux implementation never would
have passed a security or IPC review today, precisely because of this issue. The
existing implementation is legacy code that was written with far less
understanding of how the sandbox is escaped. Whereas today we have concrete
examples and this is exactly the kind of pattern that is targeted by exploits.

[bungeman-chromium, 2014-03-24 15:08:49]

On 2014/03/22 19:27:56, Justin Schuh wrote:
> On 2014/03/22 18:59:43, bungeman1 wrote:
> > I can see that you are unswayed by arguments about correctness and user
> > concerns,
> 
> I'm trying very hard to assume positive intent, but the tone and veiled
> accusations are really hurting the discussion. I'd ask that you take a moment
to
> consider how you're framing things before you respond. Because the user's
> concerns are clearly the primary consideration here, but those concerns must
be
> balanced, and one narrow perspective cannot trump everything else.
> 
> More importantly, it's still entirely possible to address the issues you've
> raised without foisting potentially dangerous code outside the sandbox. I've
> already explained how that can be addressed if needed in a follow-up patch on
> Windows, and I'm sure there are similar patterns on Mac. It's just a matter of
> having a friendly conversation with the engineers who are familiar with how
> these components work, and how they interact with the sandbox architectures on
a
> given platform.
> 
> 
> > but have you addressed the second (and possibly larger) concern about
> > significantly slowing renderer start-up on blocking? This was a major issue
> for
> > Firefox, and I would not like to see it repeated without need.
> 
> Yes, in my previous response:
> 
> """That bug refers to the scenario where the font manager isn't already
running,
> which is not the case for individual renderer launches. The approach in this
CL
> should actually have better performance overall, because the sandbox broker
> layer is much lighter and faster than the IPCs in the previous patch."""
> 
> 
> 
> > I am unsure about your argument about attack surface. As far as I can tell,
> the
> > Linux font IPC uses almost exactly this model.
> 
> Once again, I addressed this in my previous response:
> 
> """The current situation on Linux is a known
> performance and security issue, and fixing it is a work in progress. We can't
> introduce the same problem on all our other platforms."""
> 
> I can also state unequivocally that the current Linux implementation never
would
> have passed a security or IPC review today, precisely because of this issue.
The
> existing implementation is legacy code that was written with far less
> understanding of how the sandbox is escaped. Whereas today we have concrete
> examples and this is exactly the kind of pattern that is targeted by exploits.

message: On 2014/03/22 19:27:56, Justin Schuh wrote:
> On 2014/03/22 18:59:43, bungeman1 wrote:
> > I can see that you are unswayed by arguments about correctness and user
> > concerns,
> 
> I'm trying very hard to assume positive intent, but the tone and veiled
> accusations are really hurting the discussion. I'd ask that you take a moment
to
> consider how you're framing things before you respond. Because the user's
> concerns are clearly the primary consideration here, but those concerns must
be
> balanced, and one narrow perspective cannot trump everything else.
> 

I find fully supporting the font manager of the platform on which we run to be
more than a 'narrow perspective'. If we have no interest in supporting the
system font manager, then we should make that very clear to the user, as they
would see Chrome not fully supporting the system font manager a major
regression. See crbug.com/108645 for how vocal these sorts of people are, as
well as googlechromesucks.blogspot.com which is completely dedicated to this one
particular issue on Mac (which continues even with the not quite correct
workaround). I am vocal about this and find your easy dismissal of the issue
unfortunate because I would prefer not to see an even larger reaction should
this same issue affect the much greater Windows using population.

> More importantly, it's still entirely possible to address the issues you've
> raised without foisting potentially dangerous code outside the sandbox. I've
> already explained how that can be addressed if needed in a follow-up patch on
> Windows, and I'm sure there are similar patterns on Mac. It's just a matter of
> having a friendly conversation with the engineers who are familiar with how
> these components work, and how they interact with the sandbox architectures on
a
> given platform.
> 
> 
> > but have you addressed the second (and possibly larger) concern about
> > significantly slowing renderer start-up on blocking? This was a major issue
> for
> > Firefox, and I would not like to see it repeated without need.
> 
> Yes, in my previous response:
> 
> """That bug refers to the scenario where the font manager isn't already
running,
> which is not the case for individual renderer launches. The approach in this
CL
> should actually have better performance overall, because the sandbox broker
> layer is much lighter and faster than the IPCs in the previous patch."""
> 
> 
> 
> > I am unsure about your argument about attack surface. As far as I can tell,
> the
> > Linux font IPC uses almost exactly this model.
> 
> Once again, I addressed this in my previous response:
> 
> """The current situation on Linux is a known
> performance and security issue, and fixing it is a work in progress. We can't
> introduce the same problem on all our other platforms."""
> 
> I can also state unequivocally that the current Linux implementation never
would
> have passed a security or IPC review today, precisely because of this issue.
The
> existing implementation is legacy code that was written with far less
> understanding of how the sandbox is escaped. Whereas today we have concrete
> examples and this is exactly the kind of pattern that is targeted by exploits.

In that case it would seem that you would also be interested in removing
src/content/common/mac/font_loader.mm since it is essentially the same thing on
Mac as my proposal and what is currently happening on Linux. I would like to
point out that the GDI IPC PreCacheFont in fact does the exact same thing, only
worse. Effectively the font is loaded with the rights of the browser process *in
kernel space* and this mapped file is then made available to *all renderers*.

In other words, it appears that none of the current font loading mechanisms meet
your criteria. The only way I can see to actually meet your criteria is to not
use the system font loader at all. This appears to be exactly the proposal here,
which is to create a single blessed (by Chrome) set of fonts and send them to
the renderer before lockdown. This is essentially a Chrome built font manager,
not unlike ash/aura being the Chrome built windowing system. Are you working on
such a thing explicitly?

I find your statement that you find the font IPC on Linux broken "and fixing it
is a work in progress" interesting. I have not heard of any work on this, do you
have any further information or design documentation? It appears that this is an
issue for all platforms, not just Linux.

[scottmg, 2014-03-24 23:45:49]

I checked ps3 on Win 8.1, Win 7 Pro SP1 with QFE 2670838, 
and Win Home Basic RTM (i.e. no SP or QFEs).

[jschuh, 2014-03-25 04:03:47]

You need to switch things based on whether we're using DW or not, and shut off
the unused IPCs (probably just don't register the receiver and disable the
sender based on DW status).

https://codereview.chromium.org/209163002/diff/50001/content/common/sandbox_w...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/209163002/diff/50001/content/common/sandbox_w...
content/common/sandbox_win.cc:646: AddDirectory(base::DIR_WINDOWS_FONTS,
This should be scoped to only when DW is enabled.

https://codereview.chromium.org/209163002/diff/50001/content/renderer/rendere...
File content/renderer/renderer_main_platform_delegate_win.cc (right):

https://codereview.chromium.org/209163002/diff/50001/content/renderer/rendere...
content/renderer/renderer_main_platform_delegate_win.cc:53: // before sandbox
lock down to allow Skia access to font data.
Nit: s/font data/the Font Manager service/

https://codereview.chromium.org/209163002/diff/50001/content/renderer/rendere...
content/renderer/renderer_main_platform_delegate_win.cc:55: typedef
decltype(DWriteCreateFactory) * DWriteCreateFactoryProc;
Nit: loose *

https://codereview.chromium.org/209163002/diff/50001/content/renderer/rendere...
content/renderer/renderer_main_platform_delegate_win.cc:92: for (size_t i = 0; i
< ARRAYSIZE(weights); ++i) {
Do you really needed to enumerate through the weights, but just for one font?

https://codereview.chromium.org/209163002/diff/50001/content/renderer/rendere...
content/renderer/renderer_main_platform_delegate_win.cc:136:
SkTypeface_SetEnsureLOGFONTAccessibleProc(SkiaPreCacheFont);
You'll need to disable the GDI setup and both ends of the FontCacheDispatcher
depending on whether DW is enabled.

https://codereview.chromium.org/209163002/diff/50001/content/renderer/rendere...
content/renderer/renderer_main_platform_delegate_win.cc:140:
WarmupDirectWrite();
This should be scoped to run only when DW is enabled.

[scottmg, 2014-03-25 05:43:18]

https://codereview.chromium.org/209163002/diff/50001/content/renderer/rendere...
File content/renderer/renderer_main_platform_delegate_win.cc (right):

https://codereview.chromium.org/209163002/diff/50001/content/renderer/rendere...
content/renderer/renderer_main_platform_delegate_win.cc:53: // before sandbox
lock down to allow Skia access to font data.
On 2014/03/25 04:03:48, Justin Schuh wrote:
> Nit: s/font data/the Font Manager service/

Done.

https://codereview.chromium.org/209163002/diff/50001/content/renderer/rendere...
content/renderer/renderer_main_platform_delegate_win.cc:55: typedef
decltype(DWriteCreateFactory) * DWriteCreateFactoryProc;
On 2014/03/25 04:03:48, Justin Schuh wrote:
> Nit: loose *

clang-format. But Done.

https://codereview.chromium.org/209163002/diff/50001/content/renderer/rendere...
content/renderer/renderer_main_platform_delegate_win.cc:92: for (size_t i = 0; i
< ARRAYSIZE(weights); ++i) {
On 2014/03/25 04:03:48, Justin Schuh wrote:
> Do you really needed to enumerate through the weights, but just for one font?

Oops, thanks, probably not any more. Done. I'll re-confirm on the various OS
levels.

https://codereview.chromium.org/209163002/diff/50001/content/renderer/rendere...
content/renderer/renderer_main_platform_delegate_win.cc:136:
SkTypeface_SetEnsureLOGFONTAccessibleProc(SkiaPreCacheFont);
On 2014/03/25 04:03:48, Justin Schuh wrote:
> You'll need to disable the GDI setup and both ends of the FontCacheDispatcher
> depending on whether DW is enabled.

At the moment, the about:flag isn't conditionalized in any way, and Blink
decides (on a per-renderer basis, in theory) whether it wants to use DW or GDI.
In practice, it just tries to create a DW Skia FontMgr and if that fails it
falls back to GDI.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

So... that's why I didn't do that here.

I guess to only enable one of either GDI or DW here, we could:
- move the equivalent of the Blink fallback code to somewhere in content/common
- update the command line near StartSandboxedProcess (i.e. remove
--enable-direct-write if it's unavailable) 
- make Blink CHECK if it can't create the expected font manager rather than fall
back (as it needs to match this side to work anyway)

Sound OK?

[jschuh, 2014-03-25 12:42:02]

https://codereview.chromium.org/209163002/diff/50001/content/renderer/rendere...
File content/renderer/renderer_main_platform_delegate_win.cc (right):

https://codereview.chromium.org/209163002/diff/50001/content/renderer/rendere...
content/renderer/renderer_main_platform_delegate_win.cc:55: typedef
decltype(DWriteCreateFactory) * DWriteCreateFactoryProc;
On 2014/03/25 05:43:18, scottmg wrote:
> On 2014/03/25 04:03:48, Justin Schuh wrote:
> > Nit: loose *
> 
> clang-format. But Done.

Yep, I've been bitten by clang-format repeatedly on template formatting. You
should file a bug with the Build-Tools flag against nick@chromium.org.

https://codereview.chromium.org/209163002/diff/50001/content/renderer/rendere...
content/renderer/renderer_main_platform_delegate_win.cc:136:
SkTypeface_SetEnsureLOGFONTAccessibleProc(SkiaPreCacheFont);
On 2014/03/25 05:43:18, scottmg wrote:
> On 2014/03/25 04:03:48, Justin Schuh wrote:
> > You'll need to disable the GDI setup and both ends of the
FontCacheDispatcher
> > depending on whether DW is enabled.
> 
> At the moment, the about:flag isn't conditionalized in any way, and Blink
> decides (on a per-renderer basis, in theory) whether it wants to use DW or
GDI.
> In practice, it just tries to create a DW Skia FontMgr and if that fails it
> falls back to GDI.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> So... that's why I didn't do that here.
> 
> I guess to only enable one of either GDI or DW here, we could:
> - move the equivalent of the Blink fallback code to somewhere in
content/common
> - update the command line near StartSandboxedProcess (i.e. remove
> --enable-direct-write if it's unavailable) 
> - make Blink CHECK if it can't create the expected font manager rather than
fall
> back (as it needs to match this side to work anyway)
> 
> Sound OK?
> 

Yeah, that works. As long as the browser makes the decision and the renderer
just does what it's told I'm happy on the security front.

[scottmg, 2014-03-25 17:56:28]

On 2014/03/25 12:42:02, Justin Schuh wrote:
>
https://codereview.chromium.org/209163002/diff/50001/content/renderer/rendere...
> File content/renderer/renderer_main_platform_delegate_win.cc (right):
> 
>
https://codereview.chromium.org/209163002/diff/50001/content/renderer/rendere...
> content/renderer/renderer_main_platform_delegate_win.cc:55: typedef
> decltype(DWriteCreateFactory) * DWriteCreateFactoryProc;
> On 2014/03/25 05:43:18, scottmg wrote:
> > On 2014/03/25 04:03:48, Justin Schuh wrote:
> > > Nit: loose *
> > 
> > clang-format. But Done.
> 
> Yep, I've been bitten by clang-format repeatedly on template formatting. You
> should file a bug with the Build-Tools flag against mailto:nick@chromium.org.
> 
>
https://codereview.chromium.org/209163002/diff/50001/content/renderer/rendere...
> content/renderer/renderer_main_platform_delegate_win.cc:136:
> SkTypeface_SetEnsureLOGFONTAccessibleProc(SkiaPreCacheFont);
> On 2014/03/25 05:43:18, scottmg wrote:
> > On 2014/03/25 04:03:48, Justin Schuh wrote:
> > > You'll need to disable the GDI setup and both ends of the
> FontCacheDispatcher
> > > depending on whether DW is enabled.
> > 
> > At the moment, the about:flag isn't conditionalized in any way, and Blink
> > decides (on a per-renderer basis, in theory) whether it wants to use DW or
> GDI.
> > In practice, it just tries to create a DW Skia FontMgr and if that fails it
> > falls back to GDI.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > So... that's why I didn't do that here.
> > 
> > I guess to only enable one of either GDI or DW here, we could:
> > - move the equivalent of the Blink fallback code to somewhere in
> content/common
> > - update the command line near StartSandboxedProcess (i.e. remove
> > --enable-direct-write if it's unavailable) 
> > - make Blink CHECK if it can't create the expected font manager rather than
> fall
> > back (as it needs to match this side to work anyway)
> > 
> > Sound OK?
> > 
> 
> Yeah, that works. As long as the browser makes the decision and the renderer
> just does what it's told I'm happy on the security front.

Done. Blink side here https://codereview.chromium.org/210243004 but can land
later.

[scottmg, 2014-03-25 18:19:22]

Removed FontCacheDispatcher filter when using DW.

[jschuh, 2014-03-25 18:32:58]

Lgtm on the overall patch and for the security and IPC changes.

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

https://codereview.chromium.org/209163002/diff/190001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:767: // The
FontCacheDispatcher is only required when we're using GDI rendering.
Nit: s/only required/required only/ if you really want to be grammatically
correct. :P

[scottmg, 2014-03-25 19:07:08]

OWNERS:
+brettw for base/
+jam for content/

[brettw, 2014-03-25 19:28:17]

https://codereview.chromium.org/209163002/diff/140003/base/base_paths_win.cc
File base/base_paths_win.cc (right):

https://codereview.chromium.org/209163002/diff/140003/base/base_paths_win.cc#...
base/base_paths_win.cc:195: if (win::GetVersion() < win::VERSION_VISTA)
be consistent w.r.t. base::win or just ::win. I usually fully qualify nested
namespaces for clarity, so add base:: here.

It seems like not working on XP is a significant limitation that should be
mentioned in the header. Or is it reasonable to code %WINDIR%/Fonts for XP?

[scottmg, 2014-03-25 19:45:57]

Thanks

https://codereview.chromium.org/209163002/diff/140003/base/base_paths_win.cc
File base/base_paths_win.cc (right):

https://codereview.chromium.org/209163002/diff/140003/base/base_paths_win.cc#...
base/base_paths_win.cc:195: if (win::GetVersion() < win::VERSION_VISTA)
On 2014/03/25 19:28:18, brettw wrote:
> be consistent w.r.t. base::win or just ::win. I usually fully qualify nested
> namespaces for clarity, so add base:: here.

Was copy-pasta from DIR_APP_SHORTCUTS case. :/

> 
> It seems like not working on XP is a significant limitation that should be
> mentioned in the header. Or is it reasonable to code %WINDIR%/Fonts for XP?

Switched to SHGetFolderPath which works on XP. This patch will only use it on
Win7+ though.

[brettw, 2014-03-25 19:48:32]

base lgtm

[eae, 2014-03-25 20:24:17]

On 2014/03/25 12:42:02, Justin Schuh wrote:
>
https://codereview.chromium.org/209163002/diff/50001/content/renderer/rendere...
> File content/renderer/renderer_main_platform_delegate_win.cc (right):
> 
>
https://codereview.chromium.org/209163002/diff/50001/content/renderer/rendere...
> content/renderer/renderer_main_platform_delegate_win.cc:55: typedef
> decltype(DWriteCreateFactory) * DWriteCreateFactoryProc;
> On 2014/03/25 05:43:18, scottmg wrote:
> > On 2014/03/25 04:03:48, Justin Schuh wrote:
> > > Nit: loose *
> > 
> > clang-format. But Done.
> 
> Yep, I've been bitten by clang-format repeatedly on template formatting. You
> should file a bug with the Build-Tools flag against mailto:nick@chromium.org.
> 
>
https://codereview.chromium.org/209163002/diff/50001/content/renderer/rendere...
> content/renderer/renderer_main_platform_delegate_win.cc:136:
> SkTypeface_SetEnsureLOGFONTAccessibleProc(SkiaPreCacheFont);
> On 2014/03/25 05:43:18, scottmg wrote:
> > On 2014/03/25 04:03:48, Justin Schuh wrote:
> > > You'll need to disable the GDI setup and both ends of the
> FontCacheDispatcher
> > > depending on whether DW is enabled.
> > 
> > At the moment, the about:flag isn't conditionalized in any way, and Blink
> > decides (on a per-renderer basis, in theory) whether it wants to use DW or
> GDI.
> > In practice, it just tries to create a DW Skia FontMgr and if that fails it
> > falls back to GDI.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > So... that's why I didn't do that here.
> > 
> > I guess to only enable one of either GDI or DW here, we could:
> > - move the equivalent of the Blink fallback code to somewhere in
> content/common
> > - update the command line near StartSandboxedProcess (i.e. remove
> > --enable-direct-write if it's unavailable) 
> > - make Blink CHECK if it can't create the expected font manager rather than
> fall
> > back (as it needs to match this side to work anyway)
> > 
> > Sound OK?
> > 
> 
> Yeah, that works. As long as the browser makes the decision and the renderer
> just does what it's told I'm happy on the security front.

If we are now making the decision whether to use directwrite or GDI in the
browser (as opposed to in blink code in the renderer) we also need to change the
way the browser indicates this to blink. Currently a runtime enabled feature is
used that can either be triggered from the browser using --enable-direct-write
or by enabling the runtime enabled feature in blink. The way this patch works
the first method will work while the second will cause the renderer to crash on
startup.

Specifically the DirectWrite runtime enabled feature needs to be removed and
runtime_features.cc needs to be changed to communicate the DW/GDI decision using
some other mechanism.

[cpu_(ooo_6.6-7.5), 2014-03-25 20:47:30]

lgtm

https://codereview.chromium.org/209163002/diff/220002/content/browser/rendere...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/209163002/diff/220002/content/browser/rendere...
content/browser/renderer_host/render_message_filter.cc:1205: if
(!ShouldUseDirectWrite())
add todo here to unify this in the filter.

[jam, 2014-03-25 22:57:26]

lgtm

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

https://codereview.chromium.org/209163002/diff/320008/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:768: if
(!content::ShouldUseDirectWrite())
nit: no "content::"

https://codereview.chromium.org/209163002/diff/320008/content/common/sandbox_...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/209163002/diff/320008/content/common/sandbox_...
content/common/sandbox_win.cc:655: } else if (ShouldUseDirectWrite()) {
nit: it would be clearer to have if (type_str == renderer) {} else {} instead of
this

https://codereview.chromium.org/209163002/diff/320008/content/renderer/render...
File content/renderer/renderer_main_platform_delegate_win.cc (right):

https://codereview.chromium.org/209163002/diff/320008/content/renderer/render...
content/renderer/renderer_main_platform_delegate_win.cc:130: if
(content::ShouldUseDirectWrite()) {
ditto

[scottmg, 2014-03-25 23:38:17]

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

https://codereview.chromium.org/209163002/diff/320008/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:768: if
(!content::ShouldUseDirectWrite())
On 2014/03/25 22:57:26, jam wrote:
> nit: no "content::"

Done.

https://codereview.chromium.org/209163002/diff/320008/content/common/sandbox_...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/209163002/diff/320008/content/common/sandbox_...
content/common/sandbox_win.cc:655: } else if (ShouldUseDirectWrite()) {
On 2014/03/25 22:57:26, jam wrote:
> nit: it would be clearer to have if (type_str == renderer) {} else {} instead
of
> this 

Done.

[scottmg, 2014-03-26 01:23:35]

Thanks for everyone's (extensive!) feedback on this CL. I'm going to land this
as-is so we can get DirectWrite "out there" and start getting feedback on the
various issues we're sure to have with trying to ship to Stable.

We can of course revisit the specific way it's implemented in the future and/or
work on homogenizing the implementation across platforms in the future.

[scottmg, 2014-03-26 01:41:19]

Committed patchset #13 manually as r259434 (presubmit successful).