New implementation of font precache on Windows.

New implementation of font precache on Windows.

BUG=94421
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=101911

[arthurhsu, 2011-09-13 18:36:58]

If you don't have time to review this CL, please let me know.  Thanks.

[nsylvain, 2011-09-13 19:32:17]

The code itself looks pretty good, but i haven't really written code in chrome
in the last 2 years, so I think you'll want a more seasoned reviewer to LGTM
this.  I suggest rvargas, but feel free to add anyone else.

I'm still concerned about what I said earlier in the thread about this. Sorry if
I'm repeating myself.

Take this case for example:

Renderer A calls GetFontData for font_x and it fails, so Renderer A asks for the
browser the load the font. Renderer A calls GetFontData again, and now it works.

So far the browser cache has only 1 element, font_x, and it is associated with
renderer A

Now Renderer B also tries to load font_x using GetFontData.  Since font_x is
already cached by the browser, GetFontData succeeds.

At this point, if Renderer A terminates, it will destroy the cache for font_x,
and this will make Renderer B crash.

I believe that if you want to ever clean up the cache, you need to talk to the
browser to update the cache even if GetFontData succeeds on the first try.  But
I assume this will slow down quite a bit the browser... so we probably need to
optimize this quite a bit.

WDYT?

http://codereview.chromium.org/7866019/diff/1/chrome/common/chrome_utility_me...
File chrome/common/chrome_utility_messages.h (right):

http://codereview.chromium.org/7866019/diff/1/chrome/common/chrome_utility_me...
chrome/common/chrome_utility_messages.h:151: int /*pid*/)
should you really trust the renderer with the pid? A malicious renderer could
potentially screw your cache.  I'm pretty sure there is a way to get the
renderer pid within the message handler.

[arthurhsu, 2011-09-13 19:41:33]

On 2011/09/13 19:32:17, nsylvain wrote:
> The code itself looks pretty good, but i haven't really written code in chrome
> in the last 2 years, so I think you'll want a more seasoned reviewer to LGTM
> this.  I suggest rvargas, but feel free to add anyone else.
> 
> I'm still concerned about what I said earlier in the thread about this. Sorry
if
> I'm repeating myself.
> 
> Take this case for example:
> 
> Renderer A calls GetFontData for font_x and it fails, so Renderer A asks for
the
> browser the load the font. Renderer A calls GetFontData again, and now it
works.
> 
> So far the browser cache has only 1 element, font_x, and it is associated with
> renderer A
> 
> Now Renderer B also tries to load font_x using GetFontData.  Since font_x is
> already cached by the browser, GetFontData succeeds.
> 
> At this point, if Renderer A terminates, it will destroy the cache for font_x,
> and this will make Renderer B crash.
> 
> I believe that if you want to ever clean up the cache, you need to talk to the
> browser to update the cache even if GetFontData succeeds on the first try. 
But
> I assume this will slow down quite a bit the browser... so we probably need to
> optimize this quite a bit.
> 
> WDYT?
> 
>
http://codereview.chromium.org/7866019/diff/1/chrome/common/chrome_utility_me...
> File chrome/common/chrome_utility_messages.h (right):
> 
>
http://codereview.chromium.org/7866019/diff/1/chrome/common/chrome_utility_me...
> chrome/common/chrome_utility_messages.h:151: int /*pid*/)
> should you really trust the renderer with the pid? A malicious renderer could
> potentially screw your cache.  I'm pretty sure there is a way to get the
> renderer pid within the message handler.

The pid is used to ref-count the handle.

Take your case for example,

Renderer A call GetFontData for font_x, ask browser to cache.  Browser caches
font_x and it works.  ref count of font_x is 1.

Renderer B call GetFontData for font_x, Browser create the GDI object again (to
avoid GDI swap out of font_x) and bump the ref count to 2

Renderer B goes away, ref count of its requests goes down, so ref count of
font_x is now 1.

Renderer A goes away, ref count of font_x is zero.  This time we really close
out the handle.

The clean up is triggered by browser_child_process_host when a child process
died so I don't need to really talk to the process, it just act as an async
event.

I don't know how to get renderer pid within the message handler.  Do you have
any examples?

[arthurhsu, 2011-09-13 20:01:43]

On 2011/09/13 19:41:33, arthurhsu wrote:
> I don't know how to get renderer pid within the message handler.  Do you have
> any examples?

Nevermind.  I figured out how.  Will update the CL.  Thanks.

[nsylvain, 2011-09-13 20:16:51]

On Tue, Sep 13, 2011 at 12:41 PM, <arthurhsu@chromium.org> wrote:

