Implement open_resource in non-SFI mode.

Implement open_resource in non-SFI mode.

This CL implements open_resource() in non-SFI mode.
1) Introduced a new sync message PpapiHostMsg_OpenResource
and its handlers.
2) Implement async version of OpenManifestEntry. As IPC's
handler is called on renderer's main thread, otherwise it
causes deadlock.

TEST=Run trybots.
BUG=358431
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267962

[Junichi Uekawa, 2014-04-23 22:19:30]

https://codereview.chromium.org/249183004/diff/1/components/nacl.gyp
File components/nacl.gyp (right):

https://codereview.chromium.org/249183004/diff/1/components/nacl.gyp#newcode240
components/nacl.gyp:240: 'nacl/loader/nonsfi/irt_resource_open.cc',
I think you forgot to inlcude irt_resource_open.cc and irt_manifest.cc itself.

[hidehiko, 2014-04-24 16:07:31]

Hi Mark, Dave,

Could you take a look?

Thanks in advance,
- hidehiko

https://codereview.chromium.org/249183004/diff/1/components/nacl.gyp
File components/nacl.gyp (right):

https://codereview.chromium.org/249183004/diff/1/components/nacl.gyp#newcode240
components/nacl.gyp:240: 'nacl/loader/nonsfi/irt_resource_open.cc',
On 2014/04/23 22:19:30, uekawa wrote:
> I think you forgot to inlcude irt_resource_open.cc and irt_manifest.cc itself.

Oops. Done.

[teravest, 2014-04-24 20:00:52]

https://codereview.chromium.org/249183004/diff/80001/components/nacl/renderer...
File components/nacl/renderer/manifest_service_channel.cc (right):

https://codereview.chromium.org/249183004/diff/80001/components/nacl/renderer...
components/nacl/renderer/manifest_service_channel.cc:84:
IPC::GetFileHandleForProcess(
You should use BrokerGetFileHandleForProcess here instead of
IPC::GetFileHandleForProcess.

It doesn't matter right now, since Windows isn't supported, but this won't work
if/when someone wants to add Windows support later.

https://codereview.chromium.org/249183004/diff/80001/components/nacl/renderer...
components/nacl/renderer/manifest_service_channel.cc:91: // a same thread.
s/called only/only called/
s/a/the/

https://codereview.chromium.org/249183004/diff/80001/ppapi/api/private/ppb_na...
File ppapi/api/private/ppb_nacl_private.idl (right):

https://codereview.chromium.org/249183004/diff/80001/ppapi/api/private/ppb_na...
ppapi/api/private/ppb_nacl_private.idl:149: interface PPB_ManifestService {
nit: I'd prefer that this stay as PP_ManifestService.

PPB_ManifestService suggests that the plugin is sending methods to the browser
to do work, but this is more like what a PPP_ interface would be. I think just
leaving the PP_ prefix is fine, though.

https://codereview.chromium.org/249183004/diff/80001/ppapi/nacl_irt/irt_manif...
File ppapi/nacl_irt/irt_manifest.cc (right):

https://codereview.chromium.org/249183004/diff/80001/ppapi/nacl_irt/irt_manif...
ppapi/nacl_irt/irt_manifest.cc:22: *fd = -1;
Why set |fd| on failure? It seems unnecessary.

https://codereview.chromium.org/249183004/diff/80001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/249183004/diff/80001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:331: entry_key, info,
&op_complete, callback);
Do we need to pass a callback into OpenManifestEntryResource here?
It looks like OpenManifestEntry_MainThreadContinuation is effectively
synchronous.

Why does OpenManifestEntryAsync take a callback at all? I feel like I'm missing
something.

This is calling OpenManifestEntry_MainThreadContinuation, so I don't think
there's any call having to happen across threads...

[hidehiko, 2014-04-25 05:53:54]

Thank you for review, Justin. PTAL.

Julien, could you also take a look for *_message.h, and
components/nacl/renderer/DEPS as an owner?

https://codereview.chromium.org/249183004/diff/80001/components/nacl/renderer...
File components/nacl/renderer/manifest_service_channel.cc (right):

https://codereview.chromium.org/249183004/diff/80001/components/nacl/renderer...
components/nacl/renderer/manifest_service_channel.cc:84:
IPC::GetFileHandleForProcess(
On 2014/04/24 20:00:52, teravest wrote:
> You should use BrokerGetFileHandleForProcess here instead of
> IPC::GetFileHandleForProcess.
> 
> It doesn't matter right now, since Windows isn't supported, but this won't
work
> if/when someone wants to add Windows support later. 

Done.

https://codereview.chromium.org/249183004/diff/80001/components/nacl/renderer...
components/nacl/renderer/manifest_service_channel.cc:91: // a same thread.
On 2014/04/24 20:00:52, teravest wrote:
> s/called only/only called/
> s/a/the/

Done.

https://codereview.chromium.org/249183004/diff/80001/ppapi/api/private/ppb_na...
File ppapi/api/private/ppb_nacl_private.idl (right):

https://codereview.chromium.org/249183004/diff/80001/ppapi/api/private/ppb_na...
ppapi/api/private/ppb_nacl_private.idl:149: interface PPB_ManifestService {
On 2014/04/24 20:00:52, teravest wrote:
> nit: I'd prefer that this stay as PP_ManifestService.
> 
> PPB_ManifestService suggests that the plugin is sending methods to the browser
> to do work, but this is more like what a PPP_ interface would be. I think just
> leaving the PP_ prefix is fine, though.

PPKind.ChoosePPFunc in idl_gen_wrapper.py doesn't work with PP_. So, let's use
PPP_ instead, then.

https://codereview.chromium.org/249183004/diff/80001/ppapi/nacl_irt/irt_manif...
File ppapi/nacl_irt/irt_manifest.cc (right):

https://codereview.chromium.org/249183004/diff/80001/ppapi/nacl_irt/irt_manif...
ppapi/nacl_irt/irt_manifest.cc:22: *fd = -1;
On 2014/04/24 20:00:52, teravest wrote:
> Why set |fd| on failure? It seems unnecessary.

Ok, then let's move this to ManifestService::OpenResource().
As this is called from outside, returning -1 should be safer.

https://codereview.chromium.org/249183004/diff/80001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/249183004/diff/80001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:331: entry_key, info,
&op_complete, callback);
On 2014/04/24 20:00:52, teravest wrote:
> Do we need to pass a callback into OpenManifestEntryResource here?
> It looks like OpenManifestEntry_MainThreadContinuation is effectively
> synchronous.
> 
> Why does OpenManifestEntryAsync take a callback at all? I feel like I'm
missing
> something.
> 
> This is calling OpenManifestEntry_MainThreadContinuation, so I don't think
> there's any call having to happen across threads...

OpenManifestEntry can be called only on a non-main thread
(if it is called on the main thread, it'd just hung up),
but the ManifestService::OpenResource is called on the main thread.

The implementation of OpenManifestEntry (i.e.
OpenManifestEntry_MainThreadContinuation and
StreamAsFile_MainThreadContinuation) is async call (pls see also around
L430-L436), so that we need its completion callback.

The reason why it's an async call is that, in some cases, opening the stream
triggers async IPC with the browser, so it needs to wait for its reply.

[teravest, 2014-04-25 20:44:01]

Thanks for explaining why OpenManifestEntry has to be asynchronous in this case.

I wish that Manifest was already moved over to the renderer from the trusted
plugin; this would be much simpler if we could resolve the key there and do the
fetch instead of having to jump down to the trusted plugin.

I'm not crazy about all the callback jumping here, but I can clean this up as
part of the refactor. Sorry you had to write such tricky code here.

I think a better change would call down to the trusted plugin for ResolveKey()
and PnaclUrls::IsPnaclComponent(), but in the "normal" file case, use
WebURLLoader in the renderer to prevent the need for the async logic.

However, I know there's pressure to get this into M36, so I'm fine with this
approach, knowing that it'll get cleaned up soon.

https://codereview.chromium.org/249183004/diff/100001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/249183004/diff/100001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:201: // When succeeded, the
callback will be called, and the data will be freed
Would it be simpler to always invoke the callback?

OpenResource could invoke the callback with PP_ERROR_ABORTED instead of
returning false. Then there are fewer paths to follow.

[dmichael (off chromium), 2014-04-25 21:06:58]

https://codereview.chromium.org/249183004/diff/100001/components/nacl/rendere...
File components/nacl/renderer/manifest_service_channel.cc (right):

https://codereview.chromium.org/249183004/diff/100001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.cc:79: IPC::Message* reply,
int32_t pp_error,
style nit: Parameters should be either all on one line or each have their own
line in a function declaration or definition.

https://codereview.chromium.org/249183004/diff/100001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/249183004/diff/100001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:193: &data->platform_file,
nit: I think parens around data->platform_file would help readability here.

https://codereview.chromium.org/249183004/diff/100001/ppapi/api/private/ppb_n...
File ppapi/api/private/ppb_nacl_private.idl (right):

https://codereview.chromium.org/249183004/diff/100001/ppapi/api/private/ppb_n...
ppapi/api/private/ppb_nacl_private.idl:162: [in] PP_CompletionCallback
callback);
We never use completion callbacks in this direction. I think I would prefer a
new PPB function on PPB_NaCl_Private instead.

https://codereview.chromium.org/249183004/diff/100001/ppapi/nacl_irt/irt_mani...
File ppapi/nacl_irt/irt_manifest.h (right):

https://codereview.chromium.org/249183004/diff/100001/ppapi/nacl_irt/irt_mani...
ppapi/nacl_irt/irt_manifest.h:12: // The implementation of irt_open_resource()
based on ManifestService.
This ought to document what it does and what the return value means.

Also, it's probably more appropriate to rename the "file" parameter to "url"?

https://codereview.chromium.org/249183004/diff/100001/ppapi/nacl_irt/manifest...
File ppapi/nacl_irt/manifest_service.cc (right):

https://codereview.chromium.org/249183004/diff/100001/ppapi/nacl_irt/manifest...
ppapi/nacl_irt/manifest_service.cc:39: std::string(kFilePrefix) + file,
&ipc_fd))) {
Why this magical files/ prefix? Shouldn't "file" just be a relative URL?

https://codereview.chromium.org/249183004/diff/100001/ppapi/nacl_irt/manifest...
File ppapi/nacl_irt/manifest_service.h (right):

https://codereview.chromium.org/249183004/diff/100001/ppapi/nacl_irt/manifest...
ppapi/nacl_irt/manifest_service.h:33: bool OpenResource(const char* file, int*
fd);
nit: file->url ?

https://codereview.chromium.org/249183004/diff/100001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/249183004/diff/100001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:83: // supproted on
windows.
supproted->supported

https://codereview.chromium.org/249183004/diff/100001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:96:
pp::CompletionCallback pp_completion_callback() {
I find this confusing, that this class deals with 2 PP_CompletionCallbacks.
1) callback_ appears to be the one provided by Chrome when it invoked
OpenResource. We don't use PP_CompletionCallback to make calls in to the
browser, so I would prefer just a new function on PPB_NaCl_Private (it can
disambiguate with a user_data parameter or something).
2) The one returned by pp_completion_callback, which seems to be just a way to
provide a function pointer and some associated stuff? It's not actually passed
to Pepper. If you use a new PPB function to talk back to Chrome, you probably
don't need this one anymore.

