Add a NaCl-Private interface for opening files in DIR_PNACL_COMPONENT.

Add a NaCl-Private interface for opening files in DIR_PNACL_COMPONENT.

This will later be used in the NaCl plugin to grab
"llc.nexe", and linker resources for PNaCl directly,
instead of using the URL-downloader interface to get these
from a chrome extension url.

BUG= http://code.google.com/p/nativeclient/issues/detail?id=2365
TEST= pnacl_file_host_unittest (just checks path sanity checks)

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

[jvoung (off chromium), 2012-06-25 23:56:11]

This is a newer version of:
http://codereview.chromium.org/9158005/

Sorry, I had lost my old client and wasn't sure how to reuse
issue numbers.  Would you guys be able to review this?


Mark, as for your comment about using
GetBrowserInterface(PPB_NACL_PRIVATE_INTERFACE) in the old
CL, I thought I would split the usage out to a different CL.

[jvoung (off chromium), 2012-06-27 19:52:00]

On 2012/06/25 23:56:11, jvoung (chromium) wrote:
> This is a newer version of:
> http://codereview.chromium.org/9158005/
> 
> Sorry, I had lost my old client and wasn't sure how to reuse
> issue numbers.  Would you guys be able to review this?
> 
> 
> Mark, as for your comment about using
> GetBrowserInterface(PPB_NACL_PRIVATE_INTERFACE) in the old
> CL, I thought I would split the usage out to a different CL.

For reference, example plumbing to use this is in:

http://codereview.chromium.org/10689012/

[jvoung (off chromium), 2012-06-29 19:47:28]

On 2012/06/27 19:52:00, jvoung (chromium) wrote:
> On 2012/06/25 23:56:11, jvoung (chromium) wrote:
> > This is a newer version of:
> > http://codereview.chromium.org/9158005/
> > 
> > Sorry, I had lost my old client and wasn't sure how to reuse
> > issue numbers.  Would you guys be able to review this?
> > 
> > 
> > Mark, as for your comment about using
> > GetBrowserInterface(PPB_NACL_PRIVATE_INTERFACE) in the old
> > CL, I thought I would split the usage out to a different CL.
> 
> For reference, example plumbing to use this is in:
> 
> http://codereview.chromium.org/10689012/

Is this approach okay, or should I look in the fewer-IPC version you suggested
earlier?

[jvoung (off chromium), 2012-07-10 18:35:14]

On 2012/06/29 19:47:28, jvoung (chromium) wrote:
> On 2012/06/27 19:52:00, jvoung (chromium) wrote:
> > On 2012/06/25 23:56:11, jvoung (chromium) wrote:
> > > This is a newer version of:
> > > http://codereview.chromium.org/9158005/
> > > 
> > > Sorry, I had lost my old client and wasn't sure how to reuse
> > > issue numbers.  Would you guys be able to review this?
> > > 
> > > 
> > > Mark, as for your comment about using
> > > GetBrowserInterface(PPB_NACL_PRIVATE_INTERFACE) in the old
> > > CL, I thought I would split the usage out to a different CL.
> > 
> > For reference, example plumbing to use this is in:
> > 
> > http://codereview.chromium.org/10689012/
> 
> Is this approach okay, or should I look in the fewer-IPC version you suggested
> earlier?

Should I ask Brett to review?

[sehr, 2012-07-11 16:35:35]

On 2012/07/10 18:35:14, jvoung (chromium) wrote:
> On 2012/06/29 19:47:28, jvoung (chromium) wrote:
> > On 2012/06/27 19:52:00, jvoung (chromium) wrote:
> > > On 2012/06/25 23:56:11, jvoung (chromium) wrote:
> > > > This is a newer version of:
> > > > http://codereview.chromium.org/9158005/
> > > > 
> > > > Sorry, I had lost my old client and wasn't sure how to reuse
> > > > issue numbers.  Would you guys be able to review this?
> > > > 
> > > > 
> > > > Mark, as for your comment about using
> > > > GetBrowserInterface(PPB_NACL_PRIVATE_INTERFACE) in the old
> > > > CL, I thought I would split the usage out to a different CL.
> > > 
> > > For reference, example plumbing to use this is in:
> > > 
> > > http://codereview.chromium.org/10689012/
> > 
> > Is this approach okay, or should I look in the fewer-IPC version you
suggested
> > earlier?
> 
> Should I ask Brett to review?