> On 2011/09/13 19:32:17, nsylvain wrote:
>
>> The code itself looks pretty good, but i haven't really written code in
>> chrome
>> in the last 2 years, so I think you'll want a more seasoned reviewer to
>> LGTM
>> this.  I suggest rvargas, but feel free to add anyone else.
>>
>
>  I'm still concerned about what I said earlier in the thread about this.
>> Sorry
>>
> if
>
>> I'm repeating myself.
>>
>
>  Take this case for example:
>>
>
>  Renderer A calls GetFontData for font_x and it fails, so Renderer A asks
>> for
>>
> the
>
>> browser the load the font. Renderer A calls GetFontData again, and now it
>>
> works.
>
>  So far the browser cache has only 1 element, font_x, and it is associated
>> with
>> renderer A
>>
>
>  Now Renderer B also tries to load font_x using GetFontData.  Since font_x
>> is
>> already cached by the browser, GetFontData succeeds.
>>
>
>  At this point, if Renderer A terminates, it will destroy the cache for
>> font_x,
>> and this will make Renderer B crash.
>>
>
>  I believe that if you want to ever clean up the cache, you need to talk to
>> the
>> browser to update the cache even if GetFontData succeeds on the first try.
>>
> But
>
>> I assume this will slow down quite a bit the browser... so we probably
>> need to
>> optimize this quite a bit.
>>
>
>  WDYT?
>>
>
>
> http://codereview.chromium.**org/7866019/diff/1/chrome/**
>
common/chrome_utility_**messages.h<http://codereview.chromium.org/7866019/diff/1/chrome/common/chrome_utility_messages.h>
>
>> File chrome/common/chrome_utility_**messages.h (right):
>>
>
>
> http://codereview.chromium.**org/7866019/diff/1/chrome/**
>
common/chrome_utility_**messages.h#newcode151<http://codereview.chromium.org/7866019/diff/1/chrome/common/chrome_utility_messages.h#newcode151>
>
>> chrome/common/chrome_utility_**messages.h:151: int /*pid*/)
>> should you really trust the renderer with the pid? A malicious renderer
>> could
>> potentially screw your cache.  I'm pretty sure there is a way to get the
>> renderer pid within the message handler.
>>
>
> The pid is used to ref-count the handle.
>
> Take your case for example,
>
> Renderer A call GetFontData for font_x, ask browser to cache.  Browser
> caches
> font_x and it works.  ref count of font_x is 1.
>
> Renderer B call GetFontData for font_x, Browser create the GDI object again
> (to
> avoid GDI swap out of font_x) and bump the ref count to 2
>
I don't see that in the code.   I see that if GetFontData succeeds on first
try, the
renderer does not notify the browser, so the ref count is not incremented.

Am I missing something?


> Renderer B goes away, ref count of its requests goes down, so ref count of
> font_x is now 1.
>
> Renderer A goes away, ref count of font_x is zero.  This time we really
> close
> out the handle.
>
> The clean up is triggered by browser_child_process_host when a child
> process
> died so I don't need to really talk to the process, it just act as an async
> event.
>
> I don't know how to get renderer pid within the message handler.  Do you
> have
> any examples?
>
>
>
http://codereview.chromium.**org/7866019/<http://codereview.chromium.org/7866...
>

[arthurhsu, 2011-09-13 20:21:33]

On 2011/09/13 20:16:51, nsylvain wrote:
> I don't see that in the code.   I see that if GetFontData succeeds on first
> try, the
> renderer does not notify the browser, so the ref count is not incremented.
> 
> Am I missing something?
> 

ref_count is done withing the cache.
child_process_host.cc:
line 182: if the cached element is new, set ref count to 1
line 189: if the cached element is not new, and the pid is not the same, bump up
ref count by 1
line 216: remove the handle in destructor if and only if ref count is 0

[nsylvain, 2011-09-13 20:28:51]

http://codereview.chromium.org/7866019/diff/1/chrome/utility/chrome_content_u...
File chrome/utility/chrome_content_utility_client.cc (right):

http://codereview.chromium.org/7866019/diff/1/chrome/utility/chrome_content_u...
chrome/utility/chrome_content_utility_client.cc:218: int rv = GetFontData(hdc,
table, offset, buffer, length);
The code you refer is inside PreCacheFont. 

Take this line for example. If this call succeeds, PreCacheFont won't even be
called at all, and yet, the browser needs to know that it can't release that
font.

[arthurhsu, 2011-09-13 20:38:18]