https://codereview.chromium.org/249183004/diff/100001/ppapi/ppapi_nacl_test_c...
File ppapi/ppapi_nacl_test_common.gypi (right):

https://codereview.chromium.org/249183004/diff/100001/ppapi/ppapi_nacl_test_c...
ppapi/ppapi_nacl_test_common.gypi:67: # Nonsfi pnacl copy is convered below.
Currently, these are exclusive.
convered->covered

[Mark Seaborn, 2014-04-26 00:18:58]

https://codereview.chromium.org/249183004/diff/100001/chrome/test/data/nacl/n...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

https://codereview.chromium.org/249183004/diff/100001/chrome/test/data/nacl/n...
chrome/test/data/nacl/nacl_test_data.gyp:1116: # create_nmf.py doesn't support
nonsfi.
How hard would it be to fix that, so that the Gyp files don't get more
complicated?

https://codereview.chromium.org/249183004/diff/100001/components/nacl/rendere...
File components/nacl/renderer/manifest_service_channel.cc (right):

https://codereview.chromium.org/249183004/diff/100001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.cc:86: platform_file,
PeerHandle(), true)));
On Unix, the PeerHandle() arg is ignored...  You could use
base::FileDescriptor() instead of BrokerGetFileHandleForProcess().

This doesn't compile on Windows.  BrokerGetFileHandleForProcess() expects a
Windows process ID, not a Windows process handle.

https://codereview.chromium.org/249183004/diff/100001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.cc:94:
base::OpenPrivilegedProcessHandle(channel_->peer_pid(), &peer_handle_);
This only does something interesting on Windows, so I wonder if this is left
over from an earlier version of your code for SFI NaCl, or if you added it after
preparing this code for Non-SFI NaCl?

I ask because I doubt this call works in a Windows sandboxed renderer process.

peer_handle_ shouldn't be needed on Unix.

Can you remove any of the complications for building this on Windows, please? 
Since we don't need this on non-Linux systems, you can simplify the code for
now.  You can put in #ifs if necessary.

https://codereview.chromium.org/249183004/diff/100001/ppapi/nacl_irt/irt_mani...
File ppapi/nacl_irt/irt_manifest.cc (right):

https://codereview.chromium.org/249183004/diff/100001/ppapi/nacl_irt/irt_mani...
ppapi/nacl_irt/irt_manifest.cc:22: return EIO;
Does it work to #include
native_client/src/trusted/service_runtime/include/sys/errno.h and use
NACL_ABI_EIO and NACL_ABI_ENOENT?

If not, could you just add a TODO to use those names?

https://codereview.chromium.org/249183004/diff/100001/ppapi/nacl_irt/manifest...
File ppapi/nacl_irt/manifest_service.cc (right):

https://codereview.chromium.org/249183004/diff/100001/ppapi/nacl_irt/manifest...
ppapi/nacl_irt/manifest_service.cc:39: std::string(kFilePrefix) + file,
&ipc_fd))) {
On 2014/04/25 21:06:58, dmichael wrote:
> Why this magical files/ prefix? Shouldn't "file" just be a relative URL?

This is matching existing behaviour, I guess (which is not well-documented). 
Note that this is not a URL.

https://codereview.chromium.org/249183004/diff/100001/ppapi/nacl_irt/manifest...
File ppapi/nacl_irt/manifest_service.h (right):

https://codereview.chromium.org/249183004/diff/100001/ppapi/nacl_irt/manifest...
ppapi/nacl_irt/manifest_service.h:33: bool OpenResource(const char* file, int*
fd);
On 2014/04/25 21:06:58, dmichael wrote:
> nit: file->url ?

It's not actually a URL.  It's a key to look up in the dict in the manifest
file.

https://codereview.chromium.org/249183004/diff/100001/ppapi/ppapi_nacl_test_c...
File ppapi/ppapi_nacl_test_common.gypi (right):

https://codereview.chromium.org/249183004/diff/100001/ppapi/ppapi_nacl_test_c...
ppapi/ppapi_nacl_test_common.gypi:22: 'nacl_pnacl_nonsfi_out_dir':
'<(PRODUCT_DIR)/>(nexe_destination_dir)/nonsfi',
Should be "nacl_pnacl_newlib_nonsfi_out_dir" for consistency.  ("newlib" and
"nonsfi" are not alternatives to each other.)

Actually, why should this use a different output directory from
nacl_pnacl_newlib_out_dir?  Can't they use the same directory?

[hidehiko, 2014-04-28 08:44:26]

Justin, I agree with you that we can do better in theory.
I'm happy for working on clean up in later CLs, but I'd like to focus on
implementing open_resource in non-SFI mode in this CL.
Thank you for understanding the situation.

Mark, Dave, Justin, could you take another look?

Thanks,
- hidehiko

https://codereview.chromium.org/249183004/diff/100001/chrome/test/data/nacl/n...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

https://codereview.chromium.org/249183004/diff/100001/chrome/test/data/nacl/n...
chrome/test/data/nacl/nacl_test_data.gyp:1116: # create_nmf.py doesn't support
nonsfi.
On 2014/04/26 00:18:59, Mark Seaborn wrote:
> How hard would it be to fix that, so that the Gyp files don't get more
> complicated?

It sounds not easy to me.
create_nmf.py parses elf binary to detect its architecture.
For files in "program" section, we probably can add some heuristics to check if
it is for SFI or non-SFI, like checking if ehdr.e_type == ET_DYN, etc.
However, we also need the same thing for .so files, where ehdr.e_type check
doesn't work.
Also, I'm not very sure if checking e_type is appropriate way. Any thought?

Anyway, I'd like to split the task to support non-SFI on create_nmf from this
CL.

https://codereview.chromium.org/249183004/diff/100001/components/nacl/rendere...
File components/nacl/renderer/manifest_service_channel.cc (right):

https://codereview.chromium.org/249183004/diff/100001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.cc:79: IPC::Message* reply,
int32_t pp_error,
On 2014/04/25 21:06:58, dmichael wrote:
> style nit: Parameters should be either all on one line or each have their own
> line in a function declaration or definition.

Done.

https://codereview.chromium.org/249183004/diff/100001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.cc:86: platform_file,
PeerHandle(), true)));
On 2014/04/26 00:18:59, Mark Seaborn wrote:
> On Unix, the PeerHandle() arg is ignored...  You could use
> base::FileDescriptor() instead of BrokerGetFileHandleForProcess().
> 
> This doesn't compile on Windows.  BrokerGetFileHandleForProcess() expects a
> Windows process ID, not a Windows process handle.

