IPC interface for font management.

IPC interface for font management.

This is motivated by DirectWrite, but it also designed with the idea of moving the FontConfig and CoreText IPC into this interface.

Specific existing bugs this aims to avoid are 285303 and 82957.

This depends on Skia CL https://codereview.chromium.org/206683002/ . To see the changes in Blink, https://codereview.chromium.org/206693003 is required.

BUG=333029

[brettw, 2014-03-11 21:38:15]

I don't see any high-level problems. I noted some style nits

https://codereview.chromium.org/132113015/diff/80001/content/browser/renderer...
File content/browser/renderer_host/fontmgr_message_filter.cc (right):

https://codereview.chromium.org/132113015/diff/80001/content/browser/renderer...
content/browser/renderer_host/fontmgr_message_filter.cc:27: ,
fm(skia::AdoptRef(fontManager))
Comma on prev line.

https://codereview.chromium.org/132113015/diff/80001/content/browser/renderer...
content/browser/renderer_host/fontmgr_message_filter.cc:74:
skia::RefPtr<SkRemotableFontIdentitySet>* styles)
The two args should be aligned horizontally. So indent them both to 4 spaces.

https://codereview.chromium.org/132113015/diff/80001/content/browser/renderer...
content/browser/renderer_host/fontmgr_message_filter.cc:75: {
Fix { placement for this entire patch.

https://codereview.chromium.org/132113015/diff/80001/content/browser/renderer...
content/browser/renderer_host/fontmgr_message_filter.cc:88: SkString familyName,
I'd use Chrome-style for everything that you can, so family_name here, other
args the same.

https://codereview.chromium.org/132113015/diff/80001/content/browser/renderer...
content/browser/renderer_host/fontmgr_message_filter.cc:94: *identity =
fm->matchNameStyleCharacter(familyName.c_str(),
It's not clear why we need to serialize as SkString in this patch when we just
get C-strings out the other end. Can we send std::strings instead and skip the
extra ParamTraits for SkString?

https://codereview.chromium.org/132113015/diff/80001/content/browser/renderer...
File content/browser/renderer_host/fontmgr_message_filter.h (right):

https://codereview.chromium.org/132113015/diff/80001/content/browser/renderer...
content/browser/renderer_host/fontmgr_message_filter.h:36: 
I usually don't add blank lines in between IPC message handler declarations
since they never have comments and its nice to treat them as a unit.

https://codereview.chromium.org/132113015/diff/80001/content/browser/renderer...
content/browser/renderer_host/fontmgr_message_filter.h:38: int familyIndex,
Args should use Chrome-style throughout (family_index)

https://codereview.chromium.org/132113015/diff/80001/content/browser/renderer...
content/browser/renderer_host/fontmgr_message_filter.h:47: SkFontStyle,
We usually name these args.

https://codereview.chromium.org/132113015/diff/80001/content/browser/renderer...
content/browser/renderer_host/fontmgr_message_filter.h:59:
DISALLOW_COPY_AND_ASSIGN(FontMgrMessageFilter);
This should be last.

https://codereview.chromium.org/132113015/diff/80001/content/common/fontmgr_m...
File content/common/fontmgr_messages.cc (right):

https://codereview.chromium.org/132113015/diff/80001/content/common/fontmgr_m...
content/common/fontmgr_messages.cc:106: m->WriteData(NULL, 0);
You have some 4-space indents floating around.

https://codereview.chromium.org/132113015/diff/80001/content/common/fontmgr_p...
File content/common/fontmgr_proxy.h (right):

https://codereview.chromium.org/132113015/diff/80001/content/common/fontmgr_p...
content/common/fontmgr_proxy.h:5: #ifndef CONTENT_COMMON_FONTMGR_PROXY_H
Can this file be in content/renderer? I think if we need it shared between
different child process types, then it goes in content/child. content/common is
stuff stared between browser and child processes, which I don't think this is.

https://codereview.chromium.org/132113015/diff/80001/content/common/fontmgr_p...
content/common/fontmgr_proxy.h:26: virtual SkRemotableFontIdentitySet*
getIndexIdentities(int index) const OVERRIDE;
Long lines (in this case I'd normally just wrap the arg on the next line.

https://codereview.chromium.org/132113015/diff/80001/content/common/fontmgr_p...
content/common/fontmgr_proxy.h:40: typedef SkRemotableFontMgr INHERITED;
I'm not sure what this is for.

https://codereview.chromium.org/132113015/diff/80001/skia/ext/fontmgr_default...
File skia/ext/fontmgr_default_ipc.cc (right):

https://codereview.chromium.org/132113015/diff/80001/skia/ext/fontmgr_default...
skia/ext/fontmgr_default_ipc.cc:9: static SkFontMgr* gSkFontMgr = NULL;
Can this be in an anon namespace to avoid exporting it from this file? In that
case, name it just font_mgr or something. Otherwise, globals are usually named
like g_skia_font_mgr.

https://codereview.chromium.org/132113015/diff/80001/skia/skia_library.gypi
File skia/skia_library.gypi (right):

https://codereview.chromium.org/132113015/diff/80001/skia/skia_library.gypi#n...
skia/skia_library.gypi:52:
#'../third_party/skia/src/ports/SkFontMgr_default_gdi.cpp',
Is the intention not to use this any more? Then I'd just delete it. We don't
want random commented-out files in these lists.

https://codereview.chromium.org/132113015/diff/80001/skia/skia_library.gypi#n...
skia/skia_library.gypi:109: 
Just one blank

[bungeman-chromium, 2014-03-13 22:33:31]

Cleaned up style and usage, most comments addressed (Will investigate need for
SkStrings).

https://codereview.chromium.org/132113015/diff/80001/content/browser/renderer...
File content/browser/renderer_host/fontmgr_message_filter.cc (right):

https://codereview.chromium.org/132113015/diff/80001/content/browser/renderer...
content/browser/renderer_host/fontmgr_message_filter.cc:27: ,
fm(skia::AdoptRef(fontManager))
On 2014/03/11 21:38:15, brettw wrote:
> Comma on prev line.

Done.

https://codereview.chromium.org/132113015/diff/80001/content/browser/renderer...
content/browser/renderer_host/fontmgr_message_filter.cc:74:
skia::RefPtr<SkRemotableFontIdentitySet>* styles)
On 2014/03/11 21:38:15, brettw wrote:
> The two args should be aligned horizontally. So indent them both to 4 spaces.

Done.

https://codereview.chromium.org/132113015/diff/80001/content/browser/renderer...
content/browser/renderer_host/fontmgr_message_filter.cc:75: {
On 2014/03/11 21:38:15, brettw wrote:
> Fix { placement for this entire patch.

Done.

https://codereview.chromium.org/132113015/diff/80001/content/browser/renderer...
content/browser/renderer_host/fontmgr_message_filter.cc:88: SkString familyName,
On 2014/03/11 21:38:15, brettw wrote:
> I'd use Chrome-style for everything that you can, so family_name here, other
> args the same.

Done.

https://codereview.chromium.org/132113015/diff/80001/content/browser/renderer...
content/browser/renderer_host/fontmgr_message_filter.cc:94: *identity =
fm->matchNameStyleCharacter(familyName.c_str(),
On 2014/03/11 21:38:15, brettw wrote:
> It's not clear why we need to serialize as SkString in this patch when we just
> get C-strings out the other end. Can we send std::strings instead and skip the
> extra ParamTraits for SkString?

Maybe, it's this way currently because I was changing everything every time I
learned something new. Once I know if the interface should be SkString or const
char[] I'll make this consistent. I think at some point there were more
SkStrings (used std::vector<SkString> instead of SkDataTable, there was a
SkString in the SkFontIdentity, etc). Maybe now we can get away without.

https://codereview.chromium.org/132113015/diff/80001/content/browser/renderer...
File content/browser/renderer_host/fontmgr_message_filter.h (right):

https://codereview.chromium.org/132113015/diff/80001/content/browser/renderer...
content/browser/renderer_host/fontmgr_message_filter.h:36: 
On 2014/03/11 21:38:15, brettw wrote:
> I usually don't add blank lines in between IPC message handler declarations
> since they never have comments and its nice to treat them as a unit.

Since almost all of these are multi-line, I find it easier to read them with a
bit of white space between them, rather than having an unbroken run of param
names down the middle. I could scrunch them up a bit if you feel strongly about
it.

https://codereview.chromium.org/132113015/diff/80001/content/browser/renderer...
content/browser/renderer_host/fontmgr_message_filter.h:38: int familyIndex,
On 2014/03/11 21:38:15, brettw wrote:
> Args should use Chrome-style throughout (family_index)

Done.

https://codereview.chromium.org/132113015/diff/80001/content/browser/renderer...
content/browser/renderer_host/fontmgr_message_filter.h:47: SkFontStyle,
On 2014/03/11 21:38:15, brettw wrote:
> We usually name these args.

Done. Arg, I always get confused because Blink explicitly doesn't want names for
these sort of args.

https://codereview.chromium.org/132113015/diff/80001/content/browser/renderer...
content/browser/renderer_host/fontmgr_message_filter.h:59:
DISALLOW_COPY_AND_ASSIGN(FontMgrMessageFilter);
On 2014/03/11 21:38:15, brettw wrote:
> This should be last.

Done.

https://codereview.chromium.org/132113015/diff/80001/content/common/fontmgr_m...
File content/common/fontmgr_messages.cc (right):

https://codereview.chromium.org/132113015/diff/80001/content/common/fontmgr_m...
content/common/fontmgr_messages.cc:106: m->WriteData(NULL, 0);
On 2014/03/11 21:38:15, brettw wrote:
> You have some 4-space indents floating around.

Done.

https://codereview.chromium.org/132113015/diff/80001/content/common/fontmgr_p...
File content/common/fontmgr_proxy.h (right):

https://codereview.chromium.org/132113015/diff/80001/content/common/fontmgr_p...
content/common/fontmgr_proxy.h:5: #ifndef CONTENT_COMMON_FONTMGR_PROXY_H
On 2014/03/11 21:38:15, brettw wrote:
> Can this file be in content/renderer? I think if we need it shared between
> different child process types, then it goes in content/child. content/common
is
> stuff stared between browser and child processes, which I don't think this is.

Done. Hmmm... yes. As of right now the only user will be the renderer. If we
need to change that later, we can.

https://codereview.chromium.org/132113015/diff/80001/content/common/fontmgr_p...
content/common/fontmgr_proxy.h:26: virtual SkRemotableFontIdentitySet*
getIndexIdentities(int index) const OVERRIDE;
On 2014/03/11 21:38:15, brettw wrote:
> Long lines (in this case I'd normally just wrap the arg on the next line.

Done.

https://codereview.chromium.org/132113015/diff/80001/content/common/fontmgr_p...
content/common/fontmgr_proxy.h:40: typedef SkRemotableFontMgr INHERITED;
On 2014/03/11 21:38:15, brettw wrote:
> I'm not sure what this is for.

Done. Left-over from the initial implementation. Skia has a debug ref-counting
setup where this is used, but this isn't in Skia, so doesn't need it. Removed.

https://codereview.chromium.org/132113015/diff/80001/skia/ext/fontmgr_default...
File skia/ext/fontmgr_default_ipc.cc (right):

https://codereview.chromium.org/132113015/diff/80001/skia/ext/fontmgr_default...
skia/ext/fontmgr_default_ipc.cc:9: static SkFontMgr* gSkFontMgr = NULL;
On 2014/03/11 21:38:15, brettw wrote:
> Can this be in an anon namespace to avoid exporting it from this file? In that
> case, name it just font_mgr or something. Otherwise, globals are usually named
> like g_skia_font_mgr.

Done. This (and the rest of the machinery around this) needs to be thought
through a little more in any case.

https://codereview.chromium.org/132113015/diff/80001/skia/skia_library.gypi
File skia/skia_library.gypi (right):

https://codereview.chromium.org/132113015/diff/80001/skia/skia_library.gypi#n...
skia/skia_library.gypi:52:
#'../third_party/skia/src/ports/SkFontMgr_default_gdi.cpp',
On 2014/03/11 21:38:15, brettw wrote:
> Is the intention not to use this any more? Then I'd just delete it. We don't
> want random commented-out files in these lists.

Very true, it's like this mostly for dev. I don't see how this particular file
will ever end up in the new world, so yes, it will be dropped.

https://codereview.chromium.org/132113015/diff/80001/skia/skia_library.gypi#n...
skia/skia_library.gypi:109: 
On 2014/03/11 21:38:15, brettw wrote:
> Just one blank

Done. This is also somewhat WIP, so these will be sorted into the main list
before final.

[bungeman-chromium, 2014-03-20 20:24:26]

PS5 addresses the issue with SkString (by removing it) now that the Skia side is
mostly nailed down. It also cleans up the build files now that the locations of
the files has stabilized. PTAL.

[jschuh, 2014-03-20 22:13:01]

On 2014/03/20 20:24:26, bungeman2 wrote:
> PS5 addresses the issue with SkString (by removing it) now that the Skia side
is
> mostly nailed down. It also cleans up the build files now that the locations
of
> the files has stabilized. PTAL.

More broadly, using Skia primitives over the IPC layer is a bad idea if it can
be at all avoided. Chrome's baseline primitives have all been audited for buffer
management, information leakage, and other potential vulnerabilities. That's
going to incur a large security review burden now, and a continuous one for any
changes moving forward. Instead, could you use Chrome's primitives at the IPC
layer, unless the alternative is genuinely too onerous?

[bungeman-chromium, 2014-03-21 15:18:03]

On 2014/03/20 22:13:01, Justin Schuh wrote:
> On 2014/03/20 20:24:26, bungeman2 wrote:
> > PS5 addresses the issue with SkString (by removing it) now that the Skia
side
> is
> > mostly nailed down. It also cleans up the build files now that the locations
> of
> > the files has stabilized. PTAL.
> 
> More broadly, using Skia primitives over the IPC layer is a bad idea if it can
> be at all avoided. Chrome's baseline primitives have all been audited for
buffer
> management, information leakage, and other potential vulnerabilities. That's
> going to incur a large security review burden now, and a continuous one for
any
> changes moving forward. Instead, could you use Chrome's primitives at the IPC
> layer, unless the alternative is genuinely too onerous?

Actually, I believe there is an effort to make *all* Skia 'flattenables' work
with IPC. (This has to do with piping drawings to the gpu process.) See the
existing ParamTraits<SkBitmap> and especially
ParamTraits<skia::RefPtr<SkImageFilter> > which is something of an early start
at this. So it is likely that the number of Skia types in IPC is going to grow
to include all of these types anyway.

Second, I considered a design in which the data in the Skia types was copied
into std::vectors and std::strings. All this really does is move all of the code
from fontmgr_messages.cc into fontmgr_proxy.cc and fontmgr_message_filter.cc and
makes an extra copy of everything (rather wastefully). Essentially, this simply
means serializing twice. This is particularly bad for passing the font data. The
most popular CJK fonts on Windows, for example SimSun (14.6MB), MSPGothic
(8.75MB), and fallback MingLiU-ExtB (33MB) are quite large. I'd prefer to not
make an extra copy on the browser side and then make another extra copy on the
renderer side just to get the data in and out of 'approved' types (which will
serialize the same in any event, though your objection seems to be that they
might not).

In other words, I believe that the only upside to using existing IPC ParamTraits
is that fontmgr_messages.cc becomes simpler (but it doesn't seem difficult to
verify) and the downside is making fontmgr_proxy.cc and
fontmgr_message_filter.cc more complex and adding a *lot* of unneeded copies of
data. This interface is unlikely to add new types in the future in any event,
and the only real potential changes being to SkFontStyle which is a packed value
type anyway.

I would also like to point out that the 'linux' version of this (which this
somewhat based on and aims to replace eventually) FontConfigIPC which is at
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/fon...
uses SendRecvMsg directly and does it's own serialization of SkString and
SkFontConfigInterface::FontIdentity (which itself contains an SkString).

Further, I do not see how making a copy into another type prevents any kind of
'information leakage'. The code which would make the copy would be the same as
what is in fontmgr_messages.cc presently (PS5). I do not believe that there
currently is any unintended information leaking, but if there were, making a
copy would not prevent its transmission.

In other words, while I understand your general concern, I am confused as to the
actual objection with this code. It appears that the only thing which would need
to be audited is fontmgr_messages.cc to ensure that the ParamTraits are sound.
Anything further seems odd, as either we would be leaking information (which is
not helped by a copy) or the serialization would be failing (and the basic
checks for that are part of Message/Pickle). In other words, if I used only
existing ParamTraits, what would that fix, specifically?

[bungeman-chromium, 2014-03-21 22:41:44]

By request from Skia reviewers the interface now takes SkUnichar (int32_t) for
characters, and the SkFontMgr_Interface is hidden behind a factory method since
it is likely that the constructor may take different types in the  future.

[reed1, 2014-03-24 13:21:58]

lgtm

[bungeman-chromium, 2014-03-24 15:20:44]

On 2014/03/20 22:13:01, Justin Schuh wrote:
> On 2014/03/20 20:24:26, bungeman2 wrote:
> > PS5 addresses the issue with SkString (by removing it) now that the Skia
side
> is
> > mostly nailed down. It also cleans up the build files now that the locations
> of
> > the files has stabilized. PTAL.
> 
> More broadly, using Skia primitives over the IPC layer is a bad idea if it can
> be at all avoided. Chrome's baseline primitives have all been audited for
buffer
> management, information leakage, and other potential vulnerabilities. That's
> going to incur a large security review burden now, and a continuous one for
any
> changes moving forward. Instead, could you use Chrome's primitives at the IPC
> layer, unless the alternative is genuinely too onerous?

We can, of course, use Chromium's primitives, but from other communication
(particularly https://codereview.chromium.org/209163002/ ) it appears that your
actual larger objection may be to the browser sending the renderer any
information the system font manager provided, or perhaps the issue of running
any of the system font manager in the browser process at all?

[cpu_(ooo_6.6-7.5), 2014-03-26 20:02:38]

I am doing this review anyhow as I assume a fontmanager needs to be done anyhow
and this code seems pretty platform agnostic.

If not then please ignore my comments.

https://codereview.chromium.org/132113015/diff/160001/content/common/fontmgr_...
File content/common/fontmgr_messages.cc (right):

https://codereview.chromium.org/132113015/diff/160001/content/common/fontmgr_...
content/common/fontmgr_messages.cc:30: m->WriteData(reinterpret_cast<const char
*>(p->at(i)), p->atSize(i));
we want to avoid this 'bag of bytes' approach to IPC since it is hard to
validate, basically SkDataTableBuilder would have to have its own security
review in this case the append method just does a memcpy to create the
SkDataTable which in turn has scary methods like atStr().

I understand that some of the param traits are read on the renderer and write on
the browser, but once this kind of code lands in then it is very easy for
somebody else to reuse them in the other direction at some point.

https://codereview.chromium.org/132113015/diff/160001/content/common/fontmgr_...
content/common/fontmgr_messages.cc:52: SkDataTableBuilder builder(1024);
is there a limit in count? or the 1024 is just advisory?

https://codereview.chromium.org/132113015/diff/160001/content/common/fontmgr_...
content/common/fontmgr_messages.cc:84: count >= 0 && m->WriteInt(count) &&
m->WriteBytes(data, length);
we don't use that style in chrome afaik.

https://codereview.chromium.org/132113015/diff/160001/content/common/fontmgr_...
content/common/fontmgr_messages.cc:95: if (count < 0) {
count = -1 ?

https://codereview.chromium.org/132113015/diff/160001/content/common/fontmgr_...
content/common/fontmgr_messages.cc:100: int length = static_cast<int>(count *
sizeof(SkFontIdentity));
potential integer overflow at the * step.

https://codereview.chromium.org/132113015/diff/160001/content/common/fontmgr_...
content/common/fontmgr_messages.cc:135: m->WriteInt(0) && m->WriteBytes(NULL,
0);
same comment here of line 84 and elsewhere.

https://codereview.chromium.org/132113015/diff/160001/content/common/fontmgr_...
content/common/fontmgr_messages.cc:169: 
same comment as line 30.

Since the fonts are sent this way, how big do we expect each font to be (in
average) and how many fonts do we need to stream to render say google.com in
english, or whatever at hand number happens to be.

I guess how many bytes of fonts to render a common page would be the metric I
would like to know.