On 2011/09/13 20:28:51, nsylvain wrote:
>
http://codereview.chromium.org/7866019/diff/1/chrome/utility/chrome_content_u...
> File chrome/utility/chrome_content_utility_client.cc (right):
> 
>
http://codereview.chromium.org/7866019/diff/1/chrome/utility/chrome_content_u...
> chrome/utility/chrome_content_utility_client.cc:218: int rv = GetFontData(hdc,
> table, offset, buffer, length);
> The code you refer is inside PreCacheFont. 
> 
> Take this line for example. If this call succeeds, PreCacheFont won't even be
> called at all, and yet, the browser needs to know that it can't release that
> font.

It shall not matter if line 218 succeeds because the required data in the return
buffer will not change for subsequent GDI calls made in ReleaseCachedFont() in
browser process (because they are two handles from two different processes, all
we want to do is to make sure GDI keep that font in memory, which it will in
this case because GDI knows there are still open handles out there).  That's why
the ReleaseCachedFont is called immediately after PreCacheFont in FontDataPatch.

[arthurhsu, 2011-09-13 20:58:32]

http://codereview.chromium.org/7866019/diff/1/chrome/common/chrome_utility_me...
File chrome/common/chrome_utility_messages.h (right):

http://codereview.chromium.org/7866019/diff/1/chrome/common/chrome_utility_me...
chrome/common/chrome_utility_messages.h:151: int /*pid*/)
On 2011/09/13 19:32:18, nsylvain wrote:
> should you really trust the renderer with the pid? A malicious renderer could
> potentially screw your cache.  I'm pretty sure there is a way to get the
> renderer pid within the message handler.

This one is a service utility process host, which is used internally by
printing.  The other one used by renderer process is changed.

[nsylvain, 2011-09-13 21:35:47]

On 2011/09/13 20:38:18, arthurhsu wrote:
> On 2011/09/13 20:28:51, nsylvain wrote:
> >
>
http://codereview.chromium.org/7866019/diff/1/chrome/utility/chrome_content_u...
> > File chrome/utility/chrome_content_utility_client.cc (right):
> > 
> >
>
http://codereview.chromium.org/7866019/diff/1/chrome/utility/chrome_content_u...
> > chrome/utility/chrome_content_utility_client.cc:218: int rv =
GetFontData(hdc,
> > table, offset, buffer, length);
> > The code you refer is inside PreCacheFont. 
> > 
> > Take this line for example. If this call succeeds, PreCacheFont won't even
be
> > called at all, and yet, the browser needs to know that it can't release that
> > font.
> 
> It shall not matter if line 218 succeeds because the required data in the
return
> buffer will not change for subsequent GDI calls made in ReleaseCachedFont() in
> browser process (because they are two handles from two different processes,
all
> we want to do is to make sure GDI keep that font in memory, which it will in
> this case because GDI knows there are still open handles out there).  That's
why
> the ReleaseCachedFont is called immediately after PreCacheFont in
FontDataPatch.

I see, this is where the confusion comes from, since this is not how I thought
it worked. If you think this is right, then LGTM (but someone else needs to look
at the code and LGTM as well)

We should do a lot of testing. In the past we've only had to put code around
GetFontData because it was called really early in the font code, and the browser
cache was never destroyed, but until now I was fairly confident that many other
calls in chrome (and in the windows libraries) will try to load the font again
(even after the handle is closed), and if the browser is not caching it anymore,
it will fail badly.

[arthurhsu, 2011-09-13 22:33:13]

Ricardo, please let me know when you have time to review this.  This CL is
related to code-yellow defect 94421.  Thanks.

[rvargas (doing something else), 2011-09-13 23:40:30]

There is already a one to one mapping from child_process_host to child, so it
doesn't make much sense to me to have static methods and then have a map to find
out something about a given PID. That also means that PIDs are redundant.

It looks like tracking process lifetime is meant to be just a safety net and
everything is purged from the cache a few milliseconds after it was added. If
that is the intended behavior it looks like a lot of infrastructure for it. If
we rely on process destruction, why bother with a second IPC message at all?

http://codereview.chromium.org/7866019/diff/6003/content/common/child_process...
File content/common/child_process_host.h (right):

http://codereview.chromium.org/7866019/diff/6003/content/common/child_process...
content/common/child_process_host.h:32: // Types used in PreCacheFont
Comments should end with period.

These are fairly specific, so should not be global.

http://codereview.chromium.org/7866019/diff/6003/content/common/child_process...
content/common/child_process_host.h:97: static void ReleaseCachedFont(int pid);
This doesn't release a font... releases all fonts for the given PID so font
should be plural... if we keep this method :)