Ok, then, let's ifdef.
I just tried to keep the sane platform-neutral manner in this class rather than
supporting on Windows.

If ifdef is OK, I'm fine with it. Let's revisit here, later.

https://codereview.chromium.org/249183004/diff/100001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.cc:94:
base::OpenPrivilegedProcessHandle(channel_->peer_pid(), &peer_handle_);
On 2014/04/26 00:18:59, Mark Seaborn wrote:
> This only does something interesting on Windows, so I wonder if this is left
> over from an earlier version of your code for SFI NaCl, or if you added it
after
> preparing this code for Non-SFI NaCl?
> 
> I ask because I doubt this call works in a Windows sandboxed renderer process.
> 
> peer_handle_ shouldn't be needed on Unix.
> 
> Can you remove any of the complications for building this on Windows, please? 
> Since we don't need this on non-Linux systems, you can simplify the code for
> now.  You can put in #ifs if necessary.

Removed.

https://codereview.chromium.org/249183004/diff/100001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/249183004/diff/100001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:193: &data->platform_file,
On 2014/04/25 21:06:58, dmichael wrote:
> nit: I think parens around data->platform_file would help readability here.

Acknowledged.

https://codereview.chromium.org/249183004/diff/100001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:201: // When succeeded, the
callback will be called, and the data will be freed
On 2014/04/25 20:44:02, teravest wrote:
> Would it be simpler to always invoke the callback?
> 
> OpenResource could invoke the callback with PP_ERROR_ABORTED instead of
> returning false. Then there are fewer paths to follow.

Done.

https://codereview.chromium.org/249183004/diff/100001/ppapi/api/private/ppb_n...
File ppapi/api/private/ppb_nacl_private.idl (right):

https://codereview.chromium.org/249183004/diff/100001/ppapi/api/private/ppb_n...
ppapi/api/private/ppb_nacl_private.idl:162: [in] PP_CompletionCallback
callback);
On 2014/04/25 21:06:58, dmichael wrote:
> We never use completion callbacks in this direction. I think I would prefer a
> new PPB function on PPB_NaCl_Private instead.

Done.

https://codereview.chromium.org/249183004/diff/100001/ppapi/nacl_irt/irt_mani...
File ppapi/nacl_irt/irt_manifest.cc (right):

https://codereview.chromium.org/249183004/diff/100001/ppapi/nacl_irt/irt_mani...
ppapi/nacl_irt/irt_manifest.cc:22: return EIO;
On 2014/04/26 00:18:59, Mark Seaborn wrote:
> Does it work to #include
> native_client/src/trusted/service_runtime/include/sys/errno.h and use
> NACL_ABI_EIO and NACL_ABI_ENOENT?
> 
> If not, could you just add a TODO to use those names?

Done.

https://codereview.chromium.org/249183004/diff/100001/ppapi/nacl_irt/irt_mani...
File ppapi/nacl_irt/irt_manifest.h (right):

https://codereview.chromium.org/249183004/diff/100001/ppapi/nacl_irt/irt_mani...
ppapi/nacl_irt/irt_manifest.h:12: // The implementation of irt_open_resource()
based on ManifestService.
On 2014/04/25 21:06:58, dmichael wrote:
> This ought to document what it does and what the return value means.
> 
> Also, it's probably more appropriate to rename the "file" parameter to "url"?

Updated the comment.

As Mark said, the first param is not an actual url, but key.
Please let me keep it as is to be consistent with irt_open_resource().

https://codereview.chromium.org/249183004/diff/100001/ppapi/nacl_irt/manifest...
File ppapi/nacl_irt/manifest_service.cc (right):

https://codereview.chromium.org/249183004/diff/100001/ppapi/nacl_irt/manifest...
ppapi/nacl_irt/manifest_service.cc:39: std::string(kFilePrefix) + file,
&ipc_fd))) {
On 2014/04/26 00:18:59, Mark Seaborn wrote:
> On 2014/04/25 21:06:58, dmichael wrote:
> > Why this magical files/ prefix? Shouldn't "file" just be a relative URL?
> 
> This is matching existing behaviour, I guess (which is not well-documented). 
> Note that this is not a URL.

Exactly as Mark said, this is just to be compatible with the current NaCl (i.e.
NaCl in SFI-mode).
IMHO, eventually we can clean this up, but at least I'd like to split the task
from this CL.

https://codereview.chromium.org/249183004/diff/100001/ppapi/nacl_irt/manifest...
File ppapi/nacl_irt/manifest_service.h (right):

https://codereview.chromium.org/249183004/diff/100001/ppapi/nacl_irt/manifest...
ppapi/nacl_irt/manifest_service.h:33: bool OpenResource(const char* file, int*
fd);
On 2014/04/26 00:18:59, Mark Seaborn wrote:
> On 2014/04/25 21:06:58, dmichael wrote:
> > nit: file->url ?
> 
> It's not actually a URL.  It's a key to look up in the dict in the manifest
> file.

Acknowledged.

https://codereview.chromium.org/249183004/diff/100001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/249183004/diff/100001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:83: // supproted on
windows.
On 2014/04/25 21:06:58, dmichael wrote:
> supproted->supported

Done.

https://codereview.chromium.org/249183004/diff/100001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:96:
pp::CompletionCallback pp_completion_callback() {
On 2014/04/25 21:06:58, dmichael wrote:
> I find this confusing, that this class deals with 2 PP_CompletionCallbacks.
> 1) callback_ appears to be the one provided by Chrome when it invoked
> OpenResource. We don't use PP_CompletionCallback to make calls in to the
> browser, so I would prefer just a new function on PPB_NaCl_Private (it can
> disambiguate with a user_data parameter or something).
> 2) The one returned by pp_completion_callback, which seems to be just a way to
> provide a function pointer and some associated stuff? It's not actually passed
> to Pepper. If you use a new PPB function to talk back to Chrome, you probably
> don't need this one anymore.

Done, but to be honest it looks slightly layer-violation to me. I think we can
clean them up, when we switch the SFI-mode irt_open_resource() to this
implementation later, though.

https://codereview.chromium.org/249183004/diff/100001/ppapi/ppapi_nacl_test_c...
File ppapi/ppapi_nacl_test_common.gypi (right):

https://codereview.chromium.org/249183004/diff/100001/ppapi/ppapi_nacl_test_c...
ppapi/ppapi_nacl_test_common.gypi:22: 'nacl_pnacl_nonsfi_out_dir':
'<(PRODUCT_DIR)/>(nexe_destination_dir)/nonsfi',
On 2014/04/26 00:18:59, Mark Seaborn wrote:
> Should be "nacl_pnacl_newlib_nonsfi_out_dir" for consistency.  ("newlib" and
> "nonsfi" are not alternatives to each other.)
> 
> Actually, why should this use a different output directory from
> nacl_pnacl_newlib_out_dir?  Can't they use the same directory?

As create_nmf.py doesn't support nonsfi, we need to prepare .nmf file manually.
If we share the directory, it would conflict with the generated .nmf, so I
created an isolated directory.

https://codereview.chromium.org/249183004/diff/100001/ppapi/ppapi_nacl_test_c...
ppapi/ppapi_nacl_test_common.gypi:67: # Nonsfi pnacl copy is convered below.
Currently, these are exclusive.
On 2014/04/25 21:06:58, dmichael wrote:
> convered->covered

Done.

[teravest, 2014-04-28 18:47:32]

lgtm

[dmichael (off chromium), 2014-04-29 17:24:58]

lgtm

https://codereview.chromium.org/249183004/diff/100001/ppapi/nacl_irt/irt_mani...
File ppapi/nacl_irt/irt_manifest.h (right):