Sounds right to add brettw.

[jvoung (off chromium), 2012-07-11 23:18:04]

Could you take a look then, Brett? Thanks!

[brettw, 2012-07-12 19:55:16]

http://codereview.chromium.org/10662006/diff/17001/chrome/browser/nacl_host/p...
File chrome/browser/nacl_host/pnacl_file_host.cc (right):

http://codereview.chromium.org/10662006/diff/17001/chrome/browser/nacl_host/p...
chrome/browser/nacl_host/pnacl_file_host.cc:47: std::string filename,
Nit: const string&

http://codereview.chromium.org/10662006/diff/17001/chrome/browser/nacl_host/p...
chrome/browser/nacl_host/pnacl_file_host.cc:99: // Do *some* validation, even
though this should only be used
Actually we need to validate very carefully. This is a call from a sandboxed
process to open a file on disk, so we need to be sure sandbox integrity isn't
broken. I'll add Justin to audit.

http://codereview.chromium.org/10662006/diff/17001/chrome/renderer/pepper/ppb...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

http://codereview.chromium.org/10662006/diff/17001/chrome/renderer/pepper/ppb...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:50:
base::LazyInstance<scoped_refptr<IPC::SyncMessageFilter> >
Can you add a comment here about what this is for? You're using this variable
and I don't fully understand what the goal here is.

http://codereview.chromium.org/10662006/diff/17001/ppapi/api/private/finish_w...
File ppapi/api/private/finish_writing_these/ppb_nacl_private.idl (right):