http://codereview.chromium.org/7866019/diff/6003/content/common/child_process...
content/common/child_process_host.h:162: FontCache() {}  // Make Singleton<>
happy.
friend struct DefaultSingletonTraits<FontCache>;

Constructors and destructors should not be inlined.

[arthurhsu, 2011-09-14 02:06:56]

The PreCacheFont is called from two components.  One is the service utility
process, which is part of the printing; the other is from webkit's
Platform::ensureFontLoad().  

For the service utility process, we act like a hook, which we know that we can
release the font as soon as data are in the return buffer.

For the webkit part, we really don't know when will the renderer need the font,
therefore we intercept the renderer exit event and clear cache from there.

http://codereview.chromium.org/7866019/diff/6003/content/common/child_process...
File content/common/child_process_host.h (right):

http://codereview.chromium.org/7866019/diff/6003/content/common/child_process...
content/common/child_process_host.h:32: // Types used in PreCacheFont
On 2011/09/13 23:40:30, rvargas wrote:
> Comments should end with period.
> 
> These are fairly specific, so should not be global.

Done.

http://codereview.chromium.org/7866019/diff/6003/content/common/child_process...
content/common/child_process_host.h:97: static void ReleaseCachedFont(int pid);
On 2011/09/13 23:40:30, rvargas wrote:
> This doesn't release a font... releases all fonts for the given PID so font
> should be plural... if we keep this method :)

Done.

http://codereview.chromium.org/7866019/diff/6003/content/common/child_process...
content/common/child_process_host.h:162: FontCache() {}  // Make Singleton<>
happy.
On 2011/09/13 23:40:30, rvargas wrote:
> friend struct DefaultSingletonTraits<FontCache>;
> 
> Constructors and destructors should not be inlined.

Done.

[rvargas (doing something else), 2011-09-14 03:05:04]

I see. We still should be able to remove the PIDs and the map though.

http://codereview.chromium.org/7866019/diff/6016/content/common/child_process...
File content/common/child_process_host.h (right):

http://codereview.chromium.org/7866019/diff/6016/content/common/child_process...
content/common/child_process_host.h:171: FontCache();  // Make Singleton<>
happy.
nit: remove the comment.

http://codereview.chromium.org/7866019/diff/6016/content/common/child_process...
content/common/child_process_host.h:178: DISALLOW_COPY_AND_ASSIGN(FontCache);
This should be the last item on the class... struct goes first.

[arthurhsu, 2011-09-14 20:25:48]

Do you mean remove the pid from cache impl, or remove pid from IPC message map?

http://codereview.chromium.org/7866019/diff/6016/content/common/child_process...
File content/common/child_process_host.h (right):

http://codereview.chromium.org/7866019/diff/6016/content/common/child_process...
content/common/child_process_host.h:171: FontCache();  // Make Singleton<>
happy.
On 2011/09/14 03:05:05, rvargas wrote:
> nit: remove the comment.

Done.

http://codereview.chromium.org/7866019/diff/6016/content/common/child_process...
content/common/child_process_host.h:178: DISALLOW_COPY_AND_ASSIGN(FontCache);
On 2011/09/14 03:05:05, rvargas wrote:
> This should be the last item on the class... struct goes first.

Done.

[rvargas (doing something else), 2011-09-14 21:19:47]

On 2011/09/14 20:25:48, arthurhsu wrote:
> Do you mean remove the pid from cache impl, or remove pid from IPC message
map?

Both. What I mean is that ChildProcessHost is an object bound to a given child
process (renderer, or anything else that is sandboxed). The reason for
PreCacheFont to be static is that we didn't care about any specific process, but
now we do.

But in any case, you'll need a content owner so I added John.

[rvargas (doing something else), 2011-09-14 21:21:36]

http://codereview.chromium.org/7866019/diff/8004/content/common/child_process...
File content/common/child_process_host.h (right):

http://codereview.chromium.org/7866019/diff/8004/content/common/child_process...
content/common/child_process_host.h:178: friend struct
DefaultSingletonTraits<FontCache>;
Sorry I was not clear, struct declarations go before constructors.

[arthurhsu, 2011-09-15 05:52:47]

I also removed the pid from service utility message.

As to the pid used in cache implementation, it is necessary.  I use the pid as a
key to associate a child process with the fonts that it use.  As explained in
previous comments, we have no idea when will webkit use the font, so we can only
release the GDI handle when the renderer process goes away.  We want to release
unused GDI handles so that we don't use more memory than we need.