https://codereview.chromium.org/249183004/diff/100001/ppapi/nacl_irt/irt_mani...
ppapi/nacl_irt/irt_manifest.h:12: // The implementation of irt_open_resource()
based on ManifestService.
On 2014/04/28 08:44:27, hidehiko wrote:
> On 2014/04/25 21:06:58, dmichael wrote:
> > This ought to document what it does and what the return value means.
> > 
> > Also, it's probably more appropriate to rename the "file" parameter to
"url"?
> 
> Updated the comment.
> 
> As Mark said, the first param is not an actual url, but key.
> Please let me keep it as is to be consistent with irt_open_resource().
Okay, sorry for the misunderstanding, then.

Perhaps you should document the meaning of the "file" parameter, then?

Also...  what's the long-term goal for the meaning of "IrtOpenResource"? Will it
always be a lookup of a key in the manifest? How do we look up the manifest
itself? Maybe it's OK to be a little bit squishy about what this is for now, but
if it's going to be part of our ABI, we ought to nail down the semantics soon. I
don't think I completely understand the intent right now.

[hidehiko, 2014-04-30 02:26:42]

Thank you for review, Justin, Dave.

Mark, friendly ping?

Julien, as Dave and Justin gave LGTM to this CL, I don't think we'll change this
a lot. Could you kindly take a look at *_messages.h file as an owner?

Thanks,
- hidehiko

https://codereview.chromium.org/249183004/diff/100001/ppapi/nacl_irt/irt_mani...
File ppapi/nacl_irt/irt_manifest.h (right):

https://codereview.chromium.org/249183004/diff/100001/ppapi/nacl_irt/irt_mani...
ppapi/nacl_irt/irt_manifest.h:12: // The implementation of irt_open_resource()
based on ManifestService.
On 2014/04/29 17:24:59, dmichael wrote:
> On 2014/04/28 08:44:27, hidehiko wrote:
> > On 2014/04/25 21:06:58, dmichael wrote:
> > > This ought to document what it does and what the return value means.
> > > 
> > > Also, it's probably more appropriate to rename the "file" parameter to
> "url"?
> > 
> > Updated the comment.
> > 
> > As Mark said, the first param is not an actual url, but key.
> > Please let me keep it as is to be consistent with irt_open_resource().
> Okay, sorry for the misunderstanding, then.
> 
> Perhaps you should document the meaning of the "file" parameter, then?

Done.

> 
> Also...  what's the long-term goal for the meaning of "IrtOpenResource"? Will
it
> always be a lookup of a key in the manifest? How do we look up the manifest
> itself? Maybe it's OK to be a little bit squishy about what this is for now,
but
> if it's going to be part of our ABI, we ought to nail down the semantics soon.
I
> don't think I completely understand the intent right now.

My understanding is this is an API to obtain a FD of the resource specified in
the manifest file of "files" section. Mark, any comments?

[dmichael (off chromium), 2014-04-30 02:30:58]

>
>
>  Also...  what's the long-term goal for the meaning of
>>
> "IrtOpenResource"? Will it
>
>> always be a lookup of a key in the manifest? How do we look up the
>>
> manifest
>
>> itself? Maybe it's OK to be a little bit squishy about what this is
>>
> for now, but
>
>> if it's going to be part of our ABI, we ought to nail down the
>>
> semantics soon. I
>
>> don't think I completely understand the intent right now.
>>
>
> My understanding is this is an API to obtain a FD of the resource
> specified in the manifest file of "files" section. Mark, any comments?
>
> I just realized my question about the "manifest itself" doesn't really
make sense. The application shouldn't ever need direct access to the
manifest. I agree, I think it can always be a file key.


> https://codereview.chromium.org/249183004/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark Seaborn, 2014-04-30 21:20:57]

https://codereview.chromium.org/249183004/diff/100001/chrome/test/data/nacl/n...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

https://codereview.chromium.org/249183004/diff/100001/chrome/test/data/nacl/n...
chrome/test/data/nacl/nacl_test_data.gyp:1116: # create_nmf.py doesn't support
nonsfi.
On 2014/04/28 08:44:27, hidehiko wrote:
> On 2014/04/26 00:18:59, Mark Seaborn wrote:
> > How hard would it be to fix that, so that the Gyp files don't get more
> > complicated?
> 
> It sounds not easy to me.
> create_nmf.py parses elf binary to detect its architecture.
> For files in "program" section, we probably can add some heuristics to check
if
> it is for SFI or non-SFI, like checking if ehdr.e_type == ET_DYN, etc.
> However, we also need the same thing for .so files, where ehdr.e_type check
> doesn't work.
> Also, I'm not very sure if checking e_type is appropriate way. Any thought?
> 
> Anyway, I'd like to split the task to support non-SFI on create_nmf from this
> CL.

OK, could you do a follow-on change to clean this up, please?  create_nmf could
potentially rely on a file naming convention and/or a command line flag to
distinguish nonsfi binaries.

The current Gyp logic for adding a nonsfi test duplicates a lot and is very hard
to understand, so I wouldn't want any more tests to add more duplication after
this change.

https://codereview.chromium.org/249183004/diff/220001/chrome/test/nacl/nacl_b...
File chrome/test/nacl/nacl_browsertest.cc (right):

https://codereview.chromium.org/249183004/diff/220001/chrome/test/nacl/nacl_b...
chrome/test/nacl/nacl_browsertest.cc:39: 
If you rebase, you'll find there's now a definition of MAYBE_NONSFI in this file
(from Junichi's irt_exception change).  Please remove that, since you're adding
a definition to nacl_browsertest_util.h.

Also, you'll need to resolve the difference between MAYBE_NONSFI and
MAYBE_PNACL_NONSFI.

https://codereview.chromium.org/249183004/diff/220001/chrome/test/nacl/nacl_b...
chrome/test/nacl/nacl_browsertest.cc:134:
RunNaClIntegrationTest(FILE_PATH_LITERAL("irt_manifest_file_test.html"));
I had a look at this test
(chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc).  Its coverage
is a bit limited -- it doesn't test the error code path (i.e. when
open_resource() returns ENOENT).

Also, it doesn't check what happens if a file is present in the manifest, but
fetching the URL gives an error (such as 404).  Should that return something
like EIO instead of ENOENT?  What's the current behaviour with SFI NaCl?

Can you extend it to test for that, in a separate change, please?

https://codereview.chromium.org/249183004/diff/220001/chrome/test/nacl/nacl_b...
File chrome/test/nacl/nacl_browsertest_util.cc (right):

https://codereview.chromium.org/249183004/diff/220001/chrome/test/nacl/nacl_b...
chrome/test/nacl/nacl_browsertest_util.cc:333: base::FilePath::StringType
NaClBrowserTestPnaclNonSfi::Variant() {
Please rebase: similar code was added in Junichi's irt_exception change.

https://codereview.chromium.org/249183004/diff/220001/chrome/test/nacl/nacl_b...
File chrome/test/nacl/nacl_browsertest_util.h (right):

https://codereview.chromium.org/249183004/diff/220001/chrome/test/nacl/nacl_b...
chrome/test/nacl/nacl_browsertest_util.h:168: #if defined(OS_LINUX) &&
!defined(ADDRESS_SANITIZER)
These are different from the conditions for MAYBE_NONSFI as defined in
ppapi_browsertest.cc, which is a bit confusing.  Could you make it consistent,
please?

https://codereview.chromium.org/249183004/diff/220001/components/nacl/rendere...
File components/nacl/renderer/manifest_service_channel.cc (right):

https://codereview.chromium.org/249183004/diff/220001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.cc:69:
delegate_->OpenResource(key,
On Windows, it would be better for this to be a no-op that replies with an error
message.  (See comments below.)

As security hardening, could you possibly make this code path return a failure
under PNaCl too?  (i.e. On the open web, outside of the web store.) 
open_resource() is not exposed to PNaCl.

The old code path is currently exposed to untrusted code on the open web.  This
isn't ideal, and I'd like to disable it under PNaCl, but the old code path has
been around for a while and is fairly stable.  It would be nice if the new code
path is hidden from PNaCl from the start.

In ppb_nacl_private_impl.cc, you can assume that "!enable_dyncode_syscalls" is
synonymous with PNaCl mode (as loader/nacl_listener.cc does -- see the comment
there).

https://codereview.chromium.org/249183004/diff/220001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.cc:79: NOTREACHED();
This would allow the NaCl process to crash the renderer on debug builds, which
is a little undesirable.  On release builds this is a no-op, I think, so
Send(reply) gets called...

Would this leak a file handle if called on Windows?

https://codereview.chromium.org/249183004/diff/220001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.cc:86:
base::FileDescriptor(platform_file, true)));
On error, platform_file will be PP_kInvalidFileHandle (-1).  It's not clear to
me whether that's valid to pass into the IPC layer.

