Move PPB_TrueTypeFont_Dev host from renderer to browser.

Move PPB_TrueTypeFont_Dev host from renderer to browser.

On Linux, we can now use FontConfig in the browser process since it
is thread-safe, so we don't need to use SandboxIPC from the renderer.
In the browser, the needed functionality is split off from
SandboxIPCHandler into a static function,
MatchFontFaceWithFallback in a new pair of files, font_utils_linux.*.

This change also moves any potentially blocking font creation and
reading to the browser's blocking thread pool.

I reworked the PepperTrueTypeFont base class to make Create simpler.
- Added an Initialize method which returns the font descriptor.
- Removed the Describe method.

I reworked the resource to delay calls to the host until we receive the desc
and initialization is complete. Describe will now wait until Initialize completes,
and the host uses a SequencedTaskRunner to serialize tasks, so Initialize
completes before GetTableTags and GetTable calls complete.

The Describe method can be implemented without IPC since we have the desc
on the plugin side.

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

[bbudge, 2014-06-19 14:19:03]

teravest - /pepper stuff
jorgelo - browser/renderer_host/*_linux sandbox files.

[bbudge, 2014-06-19 14:21:16]

+ sky for content OWNERS
Please feel free to suggest an alternate reviewer, perhaps a font expert.

[sky, 2014-06-19 15:49:14]

I'm not a good reviewer for this. Maybe piman?

[Jorge Lucangeli Obes, 2014-06-19 16:20:35]

On 2014/06/19 14:19:03, bbudge wrote:
> teravest - /pepper stuff
> jorgelo - browser/renderer_host/*_linux sandbox files.

Everything looks reasonable. Can you make sure
https://code.google.com/p/chromium/issues/detail?id=381978 did not regress (it
shouldn't, but I already regressed it once) and that content_browsertests pass
under TSan?

[piman, 2014-06-19 20:03:42]

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
File content/browser/renderer_host/font_utils_linux.cc (right):

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
content/browser/renderer_host/font_utils_linux.cc:237: font_fd =
open(reinterpret_cast<char*>(c_filename), O_RDONLY);
Do we need to HANDLE_EINTR?

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
content/browser/renderer_host/font_utils_linux.cc:249: font_fd =
open(reinterpret_cast<char*>(c_filename), O_RDONLY);
Do we need to HANDLE_EINTR?

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc
(right):

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:157:
host_, instance, params.pp_resource(), desc));
Would you want to return failure if the PepperTrueTypeFont::Create failed?

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_truetype_font.h (right):

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font.h:16: class
PepperTrueTypeFont : public base::RefCounted<PepperTrueTypeFont> {
RefCountedThreadSafe since you pass references to other threads.

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font.h:52: virtual bool
IsValid() const = 0;
nit: you don't need this to be virtual since the only place you call it is at
construction time, where you have the concrete type.

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_truetype_font_linux.cc (right):

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font_linux.cc:41: int fd_;
Where is fd_ closed? Should this be a ScopedFD?

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font_linux.cc:75:
PP_BROWSERFONT_TRUSTED_FAMILY_DEFAULT);
If I'm not mistaken, this is happening on the IO thread, but opens files
(accesses the disk). FTM, I suspect FontConfig may be doing even more disk
accesses to manage the config files, getting the font list cache, etc. Can we
make this happen on a FILE thread?

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_truetype_font_mac.mm (right):

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font_mac.mm:348: while
(search_range<num_tables>> 1) {
nit: definitely more readable before.

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_truetype_font_win.cc (right):

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font_win.cc:47: HFONT
font_;
What deletes this? Should this be a ScopedHFONT?

[teravest, 2014-06-20 15:22:45]

I don't have much to add to the earlier review comments.

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_truetype_font_host.h (right):

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font_host.h:54:
BrowserPpapiHost* browser_ppapi_host_;
I don't see this member used anywhere. I think you can remove it.

[bbudge, 2014-06-21 14:12:50]

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
File content/browser/renderer_host/font_utils_linux.cc (right):

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
content/browser/renderer_host/font_utils_linux.cc:237: font_fd =
open(reinterpret_cast<char*>(c_filename), O_RDONLY);
On 2014/06/19 20:03:42, piman wrote:
> Do we need to HANDLE_EINTR?

Probably. Done.

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
content/browser/renderer_host/font_utils_linux.cc:249: font_fd =
open(reinterpret_cast<char*>(c_filename), O_RDONLY);
On 2014/06/19 20:03:42, piman wrote:
> Do we need to HANDLE_EINTR?

Probably. Done.

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc
(right):

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:157:
host_, instance, params.pp_resource(), desc));
On 2014/06/19 20:03:42, piman wrote:
> Would you want to return failure if the PepperTrueTypeFont::Create failed?

Create will now always succeed, only returning a failure result after attempting
to create on the blocking pool.

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_truetype_font.h (right):

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font.h:16: class
PepperTrueTypeFont : public base::RefCounted<PepperTrueTypeFont> {
On 2014/06/19 20:03:42, piman wrote:
> RefCountedThreadSafe since you pass references to other threads.

Done.

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font.h:52: virtual bool
IsValid() const = 0;
On 2014/06/19 20:03:42, piman wrote:
> nit: you don't need this to be virtual since the only place you call it is at
> construction time, where you have the concrete type.

This method was eliminated.

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_truetype_font_host.h (right):

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font_host.h:54:
BrowserPpapiHost* browser_ppapi_host_;
On 2014/06/20 15:22:45, teravest wrote:
> I don't see this member used anywhere. I think you can remove it.

Done.

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_truetype_font_linux.cc (right):

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font_linux.cc:41: int fd_;
On 2014/06/19 20:03:42, piman wrote:
> Where is fd_ closed? Should this be a ScopedFD?

Done.

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font_linux.cc:75:
PP_BROWSERFONT_TRUSTED_FAMILY_DEFAULT);
On 2014/06/19 20:03:42, piman wrote:
> If I'm not mistaken, this is happening on the IO thread, but opens files
> (accesses the disk). FTM, I suspect FontConfig may be doing even more disk
> accesses to manage the config files, getting the font list cache, etc. Can we
> make this happen on a FILE thread?

Done. On the blocking pool.

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_truetype_font_mac.mm (right):

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font_mac.mm:348: while
(search_range<num_tables>> 1) {
On 2014/06/19 20:03:42, piman wrote:
> nit: definitely more readable before.

clang format doesn't quite know what to do in here. Fixed with some parens.

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_truetype_font_win.cc (right):

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font_win.cc:47: HFONT
font_;
On 2014/06/19 20:03:42, piman wrote:
> What deletes this? Should this be a ScopedHFONT?

Done.

[bbudge, 2014-06-21 14:15:51]

+tsepez for ppapi_messages.h

[piman, 2014-06-23 18:21:13]

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_truetype_font_mac.mm (right):

https://codereview.chromium.org/337203003/diff/210001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font_mac.mm:348: while
(search_range<num_tables>> 1) {
On 2014/06/21 14:12:50, bbudge wrote:
> On 2014/06/19 20:03:42, piman wrote:
> > nit: definitely more readable before.
> 
> clang format doesn't quite know what to do in here. Fixed with some parens.

worth filing a bug :)

https://codereview.chromium.org/337203003/diff/300001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_truetype_font_host.cc (right):

https://codereview.chromium.org/337203003/diff/300001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font_host.cc:74: font_ =
NULL;
Why is this logic needed? It may not work the way you expect:
1- the task could execute before font_ is reset, so the last reference would be
released on the main thread (io thread?)
2- even if you "pass" the reference to the task, there is a temporary
base::Closure on this thread, that keeps a reference to the PepperTrueTypeFont,
that may outlive the task (for the same reason)

You could use SequencedTaskRunner::ReleaseSoon I suppose, but what is the reason
you need the PepperTrueTypeFont to be deleted on the blocking thread?

https://codereview.chromium.org/337203003/diff/300001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_truetype_font_linux.cc (right):

https://codereview.chromium.org/337203003/diff/300001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font_linux.cc:87: if
(!GetFontTable(fd_.get(),
So, the fd here could be invalid in case of a malicious plugin: while the new
code waits for the result before sending the GetTableTags/GetTable IPCs, a
malicious plugin may choose not to respect that.

I think you need to take this into account here, and return failure if the fd is
invalid (and probably similar things on other platforms).

If that's the case, then I think you can skip the delaying logic on the plugin
side by using SequencedWorkerPool::GetSequencedTaskRunner on the blocking pool
(to ensure task ordering): if the font creation fails, the GetTableTags/GetTable
IPCs will fail too here. If it succeeds, IPC/post task ordering should ensure
we'll get there only after Initialize has run.

I think it would make the code simpler on the plugin side, with minor complexity
on the host side.

[Tom Sepez, 2014-06-23 18:29:14]

Messages themselves LGTM, but it looks like we are moving some more attack
surface into the browser.  +jln, +jschuh.

[bbudge, 2014-06-23 18:34:10]

https://codereview.chromium.org/337203003/diff/300001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_truetype_font_host.cc (right):

https://codereview.chromium.org/337203003/diff/300001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font_host.cc:74: font_ =
NULL;
On 2014/06/23 18:21:12, piman wrote:
> Why is this logic needed? It may not work the way you expect:
> 1- the task could execute before font_ is reset, so the last reference would
be
> released on the main thread (io thread?)
> 2- even if you "pass" the reference to the task, there is a temporary
> base::Closure on this thread, that keeps a reference to the
PepperTrueTypeFont,
> that may outlive the task (for the same reason)
> 
> You could use SequencedTaskRunner::ReleaseSoon I suppose, but what is the
reason
> you need the PepperTrueTypeFont to be deleted on the blocking thread?

PepperTrueTypeFontLinux has to close its file descriptor. I was trying to do
that on a blocking pool thread.

https://codereview.chromium.org/337203003/diff/300001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_truetype_font_linux.cc (right):

https://codereview.chromium.org/337203003/diff/300001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font_linux.cc:87: if
(!GetFontTable(fd_.get(),
On 2014/06/23 18:21:12, piman wrote:
> So, the fd here could be invalid in case of a malicious plugin: while the new
> code waits for the result before sending the GetTableTags/GetTable IPCs, a
> malicious plugin may choose not to respect that.
> 
> I think you need to take this into account here, and return failure if the fd
is
> invalid (and probably similar things on other platforms).
> 
> If that's the case, then I think you can skip the delaying logic on the plugin
> side by using SequencedWorkerPool::GetSequencedTaskRunner on the blocking pool
> (to ensure task ordering): if the font creation fails, the
GetTableTags/GetTable
> IPCs will fail too here. If it succeeds, IPC/post task ordering should ensure
> we'll get there only after Initialize has run.
> 
> I think it would make the code simpler on the plugin side, with minor
complexity
> on the host side.

I thought about using SequencedTaskRunner but it seems like that would serialize
all Get* calls. I was trying to allow them to occur in parallel since they're
just reading the font data. It's probably not a big deal performance-wise. I
agree it would be much simpler to use STR. Is there a way to just have a
sequencing constraint on Initialize and the Get* tasks and allow the Get* tasks
to run in parallel?

[Jorge Lucangeli Obes, 2014-06-23 18:34:52]

https://codereview.chromium.org/337203003/diff/300001/content/browser/rendere...
File content/browser/renderer_host/sandbox_ipc_linux.cc (left):

https://codereview.chromium.org/337203003/diff/300001/content/browser/rendere...
content/browser/renderer_host/sandbox_ipc_linux.cc:585:
FcPatternDestroy(pattern);
Reiterating my previous comment to test this under TSan.

[piman, 2014-06-23 19:05:47]

https://codereview.chromium.org/337203003/diff/300001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_truetype_font_host.cc (right):

https://codereview.chromium.org/337203003/diff/300001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font_host.cc:74: font_ =
NULL;
On 2014/06/23 18:34:10, bbudge wrote:
> On 2014/06/23 18:21:12, piman wrote:
> > Why is this logic needed? It may not work the way you expect:
> > 1- the task could execute before font_ is reset, so the last reference would
> be
> > released on the main thread (io thread?)
> > 2- even if you "pass" the reference to the task, there is a temporary
> > base::Closure on this thread, that keeps a reference to the
> PepperTrueTypeFont,
> > that may outlive the task (for the same reason)
> > 
> > You could use SequencedTaskRunner::ReleaseSoon I suppose, but what is the
> reason
> > you need the PepperTrueTypeFont to be deleted on the blocking thread?
> 
> PepperTrueTypeFontLinux has to close its file descriptor. I was trying to do
> that on a blocking pool thread.

I see. IIRC close() doesn't block if you don't have writes pending, but if you
want to be safe, then maybe ReleaseSoon is the way to go (worth adding a
comment).

https://codereview.chromium.org/337203003/diff/300001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_truetype_font_linux.cc (right):

https://codereview.chromium.org/337203003/diff/300001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font_linux.cc:87: if
(!GetFontTable(fd_.get(),
On 2014/06/23 18:34:10, bbudge wrote:
> On 2014/06/23 18:21:12, piman wrote:
> > So, the fd here could be invalid in case of a malicious plugin: while the
new
> > code waits for the result before sending the GetTableTags/GetTable IPCs, a
> > malicious plugin may choose not to respect that.
> > 
> > I think you need to take this into account here, and return failure if the
fd
> is
> > invalid (and probably similar things on other platforms).
> > 
> > If that's the case, then I think you can skip the delaying logic on the
plugin
> > side by using SequencedWorkerPool::GetSequencedTaskRunner on the blocking
pool
> > (to ensure task ordering): if the font creation fails, the
> GetTableTags/GetTable
> > IPCs will fail too here. If it succeeds, IPC/post task ordering should
ensure
> > we'll get there only after Initialize has run.
> > 
> > I think it would make the code simpler on the plugin side, with minor
> complexity
> > on the host side.
> 
> I thought about using SequencedTaskRunner but it seems like that would
serialize
> all Get* calls. I was trying to allow them to occur in parallel since they're
> just reading the font data. It's probably not a big deal performance-wise. I
> agree it would be much simpler to use STR. Is there a way to just have a
> sequencing constraint on Initialize and the Get* tasks and allow the Get*
tasks
> to run in parallel?

I'm not sure if there is a way, it doesn't seem obvious from the API.

That said in the previous code the operations were already serial.

[bbudge, 2014-06-24 01:50:56]

https://codereview.chromium.org/337203003/diff/300001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_truetype_font_host.cc (right):

https://codereview.chromium.org/337203003/diff/300001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font_host.cc:74: font_ =
NULL;
On 2014/06/23 19:05:47, piman wrote:
> On 2014/06/23 18:34:10, bbudge wrote:
> > On 2014/06/23 18:21:12, piman wrote:
> > > Why is this logic needed? It may not work the way you expect:
> > > 1- the task could execute before font_ is reset, so the last reference
would
> > be
> > > released on the main thread (io thread?)
> > > 2- even if you "pass" the reference to the task, there is a temporary
> > > base::Closure on this thread, that keeps a reference to the
> > PepperTrueTypeFont,
> > > that may outlive the task (for the same reason)
> > > 
> > > You could use SequencedTaskRunner::ReleaseSoon I suppose, but what is the
> > reason
> > > you need the PepperTrueTypeFont to be deleted on the blocking thread?
> > 
> > PepperTrueTypeFontLinux has to close its file descriptor. I was trying to do
> > that on a blocking pool thread.
> 
> I see. IIRC close() doesn't block if you don't have writes pending, but if you
> want to be safe, then maybe ReleaseSoon is the way to go (worth adding a
> comment).

Done via ReleaseSoon on the task runner. + comment

https://codereview.chromium.org/337203003/diff/300001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_truetype_font_linux.cc (right):

https://codereview.chromium.org/337203003/diff/300001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font_linux.cc:87: if
(!GetFontTable(fd_.get(),
On 2014/06/23 19:05:47, piman wrote:
> On 2014/06/23 18:34:10, bbudge wrote:
> > On 2014/06/23 18:21:12, piman wrote:
> > > So, the fd here could be invalid in case of a malicious plugin: while the
> new
> > > code waits for the result before sending the GetTableTags/GetTable IPCs, a
> > > malicious plugin may choose not to respect that.
> > > 
> > > I think you need to take this into account here, and return failure if the
> fd
> > is
> > > invalid (and probably similar things on other platforms).
> > > 
> > > If that's the case, then I think you can skip the delaying logic on the
> plugin
> > > side by using SequencedWorkerPool::GetSequencedTaskRunner on the blocking
> pool
> > > (to ensure task ordering): if the font creation fails, the
> > GetTableTags/GetTable
> > > IPCs will fail too here. If it succeeds, IPC/post task ordering should
> ensure
> > > we'll get there only after Initialize has run.
> > > 
> > > I think it would make the code simpler on the plugin side, with minor
> > complexity
> > > on the host side.
> > 
> > I thought about using SequencedTaskRunner but it seems like that would
> serialize
> > all Get* calls. I was trying to allow them to occur in parallel since
they're
> > just reading the font data. It's probably not a big deal performance-wise. I
> > agree it would be much simpler to use STR. Is there a way to just have a
> > sequencing constraint on Initialize and the Get* tasks and allow the Get*
> tasks
> > to run in parallel?
> 
> I'm not sure if there is a way, it doesn't seem obvious from the API.
> 
> That said in the previous code the operations were already serial.

Switched to SequenceTaskRunner.

https://codereview.chromium.org/337203003/diff/300001/content/browser/rendere...
File content/browser/renderer_host/sandbox_ipc_linux.cc (left):

https://codereview.chromium.org/337203003/diff/300001/content/browser/rendere...
content/browser/renderer_host/sandbox_ipc_linux.cc:585:
FcPatternDestroy(pattern);
On 2014/06/23 18:34:52, Jorge Lucangeli Obes wrote:
> Reiterating my previous comment to test this under TSan.

Sorry I didn't address it earlier. I downloaded the PDF and verified, the
Chinese characters appear.

[Jorge Lucangeli Obes, 2014-06-24 01:53:58]

sandbox_ipc_linux.* lgtm. you might want to run content_browsertests under TSan
to make sure there's no threading issues, but since you're just moving code
around I don't expect any problems.

[piman, 2014-06-24 02:53:29]

https://codereview.chromium.org/337203003/diff/300001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_truetype_font_linux.cc (right):

https://codereview.chromium.org/337203003/diff/300001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font_linux.cc:87: if
(!GetFontTable(fd_.get(),
On 2014/06/24 01:50:56, bbudge wrote:
> On 2014/06/23 19:05:47, piman wrote:
> > On 2014/06/23 18:34:10, bbudge wrote:
> > > On 2014/06/23 18:21:12, piman wrote:
> > > > So, the fd here could be invalid in case of a malicious plugin: while
the
> > new
> > > > code waits for the result before sending the GetTableTags/GetTable IPCs,
a
> > > > malicious plugin may choose not to respect that.
> > > > 
> > > > I think you need to take this into account here, and return failure if
the
> > fd
> > > is
> > > > invalid (and probably similar things on other platforms).
> > > > 
> > > > If that's the case, then I think you can skip the delaying logic on the
> > plugin
> > > > side by using SequencedWorkerPool::GetSequencedTaskRunner on the
blocking
> > pool
> > > > (to ensure task ordering): if the font creation fails, the
> > > GetTableTags/GetTable
> > > > IPCs will fail too here. If it succeeds, IPC/post task ordering should
> > ensure
> > > > we'll get there only after Initialize has run.
> > > > 
> > > > I think it would make the code simpler on the plugin side, with minor
> > > complexity
> > > > on the host side.
> > > 
> > > I thought about using SequencedTaskRunner but it seems like that would
> > serialize
> > > all Get* calls. I was trying to allow them to occur in parallel since
> they're
> > > just reading the font data. It's probably not a big deal performance-wise.
I
> > > agree it would be much simpler to use STR. Is there a way to just have a
> > > sequencing constraint on Initialize and the Get* tasks and allow the Get*
> > tasks
> > > to run in parallel?
> > 
> > I'm not sure if there is a way, it doesn't seem obvious from the API.
> > 
> > That said in the previous code the operations were already serial.
> 
> Switched to SequenceTaskRunner.

You still need a check on fd_ here (and GetTable, and other platforms) for the
case where initialization fails but a GetTableTags request was already in the
pipeline. You'd get here with an invalid fd.

https://codereview.chromium.org/337203003/diff/340001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_truetype_font_host.cc (right):

https://codereview.chromium.org/337203003/diff/340001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font_host.cc:37:
pool->GetNamedSequenceToken("PepperTrueTypeFontHost"));
nit: do you really want all PepperTrueTypeFontHost to use the same token? You
could use one token per PepperTrueTypeFontHost instance by using
GetSequenceToken().

[jschuh, 2014-06-24 06:11:45]

I vaguely recall that palmer@ gave the original code a very thorough security
review. So, assuming I'm correct, I'd think he'd be best to review the code
move. (palmer@ feel free to call me a liar and tell me to go to hell if I'm
wrong.)

[bbudge, 2014-06-24 11:41:24]

On 2014/06/24 06:11:45, Justin Schuh wrote:
> I vaguely recall that palmer@ gave the original code a very thorough security
> review. So, assuming I'm correct, I'd think he'd be best to review the code
> move. (palmer@ feel free to call me a liar and tell me to go to hell if I'm
> wrong.)

You may be thinking of this change, which modified content::GetFontTable.

https://codereview.chromium.org/12433021/

[bbudge, 2014-06-24 12:21:11]

https://codereview.chromium.org/337203003/diff/300001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_truetype_font_linux.cc (right):

https://codereview.chromium.org/337203003/diff/300001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font_linux.cc:87: if
(!GetFontTable(fd_.get(),
On 2014/06/24 02:53:29, piman wrote:
> On 2014/06/24 01:50:56, bbudge wrote:
> > On 2014/06/23 19:05:47, piman wrote:
> > > On 2014/06/23 18:34:10, bbudge wrote:
> > > > On 2014/06/23 18:21:12, piman wrote:
> > > > > So, the fd here could be invalid in case of a malicious plugin: while
> the
> > > new
> > > > > code waits for the result before sending the GetTableTags/GetTable
IPCs,
> a
> > > > > malicious plugin may choose not to respect that.
> > > > > 
> > > > > I think you need to take this into account here, and return failure if
> the
> > > fd
> > > > is
> > > > > invalid (and probably similar things on other platforms).
> > > > > 
> > > > > If that's the case, then I think you can skip the delaying logic on
the
> > > plugin
> > > > > side by using SequencedWorkerPool::GetSequencedTaskRunner on the
> blocking
> > > pool
> > > > > (to ensure task ordering): if the font creation fails, the
> > > > GetTableTags/GetTable
> > > > > IPCs will fail too here. If it succeeds, IPC/post task ordering should
> > > ensure
> > > > > we'll get there only after Initialize has run.
> > > > > 
> > > > > I think it would make the code simpler on the plugin side, with minor
> > > > complexity
> > > > > on the host side.
> > > > 
> > > > I thought about using SequencedTaskRunner but it seems like that would
> > > serialize
> > > > all Get* calls. I was trying to allow them to occur in parallel since
> > they're
> > > > just reading the font data. It's probably not a big deal
performance-wise.
> I
> > > > agree it would be much simpler to use STR. Is there a way to just have a
> > > > sequencing constraint on Initialize and the Get* tasks and allow the
Get*
> > > tasks
> > > > to run in parallel?
> > > 
> > > I'm not sure if there is a way, it doesn't seem obvious from the API.
> > > 
> > > That said in the previous code the operations were already serial.
> > 
> > Switched to SequenceTaskRunner.
> 
> You still need a check on fd_ here (and GetTable, and other platforms) for the
> case where initialization fails but a GetTableTags request was already in the
> pipeline. You'd get here with an invalid fd.

Done for both Get* methods, here and for Win and Mac. I added a comment on the
base class header for implementors of any future subclasses.

https://codereview.chromium.org/337203003/diff/340001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_truetype_font_host.cc (right):

https://codereview.chromium.org/337203003/diff/340001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_truetype_font_host.cc:37:
pool->GetNamedSequenceToken("PepperTrueTypeFontHost"));
On 2014/06/24 02:53:29, piman wrote:
> nit: do you really want all PepperTrueTypeFontHost to use the same token? You
> could use one token per PepperTrueTypeFontHost instance by using
> GetSequenceToken().

Done.

[piman, 2014-06-24 18:17:21]

lgtm

[palmer, 2014-06-25 20:40:44]

LGTM.

[teravest, 2014-06-25 20:59:37]

lgtm

[bbudge, 2014-06-26 20:00:03]

The CQ bit was checked by bbudge@chromium.org

[bbudge, 2014-06-26 20:00:48]

On 2014/06/19 16:20:35, Jorge Lucangeli Obes wrote:
> On 2014/06/19 14:19:03, bbudge wrote:
> > teravest - /pepper stuff
> > jorgelo - browser/renderer_host/*_linux sandbox files.
> 
> Everything looks reasonable. Can you make sure
> https://code.google.com/p/chromium/issues/detail?id=381978 did not regress (it
> shouldn't, but I already regressed it once) and that content_browsertests pass
> under TSan?

I verified that content_browsertests pass under TSan on my personal machine.

[bbudge, 2014-06-26 20:00:48]

On 2014/06/19 16:20:35, Jorge Lucangeli Obes wrote:
> On 2014/06/19 14:19:03, bbudge wrote:
> > teravest - /pepper stuff
> > jorgelo - browser/renderer_host/*_linux sandbox files.
> 
> Everything looks reasonable. Can you make sure
> https://code.google.com/p/chromium/issues/detail?id=381978 did not regress (it
> shouldn't, but I already regressed it once) and that content_browsertests pass
> under TSan?

I verified that content_browsertests pass under TSan on my personal machine.

[commit-bot: I haz the power, 2014-06-26 20:01:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bbudge@chromium.org/337203003/360001

[commit-bot: I haz the power, 2014-06-26 21:28:27]

Change committed as 280107