http://codereview.chromium.org/7866019/diff/8004/content/common/child_process...
File content/common/child_process_host.h (right):

http://codereview.chromium.org/7866019/diff/8004/content/common/child_process...
content/common/child_process_host.h:178: friend struct
DefaultSingletonTraits<FontCache>;
On 2011/09/14 21:21:36, rvargas wrote:
> Sorry I was not clear, struct declarations go before constructors.

Done.

[jam, 2011-09-15 16:18:23]

http://codereview.chromium.org/7866019/diff/4024/chrome/common/chrome_utility...
File chrome/common/chrome_utility_messages.h (right):

http://codereview.chromium.org/7866019/diff/4024/chrome/common/chrome_utility...
chrome/common/chrome_utility_messages.h:153:
IPC_MESSAGE_CONTROL0(ChromeUtilityHostMsg_ReleaseCachedFonts)
instead of declaring this message for utility/renderer, it should be in
child_process_messages.h. and while you're at it, please move
ViewHostMsg_PreCacheFont as well and ChromeUtilityHostMsg_PreCacheFont

http://codereview.chromium.org/7866019/diff/4024/chrome/service/service_utili...
File chrome/service/service_utility_process_host.cc (right):

http://codereview.chromium.org/7866019/diff/4024/chrome/service/service_utili...
chrome/service/service_utility_process_host.cc:175: ReleaseCachedFonts(pid());
you use pid for service processes, while for renderers you're using process_id
(which is not a pid, but rather a unique id). the reason we avoid using pids is
that they get recycled, which can cause some hard to catch bugs. Can you make
each ServiceutilityProcessHost generate a unique id using
ChildProcessInfo::GenerateChildProcessUniqueId, and pass that to the font
caching code

[rvargas (doing something else), 2011-09-15 18:19:26]

On 2011/09/15 05:52:47, arthurhsu wrote:
> I also removed the pid from service utility message.
> 
> As to the pid used in cache implementation, it is necessary.  I use the pid as
a
> key to associate a child process with the fonts that it use.  As explained in
> previous comments, we have no idea when will webkit use the font, so we can
only
> release the GDI handle when the renderer process goes away.  We want to
release
> unused GDI handles so that we don't use more memory than we need.

Yes, I see that, but ChildProcessHost::OnChildDied() provides that signal (time
to cleanup all fonts used by this process). Am I missing something? 


> 
>
http://codereview.chromium.org/7866019/diff/8004/content/common/child_process...
> File content/common/child_process_host.h (right):
> 
>
http://codereview.chromium.org/7866019/diff/8004/content/common/child_process...
> content/common/child_process_host.h:178: friend struct
> DefaultSingletonTraits<FontCache>;
> On 2011/09/14 21:21:36, rvargas wrote:
> > Sorry I was not clear, struct declarations go before constructors.
> 
> Done.

[arthurhsu, 2011-09-15 18:38:29]

On 2011/09/15 18:19:26, rvargas wrote:
> On 2011/09/15 05:52:47, arthurhsu wrote:
> > I also removed the pid from service utility message.
> > 
> > As to the pid used in cache implementation, it is necessary.  I use the pid
as
> a
> > key to associate a child process with the fonts that it use.  As explained
in
> > previous comments, we have no idea when will webkit use the font, so we can
> only
> > release the GDI handle when the renderer process goes away.  We want to
> release
> > unused GDI handles so that we don't use more memory than we need.
> 
> Yes, I see that, but ChildProcessHost::OnChildDied() provides that signal
(time
> to cleanup all fonts used by this process). Am I missing something? 
> 
> 
> > 
> >
>
http://codereview.chromium.org/7866019/diff/8004/content/common/child_process...
> > File content/common/child_process_host.h (right):
> > 
> >
>
http://codereview.chromium.org/7866019/diff/8004/content/common/child_process...
> > content/common/child_process_host.h:178: friend struct
> > DefaultSingletonTraits<FontCache>;
> > On 2011/09/14 21:21:36, rvargas wrote:
> > > Sorry I was not clear, struct declarations go before constructors.
> > 
> > Done.

Unique process id is stored in ChildProcessInfo, which is not directly
accessible from ChildProcessHost::OnChildDied().

For hooking style usage in service util process, I chose to release the font
immediately.  The service util process can live quite long and it does not need
to hold the font for its lifetime.

[rvargas (doing something else), 2011-09-15 18:54:36]