Having test coverage for the error case would answer this. :-)

https://codereview.chromium.org/249183004/diff/220001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.cc:88: Send(reply);
What are the implications of calling Send(Reply) without doing
WriteReplyParams() first, on Windows?

https://codereview.chromium.org/249183004/diff/220001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/249183004/diff/220001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:190: // called regardless
whether it succeeds or fails.
'it' -> 'OpenResource()' to be clearer?

https://codereview.chromium.org/249183004/diff/220001/ppapi/nacl_irt/irt_mani...
File ppapi/nacl_irt/irt_manifest.cc (right):

https://codereview.chromium.org/249183004/diff/220001/ppapi/nacl_irt/irt_mani...
ppapi/nacl_irt/irt_manifest.cc:13: int IrtOpenResource(const char* file, int*
fd) {
Can you move this into ppapi/nacl_irt/manifest_service.cc, please?  I don't see
a need to have the untrusted-code implementation split across two files -- it
makes the code harder to follow.

https://codereview.chromium.org/249183004/diff/220001/ppapi/nacl_irt/irt_mani...
File ppapi/nacl_irt/irt_manifest.h (right):

https://codereview.chromium.org/249183004/diff/220001/ppapi/nacl_irt/irt_mani...
ppapi/nacl_irt/irt_manifest.h:13: // This communicate with the renderer process
via Chrome IPC to obtain the
"communicates"

https://codereview.chromium.org/249183004/diff/220001/ppapi/nacl_irt/irt_mani...
ppapi/nacl_irt/irt_manifest.h:14: // read-only file descriptor of the resource
specified in manifest file with
Nit: "the manifest file"

https://codereview.chromium.org/249183004/diff/220001/ppapi/nacl_irt/manifest...
File ppapi/nacl_irt/manifest_service.cc (right):

https://codereview.chromium.org/249183004/diff/220001/ppapi/nacl_irt/manifest...
ppapi/nacl_irt/manifest_service.cc:45: *fd =
IPC::PlatformFileForTransitToPlatformFile(ipc_fd.descriptor());
This can just be "ipc_fd.descriptor().fd", I think?  This is untrusted code, so
it doesn't need to go through complex portability-layer functions. :-)

https://codereview.chromium.org/249183004/diff/220001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/service_runtime.h (right):

https://codereview.chromium.org/249183004/diff/220001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/service_runtime.h:86: class
OpenManifestEntryAsyncCallback;
Shouldn't forward decls be grouped together near the file's start?

https://codereview.chromium.org/249183004/diff/220001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/service_runtime.h:182: // sync function
and must be called on a non-main thread, this must be
Nit: "a sync function"?  "this" -> "OpenManifestEntryAsync" to disambiguate?

[hidehiko, 2014-05-01 05:20:30]

Thank you for review.
PTAL, Mark, Julien?

https://codereview.chromium.org/249183004/diff/100001/chrome/test/data/nacl/n...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

https://codereview.chromium.org/249183004/diff/100001/chrome/test/data/nacl/n...
chrome/test/data/nacl/nacl_test_data.gyp:1116: # create_nmf.py doesn't support
nonsfi.
On 2014/04/30 21:20:58, Mark Seaborn wrote:
> On 2014/04/28 08:44:27, hidehiko wrote:
> > On 2014/04/26 00:18:59, Mark Seaborn wrote:
> > > How hard would it be to fix that, so that the Gyp files don't get more
> > > complicated?
> > 
> > It sounds not easy to me.
> > create_nmf.py parses elf binary to detect its architecture.
> > For files in "program" section, we probably can add some heuristics to check
> if
> > it is for SFI or non-SFI, like checking if ehdr.e_type == ET_DYN, etc.
> > However, we also need the same thing for .so files, where ehdr.e_type check
> > doesn't work.
> > Also, I'm not very sure if checking e_type is appropriate way. Any thought?
> > 
> > Anyway, I'd like to split the task to support non-SFI on create_nmf from
this
> > CL.
> 
> OK, could you do a follow-on change to clean this up, please?  create_nmf
could
> potentially rely on a file naming convention and/or a command line flag to
> distinguish nonsfi binaries.
> 
> The current Gyp logic for adding a nonsfi test duplicates a lot and is very
hard
> to understand, so I wouldn't want any more tests to add more duplication after
> this change.

Sure. Filed a bug, and added TODO with referring the bug.

https://codereview.chromium.org/249183004/diff/220001/chrome/test/nacl/nacl_b...
File chrome/test/nacl/nacl_browsertest.cc (right):

https://codereview.chromium.org/249183004/diff/220001/chrome/test/nacl/nacl_b...
chrome/test/nacl/nacl_browsertest.cc:39: 
On 2014/04/30 21:20:58, Mark Seaborn wrote:
> If you rebase, you'll find there's now a definition of MAYBE_NONSFI in this
file
> (from Junichi's irt_exception change).  Please remove that, since you're
adding
> a definition to nacl_browsertest_util.h.
> 
> Also, you'll need to resolve the difference between MAYBE_NONSFI and
> MAYBE_PNACL_NONSFI.

Done.

https://codereview.chromium.org/249183004/diff/220001/chrome/test/nacl/nacl_b...
chrome/test/nacl/nacl_browsertest.cc:134:
RunNaClIntegrationTest(FILE_PATH_LITERAL("irt_manifest_file_test.html"));
On 2014/04/30 21:20:58, Mark Seaborn wrote:
> I had a look at this test
> (chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc).  Its coverage
> is a bit limited -- it doesn't test the error code path (i.e. when
> open_resource() returns ENOENT).
> 
> Also, it doesn't check what happens if a file is present in the manifest, but
> fetching the URL gives an error (such as 404).  Should that return something
> like EIO instead of ENOENT?  What's the current behaviour with SFI NaCl?
> 
> Can you extend it to test for that, in a separate change, please?

Ok, I'll work on in a following CL.

https://codereview.chromium.org/249183004/diff/220001/chrome/test/nacl/nacl_b...
File chrome/test/nacl/nacl_browsertest_util.cc (right):

https://codereview.chromium.org/249183004/diff/220001/chrome/test/nacl/nacl_b...
chrome/test/nacl/nacl_browsertest_util.cc:333: base::FilePath::StringType
NaClBrowserTestPnaclNonSfi::Variant() {
On 2014/04/30 21:20:58, Mark Seaborn wrote:
> Please rebase: similar code was added in Junichi's irt_exception change.

Rebased.

https://codereview.chromium.org/249183004/diff/220001/chrome/test/nacl/nacl_b...
File chrome/test/nacl/nacl_browsertest_util.h (right):

https://codereview.chromium.org/249183004/diff/220001/chrome/test/nacl/nacl_b...
chrome/test/nacl/nacl_browsertest_util.h:168: #if defined(OS_LINUX) &&
!defined(ADDRESS_SANITIZER)
On 2014/04/30 21:20:58, Mark Seaborn wrote:
> These are different from the conditions for MAYBE_NONSFI as defined in
> ppapi_browsertest.cc, which is a bit confusing.  Could you make it consistent,
> please?

Done.

https://codereview.chromium.org/249183004/diff/220001/components/nacl/rendere...
File components/nacl/renderer/manifest_service_channel.cc (right):

https://codereview.chromium.org/249183004/diff/220001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.cc:69:
delegate_->OpenResource(key,
On 2014/04/30 21:20:58, Mark Seaborn wrote:
> On Windows, it would be better for this to be a no-op that replies with an
error
> message.  (See comments below.)
> 
> As security hardening, could you possibly make this code path return a failure
> under PNaCl too?  (i.e. On the open web, outside of the web store.) 
> open_resource() is not exposed to PNaCl.
> 
> The old code path is currently exposed to untrusted code on the open web. 
This
> isn't ideal, and I'd like to disable it under PNaCl, but the old code path has
> been around for a while and is fairly stable.  It would be nice if the new
code
> path is hidden from PNaCl from the start.
> 
> In ppb_nacl_private_impl.cc, you can assume that "!enable_dyncode_syscalls" is
> synonymous with PNaCl mode (as loader/nacl_listener.cc does -- see the comment
> there).

Done.

https://codereview.chromium.org/249183004/diff/220001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.cc:79: NOTREACHED();
On 2014/04/30 21:20:58, Mark Seaborn wrote:
> This would allow the NaCl process to crash the renderer on debug builds, which
> is a little undesirable.  On release builds this is a no-op, I think, so
> Send(reply) gets called...

IMHO, crashing on Debug build is fine. Actually, the code path should never be
called for now. So in new code, I kept NOTREACHED(), but added the code to
return an error to the peer process for Release build.

> 
> Would this leak a file handle if called on Windows?

Yes, but again this shouldn't be called on Windows. Actually, to support windows
we need to change some code in ppapi/native_client/src/trusted/plugin/... as
well, and currently it always returns -1 on Win even if it is accidentally
invoked (again, the code shouldn't be called).

https://codereview.chromium.org/249183004/diff/220001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.cc:86:
base::FileDescriptor(platform_file, true)));
On 2014/04/30 21:20:58, Mark Seaborn wrote:
> On error, platform_file will be PP_kInvalidFileHandle (-1).  It's not clear to
> me whether that's valid to pass into the IPC layer.
> 
> Having test coverage for the error case would answer this. :-)

I think it's fine. Anyway, let's add a test case in a later CL.

https://codereview.chromium.org/249183004/diff/220001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.cc:88: Send(reply);
On 2014/04/30 21:20:58, Mark Seaborn wrote:
> What are the implications of calling Send(Reply) without doing
> WriteReplyParams() first, on Windows?

acknowledged.

https://codereview.chromium.org/249183004/diff/220001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/249183004/diff/220001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:190: // called regardless
whether it succeeds or fails.
On 2014/04/30 21:20:58, Mark Seaborn wrote:
> 'it' -> 'OpenResource()' to be clearer?

Done.

https://codereview.chromium.org/249183004/diff/220001/ppapi/nacl_irt/irt_mani...
File ppapi/nacl_irt/irt_manifest.cc (right):

https://codereview.chromium.org/249183004/diff/220001/ppapi/nacl_irt/irt_mani...
ppapi/nacl_irt/irt_manifest.cc:13: int IrtOpenResource(const char* file, int*
fd) {
On 2014/04/30 21:20:58, Mark Seaborn wrote:
> Can you move this into ppapi/nacl_irt/manifest_service.cc, please?  I don't
see
> a need to have the untrusted-code implementation split across two files -- it
> makes the code harder to follow.

Done. Note that please let me keep the header file separate from the
manifest_service.h. I do not want to include unnecessary code in the header,
especially in the exposed header file.

https://codereview.chromium.org/249183004/diff/220001/ppapi/nacl_irt/irt_mani...
File ppapi/nacl_irt/irt_manifest.h (right):

https://codereview.chromium.org/249183004/diff/220001/ppapi/nacl_irt/irt_mani...
ppapi/nacl_irt/irt_manifest.h:13: // This communicate with the renderer process
via Chrome IPC to obtain the
On 2014/04/30 21:20:58, Mark Seaborn wrote:
> "communicates"

Done.

https://codereview.chromium.org/249183004/diff/220001/ppapi/nacl_irt/irt_mani...
ppapi/nacl_irt/irt_manifest.h:14: // read-only file descriptor of the resource
specified in manifest file with
On 2014/04/30 21:20:58, Mark Seaborn wrote:
> Nit: "the manifest file"

Done.

https://codereview.chromium.org/249183004/diff/220001/ppapi/nacl_irt/manifest...
File ppapi/nacl_irt/manifest_service.cc (right):

https://codereview.chromium.org/249183004/diff/220001/ppapi/nacl_irt/manifest...
ppapi/nacl_irt/manifest_service.cc:45: *fd =
IPC::PlatformFileForTransitToPlatformFile(ipc_fd.descriptor());
On 2014/04/30 21:20:58, Mark Seaborn wrote:
> This can just be "ipc_fd.descriptor().fd", I think?  This is untrusted code,
so
> it doesn't need to go through complex portability-layer functions. :-)

Done.

https://codereview.chromium.org/249183004/diff/220001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/service_runtime.h (right):

https://codereview.chromium.org/249183004/diff/220001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/service_runtime.h:86: class
OpenManifestEntryAsyncCallback;
On 2014/04/30 21:20:58, Mark Seaborn wrote:
> Shouldn't forward decls be grouped together near the file's start?

Done.

https://codereview.chromium.org/249183004/diff/220001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/service_runtime.h:182: // sync function
and must be called on a non-main thread, this must be
On 2014/04/30 21:20:58, Mark Seaborn wrote:
> Nit: "a sync function"?  "this" -> "OpenManifestEntryAsync" to disambiguate?

Done.

[Mark Seaborn, 2014-05-01 06:32:34]

https://codereview.chromium.org/249183004/diff/220001/components/nacl/rendere...
File components/nacl/renderer/manifest_service_channel.cc (right):

https://codereview.chromium.org/249183004/diff/220001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.cc:79: NOTREACHED();
On 2014/05/01 05:20:31, hidehiko wrote:
> On 2014/04/30 21:20:58, Mark Seaborn wrote:
> > This would allow the NaCl process to crash the renderer on debug builds,
which
> > is a little undesirable.  On release builds this is a no-op, I think, so
> > Send(reply) gets called...
> 
> IMHO, crashing on Debug build is fine. Actually, the code path should never be
> called for now. So in new code, I kept NOTREACHED(), but added the code to
> return an error to the peer process for Release build.

For what meaning of "should"? :-)  Untrusted code can send this IPC if it
bypasses the IRT.

In general, untrusted code shouldn't be able to crash the renderer process.

I wouldn't like to assume here that NOTREACHED() only has an effect in debug
builds.  I could imagine it changing to abort in release builds too -- it would
be cheap to compile it to a single "HLT" instruction.

https://codereview.chromium.org/249183004/diff/220001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.cc:86:
base::FileDescriptor(platform_file, true)));
On 2014/05/01 05:20:31, hidehiko wrote:
> On 2014/04/30 21:20:58, Mark Seaborn wrote:
> > On error, platform_file will be PP_kInvalidFileHandle (-1).  It's not
> > clear to me whether that's valid to pass into the IPC layer.
> > 
> > Having test coverage for the error case would answer this. :-)
> 
> I think it's fine. Anyway, let's add a test case in a later CL.