http://codereview.chromium.org/10662006/diff/17001/ppapi/api/private/finish_w...
ppapi/api/private/finish_writing_these/ppb_nacl_private.idl:9: interface
PPB_NaCl_Private_0_5 {
Can you update the revision number here?

http://codereview.chromium.org/10662006/diff/17001/ppapi/c/private/ppb_nacl_p...
File ppapi/c/private/ppb_nacl_private.h (right):

http://codereview.chromium.org/10662006/diff/17001/ppapi/c/private/ppb_nacl_p...
ppapi/c/private/ppb_nacl_private.h:13: #define PPB_NACL_PRIVATE_INTERFACE
"PPB_NaCl(Private);0.5"
Can you also update the version number here?

[jvoung (off chromium), 2012-07-12 23:06:09]

http://codereview.chromium.org/10662006/diff/17001/chrome/browser/nacl_host/p...
File chrome/browser/nacl_host/pnacl_file_host.cc (right):

http://codereview.chromium.org/10662006/diff/17001/chrome/browser/nacl_host/p...
chrome/browser/nacl_host/pnacl_file_host.cc:47: std::string filename,
On 2012/07/12 19:55:16, brettw wrote:
> Nit: const string&

I wasn't sure if it was okay to use "&" in combination with Bind(), but changed
for now.

http://codereview.chromium.org/10662006/diff/17001/chrome/browser/nacl_host/p...
chrome/browser/nacl_host/pnacl_file_host.cc:99: // Do *some* validation, even
though this should only be used
On 2012/07/12 19:55:16, brettw wrote:
> Actually we need to validate very carefully. This is a call from a sandboxed
> process to open a file on disk, so we need to be sure sandbox integrity isn't
> broken. I'll add Justin to audit.

Ah, that's right -- removed comment.

http://codereview.chromium.org/10662006/diff/17001/chrome/renderer/pepper/ppb...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

http://codereview.chromium.org/10662006/diff/17001/chrome/renderer/pepper/ppb...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:50:
base::LazyInstance<scoped_refptr<IPC::SyncMessageFilter> >
On 2012/07/12 19:55:16, brettw wrote:
> Can you add a comment here about what this is for? You're using this variable
> and I don't fully understand what the goal here is.

Added a comment for the original goal (to send synchronous requests like
LaunchSelLdr from a background thread).

I've not actually tried to exercise that with this request yet, so I can take
out the use of that if you prefer?

http://codereview.chromium.org/10662006/diff/17001/ppapi/api/private/finish_w...
File ppapi/api/private/finish_writing_these/ppb_nacl_private.idl (right):

http://codereview.chromium.org/10662006/diff/17001/ppapi/api/private/finish_w...
ppapi/api/private/finish_writing_these/ppb_nacl_private.idl:9: interface
PPB_NaCl_Private_0_5 {
On 2012/07/12 19:55:16, brettw wrote:
> Can you update the revision number here?

Done.

http://codereview.chromium.org/10662006/diff/17001/ppapi/c/private/ppb_nacl_p...
File ppapi/c/private/ppb_nacl_private.h (right):

http://codereview.chromium.org/10662006/diff/17001/ppapi/c/private/ppb_nacl_p...
ppapi/c/private/ppb_nacl_private.h:13: #define PPB_NACL_PRIVATE_INTERFACE
"PPB_NaCl(Private);0.5"
On 2012/07/12 19:55:16, brettw wrote:
> Can you also update the version number here?

Done.

[jvoung (off chromium), 2012-07-19 22:33:47]

ping

On 2012/07/12 23:06:09, jvoung (chromium) wrote:
>
http://codereview.chromium.org/10662006/diff/17001/chrome/browser/nacl_host/p...
> File chrome/browser/nacl_host/pnacl_file_host.cc (right):
> 
>
http://codereview.chromium.org/10662006/diff/17001/chrome/browser/nacl_host/p...
> chrome/browser/nacl_host/pnacl_file_host.cc:47: std::string filename,
> On 2012/07/12 19:55:16, brettw wrote:
> > Nit: const string&
> 
> I wasn't sure if it was okay to use "&" in combination with Bind(), but
changed
> for now.
> 
>
http://codereview.chromium.org/10662006/diff/17001/chrome/browser/nacl_host/p...
> chrome/browser/nacl_host/pnacl_file_host.cc:99: // Do *some* validation, even
> though this should only be used
> On 2012/07/12 19:55:16, brettw wrote:
> > Actually we need to validate very carefully. This is a call from a sandboxed
> > process to open a file on disk, so we need to be sure sandbox integrity
isn't
> > broken. I'll add Justin to audit.
> 
> Ah, that's right -- removed comment.
> 
>
http://codereview.chromium.org/10662006/diff/17001/chrome/renderer/pepper/ppb...
> File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):
> 
>
http://codereview.chromium.org/10662006/diff/17001/chrome/renderer/pepper/ppb...
> chrome/renderer/pepper/ppb_nacl_private_impl.cc:50:
> base::LazyInstance<scoped_refptr<IPC::SyncMessageFilter> >
> On 2012/07/12 19:55:16, brettw wrote:
> > Can you add a comment here about what this is for? You're using this
variable
> > and I don't fully understand what the goal here is.
> 
> Added a comment for the original goal (to send synchronous requests like
> LaunchSelLdr from a background thread).
> 
> I've not actually tried to exercise that with this request yet, so I can take
> out the use of that if you prefer?
> 
>
http://codereview.chromium.org/10662006/diff/17001/ppapi/api/private/finish_w...
> File ppapi/api/private/finish_writing_these/ppb_nacl_private.idl (right):
> 
>
http://codereview.chromium.org/10662006/diff/17001/ppapi/api/private/finish_w...
> ppapi/api/private/finish_writing_these/ppb_nacl_private.idl:9: interface
> PPB_NaCl_Private_0_5 {
> On 2012/07/12 19:55:16, brettw wrote:
> > Can you update the revision number here?
> 
> Done.
> 
>
http://codereview.chromium.org/10662006/diff/17001/ppapi/c/private/ppb_nacl_p...
> File ppapi/c/private/ppb_nacl_private.h (right):
> 
>
http://codereview.chromium.org/10662006/diff/17001/ppapi/c/private/ppb_nacl_p...
> ppapi/c/private/ppb_nacl_private.h:13: #define PPB_NACL_PRIVATE_INTERFACE
> "PPB_NaCl(Private);0.5"
> On 2012/07/12 19:55:16, brettw wrote:
> > Can you also update the version number here?
> 
> Done.

[jschuh, 2012-07-19 22:35:45]

Oops. We went through this in person and I'm okay with it from a security
perspective. lgtm

[jvoung (off chromium), 2012-07-20 18:10:32]

np thanks -- brettw, is the rest okay?

[brettw, 2012-07-23 18:09:02]

lgtm

http://codereview.chromium.org/10662006/diff/17003/chrome/browser/nacl_host/p...
File chrome/browser/nacl_host/pnacl_file_host_unittest.cc (right):

http://codereview.chromium.org/10662006/diff/17003/chrome/browser/nacl_host/p...
chrome/browser/nacl_host/pnacl_file_host_unittest.cc:18: class PnaclFileHostTest
: public ::testing::Test {
Since this class doesn't do anything you can remove it and use TEST(...) instead
of TEST_F below.

http://codereview.chromium.org/10662006/diff/17003/chrome/renderer/pepper/ppb...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

http://codereview.chromium.org/10662006/diff/17003/chrome/renderer/pepper/ppb...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:284: base::PlatformFile handle =
IPC::PlatformFileForTransitToPlatformFile(
Random suggestion: I'd find this easier to read if you broke the line after the
= instead (to keep the arg with the function name).

[jvoung (off chromium), 2012-07-23 19:59:03]

Thanks!

http://codereview.chromium.org/10662006/diff/17003/chrome/browser/nacl_host/p...
File chrome/browser/nacl_host/pnacl_file_host_unittest.cc (right):

http://codereview.chromium.org/10662006/diff/17003/chrome/browser/nacl_host/p...
chrome/browser/nacl_host/pnacl_file_host_unittest.cc:18: class PnaclFileHostTest
: public ::testing::Test {
On 2012/07/23 18:09:02, brettw wrote:
> Since this class doesn't do anything you can remove it and use TEST(...)
instead
> of TEST_F below.

Done.

http://codereview.chromium.org/10662006/diff/17003/chrome/renderer/pepper/ppb...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

http://codereview.chromium.org/10662006/diff/17003/chrome/renderer/pepper/ppb...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:284: base::PlatformFile handle =
IPC::PlatformFileForTransitToPlatformFile(
On 2012/07/23 18:09:02, brettw wrote:
> Random suggestion: I'd find this easier to read if you broke the line after
the
> = instead (to keep the arg with the function name).

Ah, yes -- Done.

[commit-bot: I haz the power, 2012-07-23 19:59:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jvoung@chromium.org/10662006/33001

[commit-bot: I haz the power, 2012-07-23 19:59:58]

Presubmit check for 10662006-33001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
Missing PPAPI header, no change or skipped generation?

***************
  ppapi/c/private/finish_writing_these/ppb_nacl_private.idl
***************

Missing PPAPI IDL for private interface, please generate IDL:

***************
  ppapi/c/private/ppb_nacl_private.h
***************

Presubmit checks took 2.3s to calculate.

[brettw, 2012-07-23 20:05:08]

I think you can ignore this presubmit check, there's no IDL for this interface.