On 2011/09/15 18:38:29, arthurhsu wrote:
> On 2011/09/15 18:19:26, rvargas wrote:
> > On 2011/09/15 05:52:47, arthurhsu wrote:
> > > I also removed the pid from service utility message.
> > > 
> > > As to the pid used in cache implementation, it is necessary.  I use the
pid
> as
> > a
> > > key to associate a child process with the fonts that it use.  As explained
> in
> > > previous comments, we have no idea when will webkit use the font, so we
can
> > only
> > > release the GDI handle when the renderer process goes away.  We want to
> > release
> > > unused GDI handles so that we don't use more memory than we need.
> > 
> > Yes, I see that, but ChildProcessHost::OnChildDied() provides that signal
> (time
> > to cleanup all fonts used by this process). Am I missing something? 
> > 
> > 
> > > 
> > >
> >
>
http://codereview.chromium.org/7866019/diff/8004/content/common/child_process...
> > > File content/common/child_process_host.h (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/7866019/diff/8004/content/common/child_process...
> > > content/common/child_process_host.h:178: friend struct
> > > DefaultSingletonTraits<FontCache>;
> > > On 2011/09/14 21:21:36, rvargas wrote:
> > > > Sorry I was not clear, struct declarations go before constructors.
> > > 
> > > Done.
> 
> Unique process id is stored in ChildProcessInfo, which is not directly
> accessible from ChildProcessHost::OnChildDied().

hence the whole point about not really needing a map, or a unique process id at
all.

> For hooking style usage in service util process, I chose to release the font
> immediately.  The service util process can live quite long and it does not
need
> to hold the font for its lifetime.

Right, and that's why you have a ReleaseFont message, but that is not related to
needing a process id, is it? Any regular (non-static) member of ChildProcessHost
already knows what process it is dealing with ("this" is paired with that
process). Unless I'm completely misunderstanding how ChildProcessHost works.

In any case, if jam is happy I'm happy.

[arthurhsu, 2011-09-15 19:05:00]

I have already removed all pid from messages.

For cache implementation, PID is introduced to solve this issue.

Renderer A uses fontX, browser loads it
Renderer B uses fontX, browser loads it
Renderer B goes away, browser DeleteObject()
Renderer A GetFontData(), Kaboom!

I use PID to keep track how fontX maps to what process.  When the last process
referencing fontX is gone, it's safe to free fontX.

[rvargas (doing something else), 2011-09-15 19:25:51]

On 2011/09/15 19:05:00, arthurhsu wrote:
> I have already removed all pid from messages.
> 
> For cache implementation, PID is introduced to solve this issue.
> 
> Renderer A uses fontX, browser loads it
> Renderer B uses fontX, browser loads it
> Renderer B goes away, browser DeleteObject()
> Renderer A GetFontData(), Kaboom!
> 
> I use PID to keep track how fontX maps to what process.  When the last process
> referencing fontX is gone, it's safe to free fontX.

You don't need a pid to solve that problem. You have the reference count for
that (number of processes that are using that font). You have a map because you
are using static members, and then you have to go from that to a process.

That's what I feel is wrong: having a class with objects that abstract a
process, but adding static members (that lose the knowledge of individual
processes) and then having to add a process_identifier and a map to go back to
the lost concept.

But as I said, I'm happy if John is happy.

[arthurhsu, 2011-09-15 19:41:02]

On 2011/09/15 19:25:51, rvargas wrote:
> On 2011/09/15 19:05:00, arthurhsu wrote:
> > I have already removed all pid from messages.
> > 
> > For cache implementation, PID is introduced to solve this issue.
> > 
> > Renderer A uses fontX, browser loads it
> > Renderer B uses fontX, browser loads it
> > Renderer B goes away, browser DeleteObject()
> > Renderer A GetFontData(), Kaboom!
> > 
> > I use PID to keep track how fontX maps to what process.  When the last
process
> > referencing fontX is gone, it's safe to free fontX.
> 
> You don't need a pid to solve that problem. You have the reference count for
> that (number of processes that are using that font). You have a map because
you
> are using static members, and then you have to go from that to a process.
> 
> That's what I feel is wrong: having a class with objects that abstract a
> process, but adding static members (that lose the knowledge of individual
> processes) and then having to add a process_identifier and a map to go back to
> the lost concept.
> 
> But as I said, I'm happy if John is happy.

A renderer can/will request fontX for multiple times to avoid GDI swapping the
font out of memory.  Given that I don't know when renderer/webkit is done with a
font, pid is introduced to do a proper ref count so that each process
add/release a font's ref count only once.

[rvargas (doing something else), 2011-09-15 21:50:10]

> A renderer can/will request fontX for multiple times to avoid GDI swapping the
> font out of memory.  Given that I don't know when renderer/webkit is done with
a
> font, pid is introduced to do a proper ref count so that each process
> add/release a font's ref count only once.

And again, I don't see the connection between that and a map<process,
process_fonts>. I have never suggested to get rid of process_fonts.

On a more serious point, given that we don't know when it is safe to release a
font for a renderer whithout causing it to crash, it seems like we cannot ignore
renderers that are using a given font without talking to the browser process: if
there is any chance that ensureFontLoaded is not called for a given renderer
because it was able to start using a font already cached, we have a problem.

[arthurhsu, 2011-09-16 02:14:06]

http://codereview.chromium.org/7866019/diff/4024/chrome/common/chrome_utility...
File chrome/common/chrome_utility_messages.h (right):

http://codereview.chromium.org/7866019/diff/4024/chrome/common/chrome_utility...
chrome/common/chrome_utility_messages.h:153:
IPC_MESSAGE_CONTROL0(ChromeUtilityHostMsg_ReleaseCachedFonts)
On 2011/09/15 16:18:23, John Abd-El-Malek wrote:
> instead of declaring this message for utility/renderer, it should be in
> child_process_messages.h. and while you're at it, please move
> ViewHostMsg_PreCacheFont as well and ChromeUtilityHostMsg_PreCacheFont

Done.

http://codereview.chromium.org/7866019/diff/4024/chrome/service/service_utili...
File chrome/service/service_utility_process_host.cc (right):

http://codereview.chromium.org/7866019/diff/4024/chrome/service/service_utili...
chrome/service/service_utility_process_host.cc:175: ReleaseCachedFonts(pid());
On 2011/09/15 16:18:23, John Abd-El-Malek wrote:
> you use pid for service processes, while for renderers you're using process_id
> (which is not a pid, but rather a unique id). the reason we avoid using pids
is
> that they get recycled, which can cause some hard to catch bugs. Can you make
> each ServiceutilityProcessHost generate a unique id using
> ChildProcessInfo::GenerateChildProcessUniqueId, and pass that to the font
> caching code

Done.

[jam, 2011-09-19 16:31:17]

http://codereview.chromium.org/7866019/diff/18014/chrome/service/service_util...
File chrome/service/service_utility_process_host.cc (right):

http://codereview.chromium.org/7866019/diff/18014/chrome/service/service_util...
chrome/service/service_utility_process_host.cc:122: unique_process_id_ =
ChildProcessInfo::GenerateChildProcessUniqueId();
we usually set this in the constructor. that way the id stays the same even if
the process crashes. it's also simpler to have the id from the beginning,
instead of having some period of time when we don't have an id

http://codereview.chromium.org/7866019/diff/18014/chrome/service/service_util...
File chrome/service/service_utility_process_host.h (right):

http://codereview.chromium.org/7866019/diff/18014/chrome/service/service_util...
chrome/service/service_utility_process_host.h:111: int unique_process_id();
no need for this getter since it's only used in the class

http://codereview.chromium.org/7866019/diff/18014/chrome/service/service_util...
chrome/service/service_utility_process_host.h:149: int unique_process_id_;
nit: this should just be called process_id_ to be consistent with our other
usages of this (i.e. we don't say unique_process_id anywhere)

http://codereview.chromium.org/7866019/diff/18014/content/browser/browser_chi...
File content/browser/browser_child_process_host.cc (right):

http://codereview.chromium.org/7866019/diff/18014/content/browser/browser_chi...
content/browser/browser_child_process_host.cc:158:
base::GetProcId(child_process_->GetHandle()));
1) this currently doesn't do anything, since it's using the pid instead of the
process id