Hmm, as I read it, that means you're not sure whether the error path works? :-)

Note that in the current version of the change, this is exposed to untrusted
code in SFI NaCl.  In that situation, I'd want to be sure this works OK.  It
wouldn't be good for this to be broken temporarily, even if it got fixed later
-- the broken version might get included in a stable branch of Chrome.

However, see my other comment -- if we can disable this code path for SFI NaCl
as well as for PNaCl, I'm less worried about this being temporarily not fully
tested.

If you're going to extend the test coverage in a follow-on change, can you at
least test the error path manually, please, and say whether open_resource()
returns EINVAL OK?

https://codereview.chromium.org/249183004/diff/270001/components/nacl/rendere...
File components/nacl/renderer/manifest_service_channel.cc (right):

https://codereview.chromium.org/249183004/diff/270001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.cc:84:
PpapiHostMsg_OpenResource::WriteReplyParams(
Could you reduce the duplication by doing:

#if !defined(OS_WIN)
  if (!pnacl_mode_) {
    delegate_->OpenResource(...);
    return;
  }
#endif
  PpapiHostMsg_OpenResource::WriteReplyParams(...);
  Send(reply);

https://codereview.chromium.org/249183004/diff/270001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/249183004/diff/270001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:352: !enable_dyncode_syscalls,
 // true, on a plugin in PNaCl mode.
Actually, let's make this stricter and use:
  !enable_dyncode_syscalls && uses_nonsfi_mode

Checking uses_nonsfi_mode would be enough, but adding "!enable_dyncode_syscalls"
better captures the future intent.

Can you comment the reason?  Something like:

For security hardening, disable the IPCs for open_resource() when they aren't
needed.  PNaCl doesn't expose open_resource(), and the new open_resource() IPCs
are currently only used for Non-SFI NaCl so far, not SFI NaCl.

[hidehiko, 2014-05-01 13:05:39]

Could you take another look?

https://codereview.chromium.org/249183004/diff/220001/components/nacl/rendere...
File components/nacl/renderer/manifest_service_channel.cc (right):

https://codereview.chromium.org/249183004/diff/220001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.cc:79: NOTREACHED();
On 2014/05/01 06:32:35, Mark Seaborn wrote:
> On 2014/05/01 05:20:31, hidehiko wrote:
> > On 2014/04/30 21:20:58, Mark Seaborn wrote:
> > > This would allow the NaCl process to crash the renderer on debug builds,
> which
> > > is a little undesirable.  On release builds this is a no-op, I think, so
> > > Send(reply) gets called...
> > 
> > IMHO, crashing on Debug build is fine. Actually, the code path should never
be
> > called for now. So in new code, I kept NOTREACHED(), but added the code to
> > return an error to the peer process for Release build.
> 
> For what meaning of "should"? :-)  Untrusted code can send this IPC if it
> bypasses the IRT.
> 
> In general, untrusted code shouldn't be able to crash the renderer process.
> 
> I wouldn't like to assume here that NOTREACHED() only has an effect in debug
> builds.  I could imagine it changing to abort in release builds too -- it
would
> be cheap to compile it to a single "HLT" instruction.