2) why not do this in RenderMessageFilter::OnChannelError (not currently
overriden, but can be for this purpose)? that way all the font related code is
in one place

http://codereview.chromium.org/7866019/diff/18014/content/common/child_proces...
File content/common/child_process_messages.h (right):

http://codereview.chromium.org/7866019/diff/18014/content/common/child_proces...
content/common/child_process_messages.h:66:
IPC_SYNC_MESSAGE_CONTROL1_0(ChromeUtilityHostMsg_PreCacheFont,
messages in this file start with ChildProcessMsg or ChildProcessHostMsg.
ChromeUtilityHostMsg would belong in chrome_utility_messages.h, and ViewHostMsg
would belong in view_messages.h

also, sorry if I wasn't clear, but the reason I said these should be moved here
is so that we avoid duplicate messages for the same purpose. i.e. there should
be just one ChildHostMsg_PreCacheFont, and one ChildHostMsg_ReleaseCachedFonts

[arthurhsu, 2011-09-19 18:22:51]

http://codereview.chromium.org/7866019/diff/18014/chrome/service/service_util...
File chrome/service/service_utility_process_host.cc (right):

http://codereview.chromium.org/7866019/diff/18014/chrome/service/service_util...
chrome/service/service_utility_process_host.cc:122: unique_process_id_ =
ChildProcessInfo::GenerateChildProcessUniqueId();
On 2011/09/19 16:31:17, John Abd-El-Malek wrote:
> we usually set this in the constructor. that way the id stays the same even if
> the process crashes. it's also simpler to have the id from the beginning,
> instead of having some period of time when we don't have an id

Done.

http://codereview.chromium.org/7866019/diff/18014/chrome/service/service_util...
File chrome/service/service_utility_process_host.h (right):

http://codereview.chromium.org/7866019/diff/18014/chrome/service/service_util...
chrome/service/service_utility_process_host.h:111: int unique_process_id();
On 2011/09/19 16:31:17, John Abd-El-Malek wrote:
> no need for this getter since it's only used in the class

Done.

http://codereview.chromium.org/7866019/diff/18014/chrome/service/service_util...
chrome/service/service_utility_process_host.h:149: int unique_process_id_;
On 2011/09/19 16:31:17, John Abd-El-Malek wrote:
> nit: this should just be called process_id_ to be consistent with our other
> usages of this (i.e. we don't say unique_process_id anywhere)

Done.

http://codereview.chromium.org/7866019/diff/18014/content/browser/browser_chi...
File content/browser/browser_child_process_host.cc (right):

http://codereview.chromium.org/7866019/diff/18014/content/browser/browser_chi...
content/browser/browser_child_process_host.cc:158:
base::GetProcId(child_process_->GetHandle()));
On 2011/09/19 16:31:17, John Abd-El-Malek wrote:
> 1) this currently doesn't do anything, since it's using the pid instead of the
> process id
> 
> 2) why not do this in RenderMessageFilter::OnChannelError (not currently
> overriden, but can be for this purpose)? that way all the font related code is
> in one place