I'm a bit confused. I thought nacl_helper is a part of trusted code. In
nacl_listener.cc, we create the IPC channel for this class only for non-SFI
mode.
So, this class won't be instantiated and this code path won't be executed on
Win.
I thought the same thing for NaCl in SFI mode.

This is a half of the meaning of "should never" in my previous reply (sorry for
my poor explanation). # The remaining half is, trivially, we do not expect for
this function to be invoked on Win :-).

If we cannot trust nacl_helper itself (i.e., we need to assume that some bad
plugin can create an IPC channel somehow and pass it to the renderer process),
my thought is wrong, though.

https://codereview.chromium.org/249183004/diff/220001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.cc:86:
base::FileDescriptor(platform_file, true)));
On 2014/05/01 06:32:35, Mark Seaborn wrote:
> On 2014/05/01 05:20:31, hidehiko wrote:
> > On 2014/04/30 21:20:58, Mark Seaborn wrote:
> > > On error, platform_file will be PP_kInvalidFileHandle (-1).  It's not
> > > clear to me whether that's valid to pass into the IPC layer.
> > > 
> > > Having test coverage for the error case would answer this. :-)
> > 
> > I think it's fine. Anyway, let's add a test case in a later CL.
> 
> Hmm, as I read it, that means you're not sure whether the error path works?
:-)
> 
> Note that in the current version of the change, this is exposed to untrusted
> code in SFI NaCl.  In that situation, I'd want to be sure this works OK.  It
> wouldn't be good for this to be broken temporarily, even if it got fixed later
> -- the broken version might get included in a stable branch of Chrome.
> 
> However, see my other comment -- if we can disable this code path for SFI NaCl
> as well as for PNaCl, I'm less worried about this being temporarily not fully
> tested.
> 
> If you're going to extend the test coverage in a follow-on change, can you at
> least test the error path manually, please, and say whether open_resource()
> returns EINVAL OK?

I've already confirmed manually, and invoking open_resource() with non-existing
key returns ENOENT as expected. (note: not EINVAL).

https://codereview.chromium.org/249183004/diff/270001/components/nacl/rendere...
File components/nacl/renderer/manifest_service_channel.cc (right):

https://codereview.chromium.org/249183004/diff/270001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.cc:84:
PpapiHostMsg_OpenResource::WriteReplyParams(
On 2014/05/01 06:32:35, Mark Seaborn wrote:
> Could you reduce the duplication by doing:
> 
> #if !defined(OS_WIN)
>   if (!pnacl_mode_) {
>     delegate_->OpenResource(...);
>     return;
>   }
> #endif
>   PpapiHostMsg_OpenResource::WriteReplyParams(...);
>   Send(reply);

Done.

https://codereview.chromium.org/249183004/diff/270001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/249183004/diff/270001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:352: !enable_dyncode_syscalls,
 // true, on a plugin in PNaCl mode.
On 2014/05/01 06:32:35, Mark Seaborn wrote:
> Actually, let's make this stricter and use:
>   !enable_dyncode_syscalls && uses_nonsfi_mode
> 
> Checking uses_nonsfi_mode would be enough, but adding
"!enable_dyncode_syscalls"
> better captures the future intent.
> 
> Can you comment the reason?  Something like:
> 
> For security hardening, disable the IPCs for open_resource() when they aren't
> needed.  PNaCl doesn't expose open_resource(), and the new open_resource()
IPCs
> are currently only used for Non-SFI NaCl so far, not SFI NaCl.

Done.

[Mark Seaborn, 2014-05-01 23:28:32]

LGTM, thanks.  I found the changes to service_runtime.cc hard to follow, but I
think Justin and David understand that part better than I do and have reviewed
it more thoroughly than me.

https://codereview.chromium.org/249183004/diff/220001/components/nacl/rendere...
File components/nacl/renderer/manifest_service_channel.cc (right):

https://codereview.chromium.org/249183004/diff/220001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.cc:79: NOTREACHED();
On 2014/05/01 13:05:40, hidehiko wrote:
> On 2014/05/01 06:32:35, Mark Seaborn wrote:
> > On 2014/05/01 05:20:31, hidehiko wrote:
> > > On 2014/04/30 21:20:58, Mark Seaborn wrote:
> > > > This would allow the NaCl process to crash the renderer on debug builds,
> > which
> > > > is a little undesirable.  On release builds this is a no-op, I think, so
> > > > Send(reply) gets called...
> > > 
> > > IMHO, crashing on Debug build is fine. Actually, the code path should
never
> be
> > > called for now. So in new code, I kept NOTREACHED(), but added the code to
> > > return an error to the peer process for Release build.
> > 
> > For what meaning of "should"? :-)  Untrusted code can send this IPC if it
> > bypasses the IRT.
> > 
> > In general, untrusted code shouldn't be able to crash the renderer process.
> > 
> > I wouldn't like to assume here that NOTREACHED() only has an effect in debug
> > builds.  I could imagine it changing to abort in release builds too -- it
> would
> > be cheap to compile it to a single "HLT" instruction.
> 
> I'm a bit confused. I thought nacl_helper is a part of trusted code. In
> nacl_listener.cc, we create the IPC channel for this class only for non-SFI
> mode.
> So, this class won't be instantiated and this code path won't be executed on
> Win.
> I thought the same thing for NaCl in SFI mode.

Ah, sorry, I hadn't realised that the new IPC channel was only set up by
nacl_listener.cc for Non-SFI Mode.  I had assumed it was set up unconditionally.

In that case, I needn't have asked you to add these extra checks.  However, I
think it's OK to have the extra checks.  It doesn't hurt to have the renderer
check without relying on nacl_listener.cc.

You're right, nacl_helper is trusted before it loads the nexe (and after, for
SFI NaCl).

https://codereview.chromium.org/249183004/diff/250028/chrome/test/nacl/nacl_b...
File chrome/test/nacl/nacl_browsertest_util.cc (right):

https://codereview.chromium.org/249183004/diff/250028/chrome/test/nacl/nacl_b...
chrome/test/nacl/nacl_browsertest_util.cc:343: base::FilePath::StringType
NaClBrowserTestStatic::Variant() {
No need to move these definitions.

https://codereview.chromium.org/249183004/diff/250028/chrome/test/nacl/nacl_b...
File chrome/test/nacl/nacl_browsertest_util.h (right):

https://codereview.chromium.org/249183004/diff/250028/chrome/test/nacl/nacl_b...
chrome/test/nacl/nacl_browsertest_util.h:145: class NaClBrowserTestPnaclNonSfi :
public NaClBrowserTestBase {
No need to move this declaration.

https://codereview.chromium.org/249183004/diff/250028/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/249183004/diff/250028/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:349:
launch_result.manifest_service_ipc_channel_handle)) {
Since you explained this in your previous reply...

If you want, you can move the "enable_dyncode_syscalls && uses_nonsfi_mode"
conditions to the if() here (keeping the comment), and remove
"open_resource_enabled_".  I think that would be cleaner.

[hidehiko, 2014-05-02 01:32:26]

Thank you for review, Mark.

Julien, friendly ping?

https://codereview.chromium.org/249183004/diff/220001/components/nacl/rendere...
File components/nacl/renderer/manifest_service_channel.cc (right):

https://codereview.chromium.org/249183004/diff/220001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.cc:79: NOTREACHED();
On 2014/05/01 23:28:33, Mark Seaborn wrote:
> On 2014/05/01 13:05:40, hidehiko wrote:
> > On 2014/05/01 06:32:35, Mark Seaborn wrote:
> > > On 2014/05/01 05:20:31, hidehiko wrote:
> > > > On 2014/04/30 21:20:58, Mark Seaborn wrote:
> > > > > This would allow the NaCl process to crash the renderer on debug
builds,
> > > which
> > > > > is a little undesirable.  On release builds this is a no-op, I think,
so
> > > > > Send(reply) gets called...
> > > > 
> > > > IMHO, crashing on Debug build is fine. Actually, the code path should
> never
> > be
> > > > called for now. So in new code, I kept NOTREACHED(), but added the code
to
> > > > return an error to the peer process for Release build.
> > > 
> > > For what meaning of "should"? :-)  Untrusted code can send this IPC if it
> > > bypasses the IRT.
> > > 
> > > In general, untrusted code shouldn't be able to crash the renderer
process.
> > > 
> > > I wouldn't like to assume here that NOTREACHED() only has an effect in
debug
> > > builds.  I could imagine it changing to abort in release builds too -- it
> > would
> > > be cheap to compile it to a single "HLT" instruction.
> > 
> > I'm a bit confused. I thought nacl_helper is a part of trusted code. In
> > nacl_listener.cc, we create the IPC channel for this class only for non-SFI
> > mode.
> > So, this class won't be instantiated and this code path won't be executed on
> > Win.
> > I thought the same thing for NaCl in SFI mode.
> 
> Ah, sorry, I hadn't realised that the new IPC channel was only set up by
> nacl_listener.cc for Non-SFI Mode.  I had assumed it was set up
unconditionally.
> 
> In that case, I needn't have asked you to add these extra checks.  However, I
> think it's OK to have the extra checks.  It doesn't hurt to have the renderer
> check without relying on nacl_listener.cc.
> 
> You're right, nacl_helper is trusted before it loads the nexe (and after, for
> SFI NaCl).

Thanks. So let's keep the current error handling.

https://codereview.chromium.org/249183004/diff/250028/chrome/test/nacl/nacl_b...
File chrome/test/nacl/nacl_browsertest_util.cc (right):

https://codereview.chromium.org/249183004/diff/250028/chrome/test/nacl/nacl_b...
chrome/test/nacl/nacl_browsertest_util.cc:343: base::FilePath::StringType
NaClBrowserTestStatic::Variant() {
On 2014/05/01 23:28:33, Mark Seaborn wrote:
> No need to move these definitions.

Done.

https://codereview.chromium.org/249183004/diff/250028/chrome/test/nacl/nacl_b...
File chrome/test/nacl/nacl_browsertest_util.h (right):

https://codereview.chromium.org/249183004/diff/250028/chrome/test/nacl/nacl_b...
chrome/test/nacl/nacl_browsertest_util.h:145: class NaClBrowserTestPnaclNonSfi :
public NaClBrowserTestBase {
On 2014/05/01 23:28:33, Mark Seaborn wrote:
> No need to move this declaration.

Oh, reverted.

https://codereview.chromium.org/249183004/diff/250028/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/249183004/diff/250028/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:349:
launch_result.manifest_service_ipc_channel_handle)) {
On 2014/05/01 23:28:33, Mark Seaborn wrote:
> Since you explained this in your previous reply...
> 
> If you want, you can move the "enable_dyncode_syscalls && uses_nonsfi_mode"
> conditions to the if() here (keeping the comment), and remove
> "open_resource_enabled_".  I think that would be cleaner.

Done.

[jln (very slow on Chromium), 2014-05-02 18:50:50]

lgtm, sorry for the delay

[hidehiko, 2014-05-02 18:54:45]

On 2014/05/02 18:50:50, jln wrote:
> lgtm, sorry for the delay

Thank you for review, all!
Sending to CQ.

[hidehiko, 2014-05-02 18:55:08]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-05-02 18:55:30]

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

[commit-bot: I haz the power, 2014-05-02 18:55:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-02 18:55:52]

Failed to apply patch for ppapi/c/private/ppb_nacl_private.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ppapi/c/private/ppb_nacl_private.h
  Hunk #1 FAILED at 3.
  1 out of 5 hunks FAILED -- saving rejects to file
ppapi/c/private/ppb_nacl_private.h.rej

Patch:       ppapi/c/private/ppb_nacl_private.h
Index: ppapi/c/private/ppb_nacl_private.h
diff --git a/ppapi/c/private/ppb_nacl_private.h
b/ppapi/c/private/ppb_nacl_private.h
index
11e57c3cf27493bb5db0c2701a57499afe483b46..d6d21d312a1c6f742c4f5d3b988054becf79ac16
100644
--- a/ppapi/c/private/ppb_nacl_private.h
+++ b/ppapi/c/private/ppb_nacl_private.h
@@ -3,7 +3,7 @@
  * found in the LICENSE file.
  */
 
-/* From private/ppb_nacl_private.idl modified Fri Apr 25 15:10:15 2014. */
+/* From private/ppb_nacl_private.idl modified Thu May  1 10:40:57 2014. */
 
 #ifndef PPAPI_C_PRIVATE_PPB_NACL_PRIVATE_H_
 #define PPAPI_C_PRIVATE_PPB_NACL_PRIVATE_H_
@@ -15,8 +15,8 @@
 #include "ppapi/c/pp_stdint.h"
 #include "ppapi/c/pp_var.h"
 
-#define PP_MANIFESTSERVICE_INTERFACE_1_0 "PP_ManifestService;1.0"
-#define PP_MANIFESTSERVICE_INTERFACE PP_MANIFESTSERVICE_INTERFACE_1_0
+#define PPP_MANIFESTSERVICE_INTERFACE_1_0 "PPP_ManifestService;1.0"
+#define PPP_MANIFESTSERVICE_INTERFACE PPP_MANIFESTSERVICE_INTERFACE_1_0
 
 #define PPB_NACL_PRIVATE_INTERFACE_1_0 "PPB_NaCl_Private;1.0"
 #define PPB_NACL_PRIVATE_INTERFACE PPB_NACL_PRIVATE_INTERFACE_1_0
@@ -168,6 +168,17 @@ struct PP_PNaClOptions {
  */
 
 /**
+ * @addtogroup Typedefs
+ * @{
+ */
+/* Callback invoked upon completion of PPP_ManifestService::OpenResource(). */
+typedef void (*PP_OpenResourceCompletionCallback)(void* user_data,
+                                                  PP_FileHandle file_handle);
+/**
+ * @}
+ */
+
+/**
  * @addtogroup Interfaces
  * @{
  */
@@ -178,14 +189,21 @@ struct PP_PNaClOptions {
  * Once false is called, as the service has been destructed, all functions
  * should never be called afterwords.
  */
-struct PP_ManifestService_1_0 {
+struct PPP_ManifestService_1_0 {
   /* Called when ManifestService should be destructed. */
   PP_Bool (*Quit)(void* user_data);
   /* Called when PPAPI initialization in the NaCl plugin is finished. */
   PP_Bool (*StartupInitializationComplete)(void* user_data);
+  /* Called when irt_open_resource() is invoked in the NaCl plugin.
+   * Upon completion, callback will be invoked with given callback_user_data
+   * and the result file handle (or PP_kInvalidFileHandle on error). */
+  PP_Bool (*OpenResource)(void* user_data,
+                          const char* entry_key,
+                          PP_OpenResourceCompletionCallback callback,
+                          void* callback_user_data);
 };
 
-typedef struct PP_ManifestService_1_0 PP_ManifestService;
+typedef struct PPP_ManifestService_1_0 PPP_ManifestService;
 
 /* PPB_NaCl_Private */
 struct PPB_NaCl_Private_1_0 {
@@ -218,7 +236,7 @@ struct PPB_NaCl_Private_1_0 {
       PP_Bool enable_dyncode_syscalls,
       PP_Bool enable_exception_handling,
       PP_Bool enable_crash_throttling,
-      const struct PP_ManifestService_1_0* manifest_service_interface,
+      const struct PPP_ManifestService_1_0* manifest_service_interface,
       void* manifest_service_user_data,
       void* imc_handle,
       struct PP_Var* error_message,

[hidehiko, 2014-05-02 19:24:02]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-05-02 19:24:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hidehiko@chromium.org/249183004/380001

[commit-bot: I haz the power, 2014-05-02 19:51:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-02 19:51:45]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium

[hidehiko, 2014-05-02 19:56:53]

The CQ bit was checked by hidehiko@chromium.org

[hamaji, 2014-05-02 19:57:17]

The CQ bit was checked by hamaji@chromium.org

[commit-bot: I haz the power, 2014-05-02 19:58:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hidehiko@chromium.org/249183004/380001

[commit-bot: I haz the power, 2014-05-02 20:26:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-02 20:26:28]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium

[hidehiko, 2014-05-02 20:39:09]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-05-02 20:39:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hidehiko@chromium.org/249183004/380001

[commit-bot: I haz the power, 2014-05-02 21:01:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-02 21:01:17]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium

[jln (very slow on Chromium), 2014-05-02 22:18:42]

The CQ bit was checked by jln@chromium.org

[commit-bot: I haz the power, 2014-05-02 22:19:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hidehiko@chromium.org/249183004/380001

[commit-bot: I haz the power, 2014-05-02 23:24:56]

Change committed as 267962