Done.

http://codereview.chromium.org/7866019/diff/18014/content/common/child_proces...
File content/common/child_process_messages.h (right):

http://codereview.chromium.org/7866019/diff/18014/content/common/child_proces...
content/common/child_process_messages.h:66:
IPC_SYNC_MESSAGE_CONTROL1_0(ChromeUtilityHostMsg_PreCacheFont,
On 2011/09/19 16:31:17, John Abd-El-Malek wrote:
> messages in this file start with ChildProcessMsg or ChildProcessHostMsg.
> ChromeUtilityHostMsg would belong in chrome_utility_messages.h, and
ViewHostMsg
> would belong in view_messages.h
> 
> also, sorry if I wasn't clear, but the reason I said these should be moved
here
> is so that we avoid duplicate messages for the same purpose. i.e. there should
> be just one ChildHostMsg_PreCacheFont, and one ChildHostMsg_ReleaseCachedFonts

Done.

[jam, 2011-09-19 21:27:48]

lgtm

http://codereview.chromium.org/7866019/diff/27001/content/common/child_proces...
File content/common/child_process_host.h (right):

http://codereview.chromium.org/7866019/diff/27001/content/common/child_proces...
content/common/child_process_host.h:93: static void PreCacheFont(LOGFONT font,
int pid);
nit: here and below, and also in the cc file, these should be process_id to make
it a little clearer that they're not pids

[arthurhsu, 2011-09-19 22:47:31]

http://codereview.chromium.org/7866019/diff/27001/content/common/child_proces...
File content/common/child_process_host.h (right):

http://codereview.chromium.org/7866019/diff/27001/content/common/child_proces...
content/common/child_process_host.h:93: static void PreCacheFont(LOGFONT font,
int pid);
On 2011/09/19 21:27:49, John Abd-El-Malek wrote:
> nit: here and below, and also in the cc file, these should be process_id to
make
> it a little clearer that they're not pids

Done.

[commit-bot: I haz the power, 2011-09-20 00:35:33]

Can't apply patch for file chrome/renderer/chrome_render_process_observer.cc.
While running patch -p1 --forward --force;
patching file chrome/renderer/chrome_render_process_observer.cc
Hunk #2 FAILED at 164.
1 out of 2 hunks FAILED -- saving rejects to file
chrome/renderer/chrome_render_process_observer.cc.rej

[commit-bot: I haz the power, 2011-09-20 03:33:24]

Change committed as 101911