Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
649603004:
Non-SFI NaCl: Batch-open resource files (Closed)
|
Created:
6 years, 2 months ago by Yusuke Sato Modified:
5 years, 9 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, tzik, binji+watch_chromium.org, raymes+watch_chromium.org, mkwst+moarreviews-ipc_chromium.org, teravest+watch_chromium.org, nfullagar1, piman+watch_chromium.org, noelallen1, ihf+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNon-SFI NaCl: Batch-open resource files
* Let the renderer pass URLs and manifest keys of the resource files
when it asks the browser to start a new plugin process.
* The browser opens all the URLs and passes file handlers to the
plugin process if the process is for non-SFI mode.
* Modify the non-SFI loader so that it can return a pre-opened
handle without doing a renderer IPC when open_resource IRT is
called.
This CL does not change the current behavior of SFI NaCl. SFI
NaCl support will be added in a separate CL:
https://codereview.chromium.org/728113002/
Note that this CL is primarily for ARC (App Runtime for Chrome)
which has 70+ DSO files. With this CL, the loading time of arc.nexe
gets ~40% shorter. This CL should not affect NaCl applications
other than ARC because most of them do not have that many DSO
files, and even if they have some, opening one DSO file in the
browser process takes only about 0.4ms (on Z620) which is usually
negligible.
TEST=PackagedAppTest.SuccessfulLoad
TEST=manually checked that SFI-NaCl and PNaCl are still working.
BUG=nativeclient:3802
BUG=348232
Committed: https://crrev.com/67e8acdac9e043dd710058ef2e01ca7ad2b29825
Cr-Commit-Position: refs/heads/master@{#319931}
Patch Set 1 : try #Patch Set 2 : code review #
Total comments: 19
Patch Set 3 : address comments #Patch Set 4 : add ipc/ #Patch Set 5 : rebase #Patch Set 6 : fix win x64 #
Total comments: 16
Patch Set 7 : address review comments #
Total comments: 2
Patch Set 8 : address comment #Patch Set 9 : rebase #Patch Set 10 : Remove ipc/ and mojo/ changes following Mark's suggestion #
Total comments: 71
Patch Set 11 : address all review comments except one #Patch Set 12 : WIP: rebased, but not ready for review. two comments from Mark haven't been addressed. #Patch Set 13 : still WIP #Patch Set 14 : code review #
Total comments: 34
Patch Set 15 : code review #
Total comments: 38
Patch Set 16 : WIP #Patch Set 17 : code review #
Total comments: 30
Patch Set 18 : address comments #
Total comments: 18
Patch Set 19 : address comments #Patch Set 20 : rebase #Messages
Total messages: 56 (12 generated)
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
yusukes@chromium.org changed reviewers: + mseaborn@chromium.org, teravest@chromium.org
Mark, Justin, Please take a look.
https://codereview.chromium.org/649603004/diff/80001/components/nacl/browser/... File components/nacl/browser/nacl_file_host.cc (right): https://codereview.chromium.org/649603004/diff/80001/components/nacl/browser/... components/nacl/browser/nacl_file_host.cc:92: std::vector<std::pair<uint64, uint64> > resource_file_tokens; Why are you using a pair of uint64 here instead of NaClFileToken? https://codereview.chromium.org/649603004/diff/80001/components/nacl/browser/... components/nacl/browser/nacl_file_host.cc:193: scoped_ptr<base::File[]> resource_files; nit: Why not just do the following? scoped_ptr<base::File[]> resource_files(new base::File[resource_urls.size()]); https://codereview.chromium.org/649603004/diff/80001/components/nacl/browser/... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/649603004/diff/80001/components/nacl/browser/... components/nacl/browser/nacl_process_host.cc:242: scoped_ptr<base::File[]> resource_files, Maybe resource_files/resource_file_tokens/resource_keys could all go into a struct or class so they're kept together? Then that class can enforce the lengths are the same, and NaClProcessHost doesn't have to have 3 more parameters in its constructor. Why is resource_file_tokens getting passed in here anyway? I don't see it getting used... https://codereview.chromium.org/649603004/diff/80001/components/nacl/loader/n... File components/nacl/loader/nonsfi/nonsfi_listener.cc (right): https://codereview.chromium.org/649603004/diff/80001/components/nacl/loader/n... components/nacl/loader/nonsfi/nonsfi_listener.cc:30: void RegisterPreopenedDescriptors(const std::vector<std::string>& keys, Why is this defined in another file? https://codereview.chromium.org/649603004/diff/80001/components/nacl/renderer... File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/649603004/diff/80001/components/nacl/renderer... components/nacl/renderer/ppb_nacl_private_impl.cc:704: PP_FileHandle OpenNaClExecutable( OpenNaClExecutable should probably be renamed to OpenNaClResources throughout. https://codereview.chromium.org/649603004/diff/80001/components/nacl/renderer... components/nacl/renderer/ppb_nacl_private_impl.cc:759: return PP_kInvalidFileHandle; There should at least be some debug logging added to note which resource file failed to open. https://codereview.chromium.org/649603004/diff/80001/components/nacl/renderer... components/nacl/renderer/ppb_nacl_private_impl.cc:766: *nonce_lo = open_result.file_token_hi; Shouldn't this be nonce_hi? Is the behavior for file tokens tested? https://codereview.chromium.org/649603004/diff/80001/components/nacl/renderer... components/nacl/renderer/ppb_nacl_private_impl.cc:780: (*out_resource_file_handles)[open_result.resource_files.size()]; So, out_resource_file_handles is always guaranteed to have length one more than open_result.resource_files? Why store a sentinel value instead of passing a length around? https://codereview.chromium.org/649603004/diff/80001/components/nacl/renderer... components/nacl/renderer/ppb_nacl_private_impl.cc:1307: if (resource_file_urls.size() > 2) { So what happens if more than two resource file urls are requeted? Only the first two get fetched?
https://codereview.chromium.org/649603004/diff/80001/components/nacl/browser/... File components/nacl/browser/nacl_file_host.cc (right): https://codereview.chromium.org/649603004/diff/80001/components/nacl/browser/... components/nacl/browser/nacl_file_host.cc:92: std::vector<std::pair<uint64, uint64> > resource_file_tokens; On 2014/10/24 16:18:06, teravest wrote: > Why are you using a pair of uint64 here instead of NaClFileToken? Stopped using a pair and switched to a struct following your suggestion in other file. I think we can't use NaClFileToken in nacl::NaClOpenExecutableResult since we can't use the type in component/nacl/common/. https://codereview.chromium.org/649603004/diff/80001/components/nacl/browser/... components/nacl/browser/nacl_file_host.cc:193: scoped_ptr<base::File[]> resource_files; This is because resource_urls can be empty (eg. when the nacl executable is not a non-SFI one) and I didn't want to call 'new base::File[0]'. https://codereview.chromium.org/649603004/diff/80001/components/nacl/browser/... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/649603004/diff/80001/components/nacl/browser/... components/nacl/browser/nacl_process_host.cc:242: scoped_ptr<base::File[]> resource_files, On 2014/10/24 16:18:06, teravest wrote: > Maybe resource_files/resource_file_tokens/resource_keys could all go into a > struct or class so they're kept together? > > Then that class can enforce the lengths are the same, and NaClProcessHost > doesn't have to have 3 more parameters in its constructor. Done. > Why is resource_file_tokens getting passed in here anyway? I don't see it > getting used... I'll use this in the next CL for SFI-NaCl (and I don't want to tweak the IPC definitions every time...) There is a TODO(yusukes) at line 898. https://codereview.chromium.org/649603004/diff/80001/components/nacl/loader/n... File components/nacl/loader/nonsfi/nonsfi_listener.cc (right): https://codereview.chromium.org/649603004/diff/80001/components/nacl/loader/n... components/nacl/loader/nonsfi/nonsfi_listener.cc:30: void RegisterPreopenedDescriptors(const std::vector<std::string>& keys, On 2014/10/24 16:18:06, teravest wrote: > Why is this defined in another file? Because nacl::nonsfi::IrtOpenResource() in components/nacl/loader/nonsfi/irt_resource_open.cc will consume the FDs? Please let me know if you know a better way to do this. https://codereview.chromium.org/649603004/diff/80001/components/nacl/renderer... File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/649603004/diff/80001/components/nacl/renderer... components/nacl/renderer/ppb_nacl_private_impl.cc:704: PP_FileHandle OpenNaClExecutable( On 2014/10/24 16:18:06, teravest wrote: > OpenNaClExecutable should probably be renamed to OpenNaClResources throughout. Done. https://codereview.chromium.org/649603004/diff/80001/components/nacl/renderer... components/nacl/renderer/ppb_nacl_private_impl.cc:759: return PP_kInvalidFileHandle; On 2014/10/24 16:18:06, teravest wrote: > There should at least be some debug logging added to note which resource file > failed to open. Done. https://codereview.chromium.org/649603004/diff/80001/components/nacl/renderer... components/nacl/renderer/ppb_nacl_private_impl.cc:766: *nonce_lo = open_result.file_token_hi; On 2014/10/24 16:18:06, teravest wrote: > Shouldn't this be nonce_hi? Is the behavior for file tokens tested? Good catch. Fixed. This will be used in the next CL for SFI-NaCl. https://codereview.chromium.org/649603004/diff/80001/components/nacl/renderer... components/nacl/renderer/ppb_nacl_private_impl.cc:780: (*out_resource_file_handles)[open_result.resource_files.size()]; On 2014/10/24 16:18:06, teravest wrote: > So, out_resource_file_handles is always guaranteed to have length one more than > open_result.resource_files? > > Why store a sentinel value instead of passing a length around? Done. https://codereview.chromium.org/649603004/diff/80001/components/nacl/renderer... components/nacl/renderer/ppb_nacl_private_impl.cc:1307: if (resource_file_urls.size() > 2) { On 2014/10/24 16:18:06, teravest wrote: > So what happens if more than two resource file urls are requeted? Only the first > two get fetched? Yes.
Please take another look. (and sorry for the delay. I was busy doing other ARC tasks last week..)
https://codereview.chromium.org/649603004/diff/80001/components/nacl/renderer... File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/649603004/diff/80001/components/nacl/renderer... components/nacl/renderer/ppb_nacl_private_impl.cc:1307: if (resource_file_urls.size() > 2) { It turned out that removing the restriction does not require native_client/ changes. Slightly modified two files in ipc/ and removed the resize(2) calls.
Justin: As talked offline, I've rebased the CL onto master (so I can land this before https://codereview.chromium.org/572973002/ is landed). Justin/Mark: Please take a look.
https://codereview.chromium.org/649603004/diff/160001/components/nacl/browser... File components/nacl/browser/nacl_file_host.cc (right): https://codereview.chromium.org/649603004/diff/160001/components/nacl/browser... components/nacl/browser/nacl_file_host.cc:63: write_reply_message(reply_msg, file_desc, file_token_lo, file_token_hi); Why is write_reply_message passed as a parameter instead of directly calling NaClHostMsg_GetReadonlyPnaclFD::WriteReplyParams() here? It's confusing to have that level of indirection, and I only see one call to DoRegisterOpenedNaClExecutableFilePnacl in this patch. https://codereview.chromium.org/649603004/diff/160001/components/nacl/browser... File components/nacl/browser/nacl_host_message_filter.cc (right): https://codereview.chromium.org/649603004/diff/160001/components/nacl/browser... components/nacl/browser/nacl_host_message_filter.cc:177: resource_files_info.reset(new nacl::NaClProcessHost::ResourceFileInfo[ elsewhere in this change, you do a size check to prevent allocating an array of size 0. Why not do the same here? https://codereview.chromium.org/649603004/diff/160001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/649603004/diff/160001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:235: : file_token_() { Is this initializer necessary? https://codereview.chromium.org/649603004/diff/160001/components/nacl/common/... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/649603004/diff/160001/components/nacl/common/... components/nacl/common/nacl_types.h:74: base::FilePath file_path_metadata; Documenting the intended use for file_path_metadata would be good here, especially because metadata sounds mysterious. Shouldn't file_path be sufficient for the name here anyway? https://codereview.chromium.org/649603004/diff/160001/components/nacl/common/... components/nacl/common/nacl_types.h:136: std::string file_key; I think it's worth adding a comment that file_key corresponds to a key in the "files" section of the NMF file. https://codereview.chromium.org/649603004/diff/160001/components/nacl/loader/... File components/nacl/loader/nonsfi/irt_resource_open.cc (right): https://codereview.chromium.org/649603004/diff/160001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_resource_open.cc:44: g_fds->erase(it); Are the preopened resources only ever opened once with open_resource()? I suppose this is reasonable, but this is a little surprising; I had expected the returned fd to come from a dup() call. https://codereview.chromium.org/649603004/diff/160001/components/nacl/rendere... File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/649603004/diff/160001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:1335: PP_NaClResourceFileHandle& entry = (*out_resource_file_handles)[i]; Please use a pointer instead of a reference here. https://codereview.chromium.org/649603004/diff/160001/ipc/ipc_channel_nacl.cc File ipc/ipc_channel_nacl.cc (right): https://codereview.chromium.org/649603004/diff/160001/ipc/ipc_channel_nacl.cc... ipc/ipc_channel_nacl.cc:40: contents->fds.resize(NACL_ABI_IMC_DESC_MAX); Won't this still limit the number of fds to 8 (instead of the 128 from the ipc/ change?)
https://codereview.chromium.org/649603004/diff/160001/components/nacl/browser... File components/nacl/browser/nacl_file_host.cc (right): https://codereview.chromium.org/649603004/diff/160001/components/nacl/browser... components/nacl/browser/nacl_file_host.cc:63: write_reply_message(reply_msg, file_desc, file_token_lo, file_token_hi); On 2014/11/10 20:36:11, teravest wrote: > Why is write_reply_message passed as a parameter instead of directly calling > NaClHostMsg_GetReadonlyPnaclFD::WriteReplyParams() here? It's confusing to have > that level of indirection, and I only see one call to > DoRegisterOpenedNaClExecutableFilePnacl in this patch. Good catch, fixed. Thanks. https://codereview.chromium.org/649603004/diff/160001/components/nacl/browser... File components/nacl/browser/nacl_host_message_filter.cc (right): https://codereview.chromium.org/649603004/diff/160001/components/nacl/browser... components/nacl/browser/nacl_host_message_filter.cc:177: resource_files_info.reset(new nacl::NaClProcessHost::ResourceFileInfo[ On 2014/11/10 20:36:11, teravest wrote: > elsewhere in this change, you do a size check to prevent allocating an array of > size 0. Why not do the same here? Done. https://codereview.chromium.org/649603004/diff/160001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/649603004/diff/160001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:235: : file_token_() { On 2014/11/10 20:36:11, teravest wrote: > Is this initializer necessary? Since the struct does not have a constructor, I wanted to initialize it with zeros. https://codereview.chromium.org/649603004/diff/160001/components/nacl/common/... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/649603004/diff/160001/components/nacl/common/... components/nacl/common/nacl_types.h:74: base::FilePath file_path_metadata; On 2014/11/10 20:36:11, teravest wrote: > Documenting the intended use for file_path_metadata would be good here, > especially because metadata sounds mysterious. Shouldn't file_path be sufficient > for the name here anyway? Done. I think I used the suffix to make the variable name consistent with the ones in your next token validation patch, but just |file_path| is also fine. Changed. https://codereview.chromium.org/649603004/diff/160001/components/nacl/common/... components/nacl/common/nacl_types.h:136: std::string file_key; On 2014/11/10 20:36:11, teravest wrote: > I think it's worth adding a comment that file_key corresponds to a key in the > "files" section of the NMF file. Good idea. Done. https://codereview.chromium.org/649603004/diff/160001/components/nacl/loader/... File components/nacl/loader/nonsfi/irt_resource_open.cc (right): https://codereview.chromium.org/649603004/diff/160001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_resource_open.cc:44: g_fds->erase(it); I think this is reasonable. * The dynamic linker (runnable-ld) opens each DT_NEEDED DSO only once. * A NaCl app may call dlopen and dlclose like this: libX_handle = dlopen("libX.so"); dlclose(libX_so_handle); libX_handle = dlopen("libX.so"); dlclose(libX_so_handle); but nacl_io (or an equivalent library) can be implemented in a way that dlclose does not actually close a FD for libX.so with __nacl_irt_close() but instead reuse the dlclose'ed FD. Our nacl_io-like library for ARC already does that. * If a NaCl app does this: libX_handle = dlopen("libX.so"); libX_handle2 = dlopen("libX.so"); dlclose(libX_so_handle); dlclose(libX_so_handle2); the second dlopen() ends up sending a open_resource IPC to the renderer which is slow, but no application I know does this. https://codereview.chromium.org/649603004/diff/160001/components/nacl/rendere... File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/649603004/diff/160001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:1335: PP_NaClResourceFileHandle& entry = (*out_resource_file_handles)[i]; On 2014/11/10 20:36:12, teravest wrote: > Please use a pointer instead of a reference here. Done. https://codereview.chromium.org/649603004/diff/160001/ipc/ipc_channel_nacl.cc File ipc/ipc_channel_nacl.cc (right): https://codereview.chromium.org/649603004/diff/160001/ipc/ipc_channel_nacl.cc... ipc/ipc_channel_nacl.cc:40: contents->fds.resize(NACL_ABI_IMC_DESC_MAX); On 2014/11/10 20:36:12, teravest wrote: > Won't this still limit the number of fds to 8 (instead of the 128 from the ipc/ > change?) It's still limited to 8 and I think that's fine because ipc_channel_nacl.cc is for sending message from trusted NaCl code to untrusted (and vice versa) and there is no IRT that requires lots of batch FD passing (for example, we don't have an IRT like open_resource_batch.)
Please take another look.
Almost there. Just one last thing. https://codereview.chromium.org/649603004/diff/180001/components/nacl/rendere... File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/649603004/diff/180001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:86: // the browser process may use up to 7 descriptors for passing non-resource Where is the browser process limited to 7 descriptors for passing non-resource files? That needs to be enforced somewhere, otherwise the value of kMaxPreOpenResourceFiles will be wrong.
https://codereview.chromium.org/649603004/diff/180001/components/nacl/rendere... File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/649603004/diff/180001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:86: // the browser process may use up to 7 descriptors for passing non-resource On 2014/11/12 20:33:01, teravest wrote: > Where is the browser process limited to 7 descriptors for passing non-resource > files? That needs to be enforced somewhere, otherwise the value of > kMaxPreOpenResourceFiles will be wrong. Done. Added a CHECK to nacl_process_host.cc. Even though the original code didn't check the total number of FDs passed to another process, I'd agree with you. It's better to do some checks.
PTAL
lgtm
Mark: Please take a look
ping?
Mark, following your request, I'm adding open_resource test: https://codereview.chromium.org/758383002/ . Since it turned out that NaClBrowserTest*.IrtManifestFile only tested the slow path (which opens resource files with URLLoader and is almost unrelated to this CL), I implemented the test in ppapi/tests/extensions/packaged_app/ which Justin recently wrote for his nexe validation CL.
Patchset #12 (id:280001) has been deleted
Patchset #11 (id:260001) has been deleted
Patchset #9 (id:220001) has been deleted
Mark, as chatted yesterday, I've removed ipc/ and mojo/ changes from this CL. Please take a look.
I've shared a short design doc for this (which you requested on Friday) with you. I also contacted ipc/ owner (agl@) to see if it is okay to increase the number of FDs in an IPC message to 64, and his answer was affirmative. Please take a look at the CL.
ping?
yusukes@chromium.org changed reviewers: + hidehiko@chromium.org
FYI. https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... File components/nacl/browser/nacl_file_host.cc (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... components/nacl/browser/nacl_file_host.cc:164: if (!nacl::NaClBrowser::GetDelegate()->MapUrlToLocalFilePath( Looks very similar to L186-L199. Can be extracted into a function? https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... File components/nacl/browser/nacl_host_message_filter.cc (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... components/nacl/browser/nacl_host_message_filter.cc:185: launch_params.resource_files_info[i].file_token_lo, nit: indent? https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:920: if (uses_nonsfi_mode_) { nit: if (nonsfi) { // Don't retrieve the file path ... ... } else { if (NaClBrowser::GetInstance()->GetFilePath(...)) { // We have to ... if (base::PostTaskAndReplyWithResult(...)) { return true; } } } may be more understandable? Up to you. https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:921: for (size_t i = 0; i < resource_files_info_len_; ++i) { Could you add a short comment why this is needed only in non-sfi mode? https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... File components/nacl/browser/nacl_process_host.h (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... components/nacl/browser/nacl_process_host.h:106: class ResourceFileInfo { Let's avoid public nested class. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Nested_Classes https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... components/nacl/browser/nacl_process_host.h:118: base::File file() { return file_.Pass(); } Releasing the ownership by getter looks confusing. https://codereview.chromium.org/649603004/diff/300001/components/nacl/common/... File components/nacl/common/nacl_host_messages.h (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/common/... components/nacl/common/nacl_host_messages.h:88: IPC::PlatformFileForTransit /* output file */, nit: this looks as same as FileInfo? https://codereview.chromium.org/649603004/diff/300001/components/nacl/common/... File components/nacl/common/nacl_types.cc (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/common/... components/nacl/common/nacl_types.cc:27: #if defined(OS_POSIX) Isn't this only for Non-SFI? If so, limited to Linux? Otherwise, maybe we should have some check for Win, too? Anyway, I'd like to know your concept to limit OS_POSIX here. https://codereview.chromium.org/649603004/diff/300001/components/nacl/common/... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/common/... components/nacl/common/nacl_types.h:66: struct ResourceFileInfo { Move above? http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... Or, maybe declare at outside of NaClStartParams? https://codereview.chromium.org/649603004/diff/300001/components/nacl/common/... components/nacl/common/nacl_types.h:128: struct ResourceFileInfo { Move to above? http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... https://codereview.chromium.org/649603004/diff/300001/components/nacl/common/... components/nacl/common/nacl_types.h:136: IPC::PlatformFileForTransit file; These three fields looks as same as NaClOpenExecutableResult::FileInfo. https://codereview.chromium.org/649603004/diff/300001/components/nacl/loader/... File components/nacl/loader/nonsfi/irt_resource_open.cc (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_resource_open.cc:20: pthread_mutex_t g_mu = PTHREAD_MUTEX_INITIALIZER; base::Lock? https://codereview.chromium.org/649603004/diff/300001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_resource_open.cc:25: void RegisterPreopenedDescriptors( Could you move the implementation to ppapi/nacl_irt/manifest_service or something around, so that we can share the code even after switching to nacl_helper_nonsfi? https://codereview.chromium.org/649603004/diff/300001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_resource_open.cc:27: pthread_mutex_lock(&g_mu); and base::AutoLock? https://codereview.chromium.org/649603004/diff/300001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_listener.cc (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_listener.cc:162: #if !defined(OS_NACL_NONSFI) When we move the irt implementation to ppapi/nacl_irt/ then we do not need this ifdef guard. https://codereview.chromium.org/649603004/diff/300001/components/nacl/rendere... File components/nacl/renderer/json_manifest.cc (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/rendere... components/nacl/renderer/json_manifest.cc:476: PP_PNaClOptions pnacl_options; unused prefix? Maybe short comment why it is unused would be beneficial. https://codereview.chromium.org/649603004/diff/300001/components/nacl/rendere... components/nacl/renderer/json_manifest.cc:478: return false; logging, too? https://codereview.chromium.org/649603004/diff/300001/components/nacl/rendere... File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:385: base::FileDescriptor(resource_file_handles[i].handle, true), Maybe GetFileHandleForProcess? https://codereview.chromium.org/649603004/diff/300001/ppapi/api/private/ppb_n... File ppapi/api/private/ppb_nacl_private.idl (right): https://codereview.chromium.org/649603004/diff/300001/ppapi/api/private/ppb_n... ppapi/api/private/ppb_nacl_private.idl:173: str_t key; Could you add a comment about the memory management about key.
https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... File components/nacl/browser/nacl_file_host.cc (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... components/nacl/browser/nacl_file_host.cc:41: void DoRegisterOpenedNaClExecutableFilePnacl( Isn't this the same as DoRegisterOpenedNaClExecutableFile() but with 2 fewer arguments? Instead of duplicating DoRegisterOpenedNaClExecutableFile(), can you call it with some empty lists (or singleton lists)? https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... File components/nacl/browser/nacl_file_host.h (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... components/nacl/browser/nacl_file_host.h:50: const std::vector<GURL>& resource_urls, What is the difference between executable_file_url and resource_urls here? Could executable_file_url just be passed as the first entry in the resource_urls vector, so that it doesn't need to be special-cased? Treating them as separate seems to introduce a lot of duplication into nacl_file_host.cc. https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... File components/nacl/browser/nacl_host_message_filter.cc (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... components/nacl/browser/nacl_host_message_filter.cc:177: if (!launch_params.resource_files_info.empty()) { Nit: no need to conditionalise this, surely? Allocating an empty array would work OK. https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... File components/nacl/browser/nacl_process_host.h (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... components/nacl/browser/nacl_process_host.h:106: class ResourceFileInfo { On 2015/01/28 09:05:19, hidehiko wrote: > Let's avoid public nested class. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Nested_Classes I agree. https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... components/nacl/browser/nacl_process_host.h:118: base::File file() { return file_.Pass(); } On 2015/01/28 09:05:19, hidehiko wrote: > Releasing the ownership by getter looks confusing. Indeed... Any reason not to make this a struct, and make file_, file_token_ etc. public members? https://codereview.chromium.org/649603004/diff/300001/components/nacl/common/... File components/nacl/common/nacl_host_messages.h (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/common/... components/nacl/common/nacl_host_messages.h:128: std::vector<GURL> /* URLs of NaCl resource files */, Same comment here: Can you merge this with the previous argument? https://codereview.chromium.org/649603004/diff/300001/components/nacl/loader/... File components/nacl/loader/nonsfi/irt_resource_open.cc (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_resource_open.cc:25: void RegisterPreopenedDescriptors( On 2015/01/28 09:05:19, hidehiko wrote: > Could you move the implementation to ppapi/nacl_irt/manifest_service or > something around, so that we can share the code even after switching to > nacl_helper_nonsfi? I agree. This belongs in ppapi/nacl_irt/manifest_service.cc. https://codereview.chromium.org/649603004/diff/300001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_listener.cc (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_listener.cc:162: #if !defined(OS_NACL_NONSFI) I don't understand why you need to conditionalise this new code with !defined(OS_NACL_NONSFI) in nonsfi_listener.cc. https://codereview.chromium.org/649603004/diff/300001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_listener.cc:163: std::vector<std::pair<std::string, int> > key_fd_pairs; You're converting to a std::vector here only to have RegisterPreopenedDescriptors() convert to a std::map. Can you avoid having an intermediate representation? https://codereview.chromium.org/649603004/diff/300001/components/nacl/rendere... File components/nacl/renderer/json_manifest.cc (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/rendere... components/nacl/renderer/json_manifest.cc:462: bool JsonManifest::GetFiles( This returns a bool but the caller ignores the return value. Change to void? https://codereview.chromium.org/649603004/diff/300001/components/nacl/rendere... components/nacl/renderer/json_manifest.cc:464: if (out_files == NULL) Nit: no need to check for this. https://codereview.chromium.org/649603004/diff/300001/components/nacl/rendere... components/nacl/renderer/json_manifest.cc:469: VLOG(1) << "ResolveKey failed: no \"files\" dictionary"; It's valid to have no "files" dict, so drop this log message? https://codereview.chromium.org/649603004/diff/300001/components/nacl/rendere... components/nacl/renderer/json_manifest.cc:478: return false; Does this "return" mean that if one entry in "files" is invalid, you ignore all later files? That seems unnecessary. Presumably you could write: // We skip invalid entries in "files". if (GetKeyUrl(files, keys[i], &full_url, &pnacl_options)) { out_files->push_back(std::make_pair(full_url, keys[i])); } https://codereview.chromium.org/649603004/diff/300001/components/nacl/rendere... File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:754: if (!gurl.SchemeIs("chrome-extension")) Similar to other comments: this should be de-duped with the existing "chrome-extension" URL scheme check above. https://codereview.chromium.org/649603004/diff/300001/ppapi/api/private/ppb_n... File ppapi/api/private/ppb_nacl_private.idl (right): https://codereview.chromium.org/649603004/diff/300001/ppapi/api/private/ppb_n... ppapi/api/private/ppb_nacl_private.idl:192: [in] PP_NaClResourceFileHandle[] resource_file_handles, If I understand this correctly, DownloadNexe() returns a list of file handles, which src/trusted/plugin/ holds onto and passes back to LaunchSelLdr() later. That shouldn't be necessary. You can keep the list of handles entirely within ppb_nacl_private_impl.cc without having to pass them into src/trusted/plugin/, which doesn't need to process them. (BTW, src/trusted/plugin/ is now at components/nacl/renderer/plugin/.) If you need to store the file handle list as per-instance state in ppb_nacl_private_impl.cc, you can add it to NaClPluginInstance (which I introduced recently).
Please do not review the new patchset yet. I've addressed most of your comments, but Mark's comment in the idl file hasn't been addressed yet. To address it, I think I have to rebase this on top of the latest master (to pull NaClPluginInstance, etc.) I'll finish it tomorrow. https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... File components/nacl/browser/nacl_file_host.cc (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... components/nacl/browser/nacl_file_host.cc:41: void DoRegisterOpenedNaClExecutableFilePnacl( On 2015/02/02 23:21:50, Mark Seaborn wrote: > Isn't this the same as DoRegisterOpenedNaClExecutableFile() but with 2 fewer > arguments? > > Instead of duplicating DoRegisterOpenedNaClExecutableFile(), can you call it > with some empty lists (or singleton lists)? Done. https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... components/nacl/browser/nacl_file_host.cc:164: if (!nacl::NaClBrowser::GetDelegate()->MapUrlToLocalFilePath( On 2015/01/28 09:05:19, hidehiko wrote: > Looks very similar to L186-L199. > Can be extracted into a function? Done (unified the two code paths) https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... File components/nacl/browser/nacl_file_host.h (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... components/nacl/browser/nacl_file_host.h:50: const std::vector<GURL>& resource_urls, On 2015/02/02 23:21:50, Mark Seaborn wrote: > What is the difference between executable_file_url and resource_urls here? > > Could executable_file_url just be passed as the first entry in the resource_urls > vector, so that it doesn't need to be special-cased? > > Treating them as separate seems to introduce a lot of duplication into > nacl_file_host.cc. Done. https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... File components/nacl/browser/nacl_host_message_filter.cc (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... components/nacl/browser/nacl_host_message_filter.cc:177: if (!launch_params.resource_files_info.empty()) { On 2015/02/02 23:21:50, Mark Seaborn wrote: > Nit: no need to conditionalise this, surely? Allocating an empty array would > work OK. Done. https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... components/nacl/browser/nacl_host_message_filter.cc:185: launch_params.resource_files_info[i].file_token_lo, On 2015/01/28 09:05:19, hidehiko wrote: > nit: indent? Done. https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:920: if (uses_nonsfi_mode_) { On 2015/01/28 09:05:19, hidehiko wrote: > nit: > > if (nonsfi) { > // Don't retrieve the file path ... > ... > } else { > if (NaClBrowser::GetInstance()->GetFilePath(...)) { > // We have to ... > if (base::PostTaskAndReplyWithResult(...)) { > return true; > } > } > } > > may be more understandable? Up to you. Done. https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:921: for (size_t i = 0; i < resource_files_info_len_; ++i) { On 2015/01/28 09:05:19, hidehiko wrote: > Could you add a short comment why this is needed only in non-sfi mode? Done. https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... File components/nacl/browser/nacl_process_host.h (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... components/nacl/browser/nacl_process_host.h:106: class ResourceFileInfo { On 2015/02/02 23:21:50, Mark Seaborn wrote: > On 2015/01/28 09:05:19, hidehiko wrote: > > Let's avoid public nested class. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Nested_Classes > > I agree. Done. https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... components/nacl/browser/nacl_process_host.h:106: class ResourceFileInfo { On 2015/02/02 23:21:50, Mark Seaborn wrote: > On 2015/01/28 09:05:19, hidehiko wrote: > > Let's avoid public nested class. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Nested_Classes > > I agree. Done. https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... components/nacl/browser/nacl_process_host.h:118: base::File file() { return file_.Pass(); } On 2015/01/28 09:05:19, hidehiko wrote: > Releasing the ownership by getter looks confusing. Done. https://codereview.chromium.org/649603004/diff/300001/components/nacl/browser... components/nacl/browser/nacl_process_host.h:118: base::File file() { return file_.Pass(); } On 2015/02/02 23:21:50, Mark Seaborn wrote: > On 2015/01/28 09:05:19, hidehiko wrote: > > Releasing the ownership by getter looks confusing. > > Indeed... Any reason not to make this a struct, and make file_, file_token_ etc. > public members? Done. https://codereview.chromium.org/649603004/diff/300001/components/nacl/common/... File components/nacl/common/nacl_host_messages.h (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/common/... components/nacl/common/nacl_host_messages.h:88: IPC::PlatformFileForTransit /* output file */, On 2015/01/28 09:05:19, hidehiko wrote: > nit: this looks as same as FileInfo? Done. https://codereview.chromium.org/649603004/diff/300001/components/nacl/common/... components/nacl/common/nacl_host_messages.h:128: std::vector<GURL> /* URLs of NaCl resource files */, On 2015/02/02 23:21:50, Mark Seaborn wrote: > Same comment here: Can you merge this with the previous argument? Done. https://codereview.chromium.org/649603004/diff/300001/components/nacl/common/... File components/nacl/common/nacl_types.cc (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/common/... components/nacl/common/nacl_types.cc:27: #if defined(OS_POSIX) On 2015/01/28 09:05:19, hidehiko wrote: > Isn't this only for Non-SFI? If so, limited to Linux? Otherwise, maybe we should > have some check for Win, too? > Anyway, I'd like to know your concept to limit OS_POSIX here. Right, for now, OS_LINUX would suffice. Changed to OS_LINUX with a TODO. https://codereview.chromium.org/649603004/diff/300001/components/nacl/common/... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/common/... components/nacl/common/nacl_types.h:66: struct ResourceFileInfo { On 2015/01/28 09:05:19, hidehiko wrote: > Move above? > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... > > Or, maybe declare at outside of NaClStartParams? Done. Let me keep this inside the class to avoid name conflict with the one below. https://codereview.chromium.org/649603004/diff/300001/components/nacl/common/... components/nacl/common/nacl_types.h:128: struct ResourceFileInfo { On 2015/01/28 09:05:19, hidehiko wrote: > Move to above? > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... Done. https://codereview.chromium.org/649603004/diff/300001/components/nacl/common/... components/nacl/common/nacl_types.h:136: IPC::PlatformFileForTransit file; On 2015/01/28 09:05:19, hidehiko wrote: > These three fields looks as same as NaClOpenExecutableResult::FileInfo. Done. https://codereview.chromium.org/649603004/diff/300001/components/nacl/loader/... File components/nacl/loader/nonsfi/irt_resource_open.cc (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_resource_open.cc:20: pthread_mutex_t g_mu = PTHREAD_MUTEX_INITIALIZER; On 2015/01/28 09:05:19, hidehiko wrote: > base::Lock? I used this primitive type to ensure that g_mu never introduces a static initializer. Do you know if base::Lock is always safe to use as a global variable? If it is, I'll change. https://codereview.chromium.org/649603004/diff/300001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_resource_open.cc:25: void RegisterPreopenedDescriptors( On 2015/02/02 23:21:50, Mark Seaborn wrote: > On 2015/01/28 09:05:19, hidehiko wrote: > > Could you move the implementation to ppapi/nacl_irt/manifest_service or > > something around, so that we can share the code even after switching to > > nacl_helper_nonsfi? > > I agree. This belongs in ppapi/nacl_irt/manifest_service.cc. Done. https://codereview.chromium.org/649603004/diff/300001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_resource_open.cc:25: void RegisterPreopenedDescriptors( On 2015/01/28 09:05:19, hidehiko wrote: > Could you move the implementation to ppapi/nacl_irt/manifest_service or > something around, so that we can share the code even after switching to > nacl_helper_nonsfi? Done. https://codereview.chromium.org/649603004/diff/300001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_resource_open.cc:27: pthread_mutex_lock(&g_mu); On 2015/01/28 09:05:19, hidehiko wrote: > and base::AutoLock? same https://codereview.chromium.org/649603004/diff/300001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_listener.cc (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_listener.cc:162: #if !defined(OS_NACL_NONSFI) On 2015/01/28 09:05:19, hidehiko wrote: > When we move the irt implementation to ppapi/nacl_irt/ then we do not need this > ifdef guard. removed https://codereview.chromium.org/649603004/diff/300001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_listener.cc:162: #if !defined(OS_NACL_NONSFI) On 2015/02/02 23:21:50, Mark Seaborn wrote: > I don't understand why you need to conditionalise this new code with > !defined(OS_NACL_NONSFI) in nonsfi_listener.cc. removed https://codereview.chromium.org/649603004/diff/300001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_listener.cc:163: std::vector<std::pair<std::string, int> > key_fd_pairs; On 2015/02/02 23:21:50, Mark Seaborn wrote: > You're converting to a std::vector here only to have > RegisterPreopenedDescriptors() convert to a std::map. Can you avoid having an > intermediate representation? Since params.resource_files is a struct defined in components/nacl/common/nacl_types.h, passing the struct directly to ppapi/nacl_irt/manifest_service.cc doesn't seem possible (ppapi/nacl_irt/DEPS does not have the path.) Can I keep this? Or can I add components/nacl/common/ to ppapi/nacl_irt/DEPS? https://codereview.chromium.org/649603004/diff/300001/components/nacl/rendere... File components/nacl/renderer/json_manifest.cc (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/rendere... components/nacl/renderer/json_manifest.cc:462: bool JsonManifest::GetFiles( On 2015/02/02 23:21:50, Mark Seaborn wrote: > This returns a bool but the caller ignores the return value. Change to void? Done. https://codereview.chromium.org/649603004/diff/300001/components/nacl/rendere... components/nacl/renderer/json_manifest.cc:464: if (out_files == NULL) On 2015/02/02 23:21:50, Mark Seaborn wrote: > Nit: no need to check for this. Done. https://codereview.chromium.org/649603004/diff/300001/components/nacl/rendere... components/nacl/renderer/json_manifest.cc:469: VLOG(1) << "ResolveKey failed: no \"files\" dictionary"; On 2015/02/02 23:21:50, Mark Seaborn wrote: > It's valid to have no "files" dict, so drop this log message? Done. https://codereview.chromium.org/649603004/diff/300001/components/nacl/rendere... components/nacl/renderer/json_manifest.cc:476: PP_PNaClOptions pnacl_options; On 2015/01/28 09:05:20, hidehiko wrote: > unused prefix? Maybe short comment why it is unused would be beneficial. Done. https://codereview.chromium.org/649603004/diff/300001/components/nacl/rendere... components/nacl/renderer/json_manifest.cc:478: return false; On 2015/01/28 09:05:20, hidehiko wrote: > logging, too? Done. https://codereview.chromium.org/649603004/diff/300001/components/nacl/rendere... components/nacl/renderer/json_manifest.cc:478: return false; On 2015/02/02 23:21:50, Mark Seaborn wrote: > Does this "return" mean that if one entry in "files" is invalid, you ignore all > later files? That seems unnecessary. > > Presumably you could write: > > // We skip invalid entries in "files". > if (GetKeyUrl(files, keys[i], &full_url, &pnacl_options)) { > out_files->push_back(std::make_pair(full_url, keys[i])); > } Done. https://codereview.chromium.org/649603004/diff/300001/components/nacl/rendere... File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:385: base::FileDescriptor(resource_file_handles[i].handle, true), On 2015/01/28 09:05:20, hidehiko wrote: > Maybe GetFileHandleForProcess? Can be, but since this is inside OS_POSIX, I'd like to do it in a simpler way (and the same way as the existing code - line 359). https://codereview.chromium.org/649603004/diff/300001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:754: if (!gurl.SchemeIs("chrome-extension")) On 2015/02/02 23:21:51, Mark Seaborn wrote: > Similar to other comments: this should be de-duped with the existing > "chrome-extension" URL scheme check above. Done. https://codereview.chromium.org/649603004/diff/300001/ppapi/api/private/ppb_n... File ppapi/api/private/ppb_nacl_private.idl (right): https://codereview.chromium.org/649603004/diff/300001/ppapi/api/private/ppb_n... ppapi/api/private/ppb_nacl_private.idl:173: str_t key; On 2015/01/28 09:05:20, hidehiko wrote: > Could you add a comment about the memory management about key. Done.
https://codereview.chromium.org/649603004/diff/300001/components/nacl/loader/... File components/nacl/loader/nonsfi/irt_resource_open.cc (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_resource_open.cc:20: pthread_mutex_t g_mu = PTHREAD_MUTEX_INITIALIZER; On 2015/02/04 02:00:28, Yusuke Sato wrote: > On 2015/01/28 09:05:19, hidehiko wrote: > > base::Lock? > > I used this primitive type to ensure that g_mu never introduces a static > initializer. Do you know if base::Lock is always safe to use as a global > variable? If it is, I'll change. Indeed, I think it's not safe. base::Lock is a non-POD type, so to use it in a global you'd need to wrap it in a base::LazyInstance (in order to follow Chromium's rules about not using global initialisers). That's somewhat complex, and more expensive than using pthread_mutex_t, which has a POD initialiser. base::Lock exists for portability, but this file only needs to work with NaCl/Linux toolchains. So the trade-off is efficiency & complexity vs. consistency with the rest of the codebase. I think using pthread_mutex_t is OK here. https://codereview.chromium.org/649603004/diff/300001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_listener.cc (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_listener.cc:163: std::vector<std::pair<std::string, int> > key_fd_pairs; On 2015/02/04 02:00:29, Yusuke Sato wrote: > On 2015/02/02 23:21:50, Mark Seaborn wrote: > > You're converting to a std::vector here only to have > > RegisterPreopenedDescriptors() convert to a std::map. Can you avoid having an > > intermediate representation? > > Since params.resource_files is a struct defined in > components/nacl/common/nacl_types.h, passing the struct directly to > ppapi/nacl_irt/manifest_service.cc doesn't seem possible (ppapi/nacl_irt/DEPS > does not have the path.) > > Can I keep this? Or can I add components/nacl/common/ to ppapi/nacl_irt/DEPS? I think it would be OK to add components/nacl/common/ to ppapi/nacl_irt/DEPS, although ppapi/ doesn't contain any #includes of components/nacl/ at the moment. It might be cleaner to move ppapi/nacl_irt/manifest_service.cc to components/nacl/. However, I don't think that's necessary... How about just passing a pointer to a std::map to RegisterPreopenedDescriptors()?
Patchset #14 (id:380001) has been deleted
https://codereview.chromium.org/649603004/diff/300001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_listener.cc (right): https://codereview.chromium.org/649603004/diff/300001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_listener.cc:163: std::vector<std::pair<std::string, int> > key_fd_pairs; On 2015/02/04 18:52:16, Mark Seaborn wrote: > On 2015/02/04 02:00:29, Yusuke Sato wrote: > > On 2015/02/02 23:21:50, Mark Seaborn wrote: > > > You're converting to a std::vector here only to have > > > RegisterPreopenedDescriptors() convert to a std::map. Can you avoid having > an > > > intermediate representation? > > > > Since params.resource_files is a struct defined in > > components/nacl/common/nacl_types.h, passing the struct directly to > > ppapi/nacl_irt/manifest_service.cc doesn't seem possible (ppapi/nacl_irt/DEPS > > does not have the path.) > > > > Can I keep this? Or can I add components/nacl/common/ to ppapi/nacl_irt/DEPS? > > I think it would be OK to add components/nacl/common/ to ppapi/nacl_irt/DEPS, > although ppapi/ doesn't contain any #includes of components/nacl/ at the moment. > It might be cleaner to move ppapi/nacl_irt/manifest_service.cc to > components/nacl/. However, I don't think that's necessary... > > How about just passing a pointer to a std::map to > RegisterPreopenedDescriptors()? Done. https://codereview.chromium.org/649603004/diff/300001/ppapi/api/private/ppb_n... File ppapi/api/private/ppb_nacl_private.idl (right): https://codereview.chromium.org/649603004/diff/300001/ppapi/api/private/ppb_n... ppapi/api/private/ppb_nacl_private.idl:192: [in] PP_NaClResourceFileHandle[] resource_file_handles, On 2015/02/02 23:21:51, Mark Seaborn wrote: > If I understand this correctly, DownloadNexe() returns a list of file handles, > which src/trusted/plugin/ holds onto and passes back to LaunchSelLdr() later. > > That shouldn't be necessary. You can keep the list of handles entirely within > ppb_nacl_private_impl.cc without having to pass them into src/trusted/plugin/, > which doesn't need to process them. > > (BTW, src/trusted/plugin/ is now at components/nacl/renderer/plugin/.) > > If you need to store the file handle list as per-instance state in > ppb_nacl_private_impl.cc, you can add it to NaClPluginInstance (which I > introduced recently). Done.
Please take another look at the diff between patchset #10 and #11, and patchset #12 and #14. The latter addresses two comments from Mark, and the former does all of them except the two.
https://codereview.chromium.org/649603004/diff/400001/components/nacl/browser... File components/nacl/browser/nacl_file_host.cc (right): https://codereview.chromium.org/649603004/diff/400001/components/nacl/browser... components/nacl/browser/nacl_file_host.cc:41: Nit: remove empty line (only 1 between top-level decls) https://codereview.chromium.org/649603004/diff/400001/components/nacl/browser... components/nacl/browser/nacl_file_host.cc:117: std::vector<base::FilePath> resource_file_paths(1); Are you sure that base::Bind() makes a copy of this argument? Can you point me to the code that does that, or at comments? https://codereview.chromium.org/649603004/diff/400001/components/nacl/browser... File components/nacl/browser/nacl_host_message_filter.cc (right): https://codereview.chromium.org/649603004/diff/400001/components/nacl/browser... components/nacl/browser/nacl_host_message_filter.cc:173: // TODO(yusukes): Support pre-opening resource files. Nit: Add "on Windows"? https://codereview.chromium.org/649603004/diff/400001/components/nacl/browser... File components/nacl/browser/nacl_process_host.h (right): https://codereview.chromium.org/649603004/diff/400001/components/nacl/browser... components/nacl/browser/nacl_process_host.h:45: class ResourceFileInfo { It's rather confusing that you have three ResourceFileInfo types -- this class plus 2 structs in nacl_types.h. This one appears to contain the same information as NaClLaunchParams::ResourceFileInfo, which contains: NaClFileInfo file_info; // file descriptor + a NaClFileToken std::string file_key; Were there some obstacles to using the same type in both cases? Is the difference in whether these types use an auto-freeing FD (i.e. base::File)? https://codereview.chromium.org/649603004/diff/400001/components/nacl/browser... components/nacl/browser/nacl_process_host.h:57: base::File file_; Nit: If these are public, they shouldn't have a "_" suffix https://codereview.chromium.org/649603004/diff/400001/components/nacl/browser... components/nacl/browser/nacl_process_host.h:86: size_t resource_files_info_len, How about making resource_files_info a vector instead of passing the length separately? https://codereview.chromium.org/649603004/diff/400001/components/nacl/common/... File components/nacl/common/nacl_host_messages.h (right): https://codereview.chromium.org/649603004/diff/400001/components/nacl/common/... components/nacl/common/nacl_host_messages.h:116: IPC_SYNC_MESSAGE_CONTROL3_1(NaClHostMsg_OpenNaClResources, Since this is a synchronous IPC message, is it going to introduce any renderer jank if the renderer is waiting for the browser to open ~128 files? Hmm, still, I suppose that is better than the current situation of doing ~128 synchronous IPC messages. https://codereview.chromium.org/649603004/diff/400001/components/nacl/common/... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/649603004/diff/400001/components/nacl/common/... components/nacl/common/nacl_types.h:60: struct NaClFileInfo { This is basically the same type as PP_NaClFileInfo in ppb_nacl_private.h. I assume you duplicated it mainly because you wanted to add Chrome IPC marshalling for it (via IPC_STRUCT_TRAITS_BEGIN etc.)? If so, we could do this more cleanly by moving PP_NaClFileInfo out of ppapi/ so that it's easier to add Chrome IPC marshalling (https://codereview.chromium.org/911463003/). https://codereview.chromium.org/649603004/diff/400001/components/nacl/rendere... File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/649603004/diff/400001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:79: const size_t kMaxPreOpenResourceFiles = 2; Why so low? I thought your aim was to pass more than this. With this set low, I think the fast path won't be covered by the tests you added earlier, will it? https://codereview.chromium.org/649603004/diff/400001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:127: std::vector<NaClLaunchParams::ResourceFileInfo> resource_files_info; If I read this right, NaClLaunchParams::ResourceFileInfo contains an IPC::PlatformFileForTransit, which is a non-owning reference. That means this would leak FDs if the NaCl instance failed to start up. https://codereview.chromium.org/649603004/diff/400001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:427: nacl_plugin_instance->resource_files_info; Shouldn't nacl_plugin_instance->resource_files_info be cleared out after this? (At least this would reclaim some storage. Not sure if it would leak FDs for the <embed>'s lifetime without this.) https://codereview.chromium.org/649603004/diff/400001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:433: // TODO(yusukes): Support pre-opening resource files. Add "on Windows"? https://codereview.chromium.org/649603004/diff/400001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:1438: if (OpenNaClResources(instance, file_urls, &out_handles)) { I think there's a potential performance regression here... Suppose we have an app whose main nexe is served via a "chrome-extension:" URL, but which has other files served via HTTP(S). Before, the fast path (validation caching + opening without a copy) will work for the main nexe. After, the fast path will be disabled on the grounds that it doesn't work for all files in the manifest file. https://codereview.chromium.org/649603004/diff/400001/ppapi/nacl_irt/manifest... File ppapi/nacl_irt/manifest_service.cc (right): https://codereview.chromium.org/649603004/diff/400001/ppapi/nacl_irt/manifest... ppapi/nacl_irt/manifest_service.cc:163: if (file[0] == '/') This check for "/" duplicates the check in IrtOpenResource() above. Can you de-dupe them please? You don't need IrtOpenResourceNonSfi() to be a separate entry point into this file. You could define an #if'd fast-path function that IrtOpenResource() calls. https://codereview.chromium.org/649603004/diff/400001/ppapi/thunk/ppb_camera_... File ppapi/thunk/ppb_camera_capabilities_private_thunk.cc (right): https://codereview.chromium.org/649603004/diff/400001/ppapi/thunk/ppb_camera_... ppapi/thunk/ppb_camera_capabilities_private_thunk.cc:5: // From private/ppb_camera_capabilities_private.idl modified Thu Aug 28 19:38:16 Nit: Please undo the timestamp changes on the files that are unrelated to your change.
Please take another look. Re trybots, they seem broken right now. I tried 'git cl try', 'git cl try -r revision', and 'git cl try -r revision -c', but all failed. I'd give up for now and retry later. https://codereview.chromium.org/649603004/diff/400001/components/nacl/browser... File components/nacl/browser/nacl_file_host.cc (right): https://codereview.chromium.org/649603004/diff/400001/components/nacl/browser... components/nacl/browser/nacl_file_host.cc:41: On 2015/02/09 04:48:34, Mark Seaborn wrote: > Nit: remove empty line (only 1 between top-level decls) Done. https://codereview.chromium.org/649603004/diff/400001/components/nacl/browser... components/nacl/browser/nacl_file_host.cc:117: std::vector<base::FilePath> resource_file_paths(1); On 2015/02/09 04:48:34, Mark Seaborn wrote: > Are you sure that base::Bind() makes a copy of this argument? Can you point me > to the code that does that, or at comments? Yes, base/callback.h has some comments: https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&q=... https://codereview.chromium.org/649603004/diff/400001/components/nacl/browser... File components/nacl/browser/nacl_host_message_filter.cc (right): https://codereview.chromium.org/649603004/diff/400001/components/nacl/browser... components/nacl/browser/nacl_host_message_filter.cc:173: // TODO(yusukes): Support pre-opening resource files. On 2015/02/09 04:48:34, Mark Seaborn wrote: > Nit: Add "on Windows"? Done. https://codereview.chromium.org/649603004/diff/400001/components/nacl/browser... File components/nacl/browser/nacl_process_host.h (right): https://codereview.chromium.org/649603004/diff/400001/components/nacl/browser... components/nacl/browser/nacl_process_host.h:45: class ResourceFileInfo { On 2015/02/09 04:48:34, Mark Seaborn wrote: > It's rather confusing that you have three ResourceFileInfo types -- this class > plus 2 structs in nacl_types.h. > > This one appears to contain the same information as > NaClLaunchParams::ResourceFileInfo, which contains: > > NaClFileInfo file_info; // file descriptor + a NaClFileToken > std::string file_key; > > Were there some obstacles to using the same type in both cases? Is the > difference in whether these types use an auto-freeing FD (i.e. base::File)? I defined these structs to use exactly the same types as the main nexe whenever possible, but it seems like this was just confusing. I've removed most of them. New patch set uses nacl::NaClResourceFileInfo in most places. https://codereview.chromium.org/649603004/diff/400001/components/nacl/browser... components/nacl/browser/nacl_process_host.h:57: base::File file_; On 2015/02/09 04:48:34, Mark Seaborn wrote: > Nit: If these are public, they shouldn't have a "_" suffix Removed the class. Done. https://codereview.chromium.org/649603004/diff/400001/components/nacl/browser... components/nacl/browser/nacl_process_host.h:86: size_t resource_files_info_len, On 2015/02/09 04:48:34, Mark Seaborn wrote: > How about making resource_files_info a vector instead of passing the length > separately? It was difficult to do so for the move-only class, but now it's possible as I've removed the class. Done. https://codereview.chromium.org/649603004/diff/400001/components/nacl/common/... File components/nacl/common/nacl_host_messages.h (right): https://codereview.chromium.org/649603004/diff/400001/components/nacl/common/... components/nacl/common/nacl_host_messages.h:116: IPC_SYNC_MESSAGE_CONTROL3_1(NaClHostMsg_OpenNaClResources, On 2015/02/09 04:48:34, Mark Seaborn wrote: > Since this is a synchronous IPC message, is it going to introduce any renderer > jank if the renderer is waiting for the browser to open ~128 files? Hmm, still, > I suppose that is better than the current situation of doing ~128 synchronous > IPC messages. On low-end Chromebooks, opening ~128 files seems to take ~50ms. So this could cause a slight UI jank. But yes, I think this still reduces the total sync IPC time. https://codereview.chromium.org/649603004/diff/400001/components/nacl/common/... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/649603004/diff/400001/components/nacl/common/... components/nacl/common/nacl_types.h:60: struct NaClFileInfo { (This struct no longer exists) https://codereview.chromium.org/649603004/diff/400001/components/nacl/common/... components/nacl/common/nacl_types.h:74: struct ResourceFileInfo { Let me keep this one. Unlike renderer-browser IPC, browser-nacl IPC message has to have base::FilePath (for validation caching) instead of the token. https://codereview.chromium.org/649603004/diff/400001/components/nacl/common/... components/nacl/common/nacl_types.h:128: struct ResourceFileInfo { removed https://codereview.chromium.org/649603004/diff/400001/components/nacl/rendere... File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/649603004/diff/400001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:79: const size_t kMaxPreOpenResourceFiles = 2; On 2015/02/09 04:48:34, Mark Seaborn wrote: > Why so low? I thought your aim was to pass more than this. > > With this set low, I think the fast path won't be covered by the tests you added > earlier, will it? This is currently 2 because kMaxDescriptorsPerMessage in ipc/ is still 7. The test I added is to make sure that the renderer process can start a NaCl plugin whose nmf has more than kMaxDescriptorsPerMessage resource files. I think the test is useful regardless of the kMaxPreOpenResourceFiles value. I will increase kMaxDescriptorsPerMessage in ipc/ as well as kMaxPreOpenResourceFiles to something like ~80 or ~128 once this CL is submitted. ipc/ changes are not part of this CL following your earlier suggestion. https://codereview.chromium.org/649603004/diff/400001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:127: std::vector<NaClLaunchParams::ResourceFileInfo> resource_files_info; On 2015/02/09 04:48:34, Mark Seaborn wrote: > If I read this right, NaClLaunchParams::ResourceFileInfo contains an > IPC::PlatformFileForTransit, which is a non-owning reference. That means this > would leak FDs if the NaCl instance failed to start up. You're right. The IPC::PlatformFileForTransit handle here is a base::FileDescriptor with auto_close=true, so once it's passed to a IPC message, it's always auto-closed, but if something fails before creating the IPC message, the FDs will leak. I've added ~NaClPluginInstance() to handle such a case. Done. https://codereview.chromium.org/649603004/diff/400001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:427: nacl_plugin_instance->resource_files_info; On 2015/02/09 04:48:34, Mark Seaborn wrote: > Shouldn't nacl_plugin_instance->resource_files_info be cleared out after this? > (At least this would reclaim some storage. Not sure if it would leak FDs for > the <embed>'s lifetime without this.) Done. https://codereview.chromium.org/649603004/diff/400001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:433: // TODO(yusukes): Support pre-opening resource files. On 2015/02/09 04:48:34, Mark Seaborn wrote: > Add "on Windows"? Done. https://codereview.chromium.org/649603004/diff/400001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:1438: if (OpenNaClResources(instance, file_urls, &out_handles)) { I wasn't aware that HTTP(S) URL was allowed in "files". Slightly changed DownloadNexe so that it only retrieves resource URLs from nmf that start with "chrome-extension". Done. https://codereview.chromium.org/649603004/diff/400001/ppapi/nacl_irt/manifest... File ppapi/nacl_irt/manifest_service.cc (right): https://codereview.chromium.org/649603004/diff/400001/ppapi/nacl_irt/manifest... ppapi/nacl_irt/manifest_service.cc:163: if (file[0] == '/') On 2015/02/09 04:48:35, Mark Seaborn wrote: > This check for "/" duplicates the check in IrtOpenResource() above. Can you > de-dupe them please? > > You don't need IrtOpenResourceNonSfi() to be a separate entry point into this > file. You could define an #if'd fast-path function that IrtOpenResource() > calls. Done. https://codereview.chromium.org/649603004/diff/400001/ppapi/thunk/ppb_camera_... File ppapi/thunk/ppb_camera_capabilities_private_thunk.cc (right): https://codereview.chromium.org/649603004/diff/400001/ppapi/thunk/ppb_camera_... ppapi/thunk/ppb_camera_capabilities_private_thunk.cc:5: // From private/ppb_camera_capabilities_private.idl modified Thu Aug 28 19:38:16 On 2015/02/09 04:48:35, Mark Seaborn wrote: > Nit: Please undo the timestamp changes on the files that are unrelated to your > change. Done. https://codereview.chromium.org/649603004/diff/420001/components/nacl/common/... File components/nacl/common/nacl_host_messages.h (right): https://codereview.chromium.org/649603004/diff/420001/components/nacl/common/... components/nacl/common/nacl_host_messages.h:77: IPC::PlatformFileForTransit /* output file */, reverted to the original code https://codereview.chromium.org/649603004/diff/420001/components/nacl/rendere... File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/649603004/diff/420001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:621: IPC::PlatformFileForTransit out_fd = IPC::InvalidPlatformFileForTransit(); reverted to the original code.
https://codereview.chromium.org/649603004/diff/400001/components/nacl/rendere... File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/649603004/diff/400001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:79: const size_t kMaxPreOpenResourceFiles = 2; On 2015/02/11 05:54:21, Yusuke Sato wrote: > On 2015/02/09 04:48:34, Mark Seaborn wrote: > > Why so low? I thought your aim was to pass more than this. > > > > With this set low, I think the fast path won't be covered by the tests you > added > > earlier, will it? > > This is currently 2 because kMaxDescriptorsPerMessage in ipc/ is still 7. > > The test I added is to make sure that the renderer process can start a NaCl > plugin whose nmf has more than kMaxDescriptorsPerMessage resource files. I think > the test is useful regardless of the kMaxPreOpenResourceFiles value. OK, so the test is still covering the fast path. Can you add a TEST= line to the commit message, please, to give the name of the test that's covering this? https://codereview.chromium.org/649603004/diff/420001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/649603004/diff/420001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:242: const std::vector<NaClResourceFileInfo>& resource_files_info, I walked through the IPC flow to refresh my memory about how this works. Currently this happens, in order: * renderer: * DownloadNexe() does a sync IPC to the browser (NaClHostMsg_OpenNaClResources) to fetch files from the manifest file, and receives FDs back (+ NaClFileTokens). * LaunchSelLdr() sends the startup message to the browser, including FDs (+ NaClFileTokens). * browser: Forwards the startup message to the NaCl loader process. This includes FDs. * Under SFI NaCl, it would ignore the FDs it got from the renderer and use the NaClFileTokens to retrieve known-good FDs from a stash (path_cache_ in nacl_browser.cc). * Under Non-SFI NaCl, it would ignore the NaClFileTokens and just use the FDs. Why are we sending the FDs from the browser to the renderer and back again? Instead, how about this: * The renderer's startup message, sent to the browser, can contain a list of <manifest_key, url> pairs. * The browser can map the URLs to filenames, open the files, and send a list of <manifest_key, FD, file_pathname> tuples to the NaCl loader process in the startup message. Would that work OK? I think it would simplify this change a lot. I'm sorry I didn't notice this before. It's a case of not seeing the wood for the trees. It took me a while to understand what's happening in this change. https://codereview.chromium.org/649603004/diff/420001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:899: base::FilePath(), See comment in nonsfi_listener.cc -- you can add a sanity check for this empty filename there. https://codereview.chromium.org/649603004/diff/420001/components/nacl/common/... File components/nacl/common/nacl_host_messages.h (right): https://codereview.chromium.org/649603004/diff/420001/components/nacl/common/... components/nacl/common/nacl_host_messages.h:79: uint64_t /* file_token_hi */); Nit: remove stray semicolon https://codereview.chromium.org/649603004/diff/420001/components/nacl/common/... components/nacl/common/nacl_host_messages.h:113: // open a NaCl executable file from an installed application directory. I think this comment needs updating. https://codereview.chromium.org/649603004/diff/420001/components/nacl/common/... File components/nacl/common/nacl_types.cc (right): https://codereview.chromium.org/649603004/diff/420001/components/nacl/common/... components/nacl/common/nacl_types.cc:29: // '3' is for |nexe_file|, |debug_stub_server_bound_socket|, and Similar to my comment in ppb_nacl_private_impl.cc, what happens if we don't have this check? If there's nothing keeping the value 3 in sync here, could we drop this check? https://codereview.chromium.org/649603004/diff/420001/components/nacl/common/... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/649603004/diff/420001/components/nacl/common/... components/nacl/common/nacl_types.h:72: uint64_t file_token_lo; Could you use NaClFileToken instead of two uint64_t fields here? If necessary, you can move the definition of NaClFileToken from nacl_process_host.h to here. The reasons for passing the two fields around separately are historical. I think it's because NaClFileToken used to be defined in the NaCl repo. Then, rather than doing file_token_lo(0), file_token_hi(0) in constructors, you could just add a constructor to NaClFileToken to initialise that. https://codereview.chromium.org/649603004/diff/420001/components/nacl/common/... components/nacl/common/nacl_types.h:100: std::vector<ResourceFileInfo> resource_files; How about "prefetched_resource_files"? There are various places where you could add a "prefetched_" prefix to make the code more self-describing. https://codereview.chromium.org/649603004/diff/420001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_listener.cc (right): https://codereview.chromium.org/649603004/diff/420001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_listener.cc:162: key_fd_map.insert(std::make_pair( Can you add a check analogous to CHECK(params.nexe_file_path_metadata.empty()); above which ensures that the browser didn't accidentally leak the file's pathname here? https://codereview.chromium.org/649603004/diff/420001/components/nacl/rendere... File components/nacl/renderer/json_manifest.cc (right): https://codereview.chromium.org/649603004/diff/420001/components/nacl/rendere... components/nacl/renderer/json_manifest.cc:447: const std::string& scheme) const { If this arg is always the same, you might as well remove it, and rename the function to GetPrefetchableFiles(). https://codereview.chromium.org/649603004/diff/420001/components/nacl/rendere... components/nacl/renderer/json_manifest.cc:458: if (scheme.empty() || GURL(full_url).SchemeIs(scheme.c_str())) scheme is always passed as "chrome-extension", so you could drop the scheme.empty() check. https://codereview.chromium.org/649603004/diff/420001/components/nacl/rendere... File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/649603004/diff/420001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:79: const size_t kMaxPreOpenResourceFiles = 2; How about adding a TODO to increase this, to document your intentions? https://codereview.chromium.org/649603004/diff/420001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:87: COMPILE_ASSERT(kMaxPreOpenResourceFiles <= What would happen if we didn't have this check? I ask because it's not clear where the number 5 comes from. This could easily get out of sync. If it can't be kept in sync, maybe we should drop the check? https://codereview.chromium.org/649603004/diff/420001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:623: NaClResourceFileInfo file_info; Not used now? https://codereview.chromium.org/649603004/diff/420001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:794: DLOG(ERROR) << "Could not open the main nexe: " << resource_gurls[0]; I don't think you need this special-cased check. Wouldn't this error message be wrong if this is called for open_resource(), to open a single file? https://codereview.chromium.org/649603004/diff/420001/ppapi/api/private/ppb_n... File ppapi/api/private/ppb_nacl_private.idl (right): https://codereview.chromium.org/649603004/diff/420001/ppapi/api/private/ppb_n... ppapi/api/private/ppb_nacl_private.idl:5: If you rebase, you'll find that ppb_nacl_private.idl is gone, so you don't need to change it any more. https://codereview.chromium.org/649603004/diff/420001/ppapi/c/private/ppb_nac... File ppapi/c/private/ppb_nacl_private.h (right): https://codereview.chromium.org/649603004/diff/420001/ppapi/c/private/ppb_nac... ppapi/c/private/ppb_nacl_private.h:194: /* Corresponds to NaClFileInfo in native_client/src/public/nacl_file_info.h */ If you rebase, you'll find this comment is gone (since native_client/src/public/nacl_file_info.h is removed). https://codereview.chromium.org/649603004/diff/420001/ppapi/c/private/ppb_nac... ppapi/c/private/ppb_nacl_private.h:326: PP_Bool download_resource_files, How about "prefetch_resource_files", to be more descriptive? https://codereview.chromium.org/649603004/diff/420001/ppapi/nacl_irt/manifest... File ppapi/nacl_irt/manifest_service.cc (right): https://codereview.chromium.org/649603004/diff/420001/ppapi/nacl_irt/manifest... ppapi/nacl_irt/manifest_service.cc:133: std::map<std::string, int>* g_fds; How about "g_prefetched_fds" to be more descriptive? https://codereview.chromium.org/649603004/diff/420001/ppapi/nacl_irt/manifest... ppapi/nacl_irt/manifest_service.cc:152: #if !defined(OS_NACL_SFI) Can you add a comment like: // Fast path for prefetched FDs.
On 11 February 2015 at 19:57, <mseaborn@chromium.org> wrote: > https://codereview.chromium.org/649603004/diff/420001/ > components/nacl/browser/nacl_process_host.cc#newcode242 > components/nacl/browser/nacl_process_host.cc:242: const > std::vector<NaClResourceFileInfo>& resource_files_info, > I walked through the IPC flow to refresh my memory about how this > works. Currently this happens, in order: > > * renderer: > * DownloadNexe() does a sync IPC to the browser > (NaClHostMsg_OpenNaClResources) to fetch files from the manifest > file, and receives FDs back (+ NaClFileTokens). > * LaunchSelLdr() sends the startup message to the browser, > including FDs (+ NaClFileTokens). > > * browser: Forwards the startup message to the NaCl loader process. > This includes FDs. > * Under SFI NaCl, it would ignore the FDs it got from the renderer > and use the NaClFileTokens to retrieve known-good FDs from a > stash (path_cache_ in nacl_browser.cc). > * Under Non-SFI NaCl, it would ignore the NaClFileTokens and just > use the FDs. > > Why are we sending the FDs from the browser to the renderer and back > again? > > Instead, how about this: > > * The renderer's startup message, sent to the browser, can contain a > list of <manifest_key, url> pairs. > * The browser can map the URLs to filenames, open the files, and send > a list of <manifest_key, FD, file_pathname> tuples to the NaCl > loader process in the startup message. > > Would that work OK? I think it would simplify this change a lot. > > I'm sorry I didn't notice this before. It's a case of not seeing the > wood for the trees. It took me a while to understand what's happening > in this change. It occurs to me that if you use that IPC flow, you can do a further simplification. Rather than sending many pre-opened FDs in one message, it will be easier to stream them in multiple messages. That way you won't hit any arbitrary limits on the number of FDs per message. * The renderer's startup message, sent to the browser, contains the list of many <manifest_key, url> pairs. This is just data, so it doesn't hit any FD limits. * When the browser sends the <manifest_key, FD, file_pathname> tuples to the NaCl loader process, it can send one per message, before finally sending the NaClProcessMsg_Start message. The synchronisation would be straightforward because the loader process basically doesn't do anything before receiving NaClProcessMsg_Start. Cheers, Mark To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Please take another look. Fixed all except the NaClProcessMsg_Start one below: > When the browser sends the <manifest_key, FD, file_pathname> tuples to > the NaCl loader process, it can send one per message, before finally > sending the NaClProcessMsg_Start message. Working around the arbitrary FD limit is nice, but if we implement the idea you proposed, I think we need to send 70+ IPC to the plugin before sending NaClProcessMsg_Start. We could send 7 FDs per message instead of 1, but even if we do that, we still need 10+ IPC. On L version of ARC, the number would be even larger. My understanding is that Chrome IPC can be slowed down for many reasons such as OS thread scheduling and other slow IO, and I'm reluctant to send that many IPC for starting the plugin process. Can I make this a TODO? I still would like to change the max FD constant to something like 128 which should be large enough even for L version of ARC, and, once ARC hits the limit (128) again in the future, I'd work on the TODO to workaround the limit. WDYT? https://codereview.chromium.org/649603004/diff/400001/components/nacl/rendere... File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/649603004/diff/400001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:79: const size_t kMaxPreOpenResourceFiles = 2; On 2015/02/12 03:57:33, Mark Seaborn wrote: > On 2015/02/11 05:54:21, Yusuke Sato wrote: > > On 2015/02/09 04:48:34, Mark Seaborn wrote: > > > Why so low? I thought your aim was to pass more than this. > > > > > > With this set low, I think the fast path won't be covered by the tests you > > added > > > earlier, will it? > > > > This is currently 2 because kMaxDescriptorsPerMessage in ipc/ is still 7. > > > > The test I added is to make sure that the renderer process can start a NaCl > > plugin whose nmf has more than kMaxDescriptorsPerMessage resource files. I > think > > the test is useful regardless of the kMaxPreOpenResourceFiles value. > > OK, so the test is still covering the fast path. Can you add a TEST= line to > the commit message, please, to give the name of the test that's covering this? Done. https://codereview.chromium.org/649603004/diff/420001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/649603004/diff/420001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:242: const std::vector<NaClResourceFileInfo>& resource_files_info, On 2015/02/12 03:57:33, Mark Seaborn wrote: > I walked through the IPC flow to refresh my memory about how this > works. Currently this happens, in order: > > * renderer: > * DownloadNexe() does a sync IPC to the browser > (NaClHostMsg_OpenNaClResources) to fetch files from the manifest > file, and receives FDs back (+ NaClFileTokens). > * LaunchSelLdr() sends the startup message to the browser, > including FDs (+ NaClFileTokens). > > * browser: Forwards the startup message to the NaCl loader process. > This includes FDs. > * Under SFI NaCl, it would ignore the FDs it got from the renderer > and use the NaClFileTokens to retrieve known-good FDs from a > stash (path_cache_ in nacl_browser.cc). > * Under Non-SFI NaCl, it would ignore the NaClFileTokens and just > use the FDs. > > Why are we sending the FDs from the browser to the renderer and back > again? > > Instead, how about this: > > * The renderer's startup message, sent to the browser, can contain a > list of <manifest_key, url> pairs. > * The browser can map the URLs to filenames, open the files, and send > a list of <manifest_key, FD, file_pathname> tuples to the NaCl > loader process in the startup message. > > Would that work OK? I think it would simplify this change a lot. > > I'm sorry I didn't notice this before. It's a case of not seeing the > wood for the trees. It took me a while to understand what's happening > in this change. I didn't do this originally mainly because I wanted to reuse the security checks (in renderer and browser) for open_resource as-is, but yes, the approach is probably too complicated. I've implemented yours with security checks. Done. BTW, opening a main nexe binary with a separate IPC before sending the LaunchNaCl IPC is also unnecessary then (at least when the nexe's scheme is chrome-extension://)? If you think it's possible to remove the IPC for faster and even simpler NaCl process creation, I'll file a Chrome bug. https://codereview.chromium.org/649603004/diff/420001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:899: base::FilePath(), On 2015/02/12 03:57:33, Mark Seaborn wrote: > See comment in nonsfi_listener.cc -- you can add a sanity check for this empty > filename there. Done. https://codereview.chromium.org/649603004/diff/420001/components/nacl/common/... File components/nacl/common/nacl_host_messages.h (right): https://codereview.chromium.org/649603004/diff/420001/components/nacl/common/... components/nacl/common/nacl_host_messages.h:79: uint64_t /* file_token_hi */); On 2015/02/12 03:57:33, Mark Seaborn wrote: > Nit: remove stray semicolon Done. https://codereview.chromium.org/649603004/diff/420001/components/nacl/common/... components/nacl/common/nacl_host_messages.h:113: // open a NaCl executable file from an installed application directory. On 2015/02/12 03:57:33, Mark Seaborn wrote: > I think this comment needs updating. Done (reverted to the original IPC definition) https://codereview.chromium.org/649603004/diff/420001/components/nacl/common/... File components/nacl/common/nacl_types.cc (right): https://codereview.chromium.org/649603004/diff/420001/components/nacl/common/... components/nacl/common/nacl_types.cc:29: // '3' is for |nexe_file|, |debug_stub_server_bound_socket|, and On 2015/02/12 03:57:33, Mark Seaborn wrote: > Similar to my comment in ppb_nacl_private_impl.cc, what happens if we don't have > this check? If there's nothing keeping the value 3 in sync here, could we drop > this check? ok, dropped. https://codereview.chromium.org/649603004/diff/420001/components/nacl/common/... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/649603004/diff/420001/components/nacl/common/... components/nacl/common/nacl_types.h:72: uint64_t file_token_lo; Done (removed the class) https://codereview.chromium.org/649603004/diff/420001/components/nacl/common/... components/nacl/common/nacl_types.h:100: std::vector<ResourceFileInfo> resource_files; On 2015/02/12 03:57:33, Mark Seaborn wrote: > How about "prefetched_resource_files"? > > There are various places where you could add a "prefetched_" prefix to make the > code more self-describing. Done. https://codereview.chromium.org/649603004/diff/420001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_listener.cc (right): https://codereview.chromium.org/649603004/diff/420001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_listener.cc:162: key_fd_map.insert(std::make_pair( On 2015/02/12 03:57:33, Mark Seaborn wrote: > Can you add a check analogous to > CHECK(params.nexe_file_path_metadata.empty()); > above which ensures that the browser didn't accidentally leak the file's > pathname here? Done. https://codereview.chromium.org/649603004/diff/420001/components/nacl/rendere... File components/nacl/renderer/json_manifest.cc (right): https://codereview.chromium.org/649603004/diff/420001/components/nacl/rendere... components/nacl/renderer/json_manifest.cc:447: const std::string& scheme) const { On 2015/02/12 03:57:34, Mark Seaborn wrote: > If this arg is always the same, you might as well remove it, and rename the > function to GetPrefetchableFiles(). Done. https://codereview.chromium.org/649603004/diff/420001/components/nacl/rendere... components/nacl/renderer/json_manifest.cc:458: if (scheme.empty() || GURL(full_url).SchemeIs(scheme.c_str())) On 2015/02/12 03:57:34, Mark Seaborn wrote: > scheme is always passed as "chrome-extension", so you could drop the > scheme.empty() check. Done. https://codereview.chromium.org/649603004/diff/420001/components/nacl/rendere... File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/649603004/diff/420001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:79: const size_t kMaxPreOpenResourceFiles = 2; On 2015/02/12 03:57:34, Mark Seaborn wrote: > How about adding a TODO to increase this, to document your intentions? Done. Moved to nacl_host_message_filter.cc. https://codereview.chromium.org/649603004/diff/420001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:87: COMPILE_ASSERT(kMaxPreOpenResourceFiles <= On 2015/02/12 03:57:34, Mark Seaborn wrote: > What would happen if we didn't have this check? > > I ask because it's not clear where the number 5 comes from. This could easily > get out of sync. If it can't be kept in sync, maybe we should drop the check? Ok, dropped. https://codereview.chromium.org/649603004/diff/420001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:623: NaClResourceFileInfo file_info; On 2015/02/12 03:57:34, Mark Seaborn wrote: > Not used now? Done. https://codereview.chromium.org/649603004/diff/420001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:794: DLOG(ERROR) << "Could not open the main nexe: " << resource_gurls[0]; On 2015/02/12 03:57:34, Mark Seaborn wrote: > I don't think you need this special-cased check. Wouldn't this error message be > wrong if this is called for open_resource(), to open a single file? Done, removed https://codereview.chromium.org/649603004/diff/420001/ppapi/c/private/ppb_nac... File ppapi/c/private/ppb_nacl_private.h (right): https://codereview.chromium.org/649603004/diff/420001/ppapi/c/private/ppb_nac... ppapi/c/private/ppb_nacl_private.h:326: PP_Bool download_resource_files, On 2015/02/12 03:57:34, Mark Seaborn wrote: > How about "prefetch_resource_files", to be more descriptive? Done. (removed) https://codereview.chromium.org/649603004/diff/420001/ppapi/nacl_irt/manifest... File ppapi/nacl_irt/manifest_service.cc (right): https://codereview.chromium.org/649603004/diff/420001/ppapi/nacl_irt/manifest... ppapi/nacl_irt/manifest_service.cc:133: std::map<std::string, int>* g_fds; On 2015/02/12 03:57:34, Mark Seaborn wrote: > How about "g_prefetched_fds" to be more descriptive? Done. https://codereview.chromium.org/649603004/diff/420001/ppapi/nacl_irt/manifest... ppapi/nacl_irt/manifest_service.cc:152: #if !defined(OS_NACL_SFI) On 2015/02/12 03:57:34, Mark Seaborn wrote: > Can you add a comment like: > // Fast path for prefetched FDs. Done.
Mark: ping?
This is a lot simpler now, thanks. My remaining comments are for smaller issues... https://codereview.chromium.org/649603004/diff/460001/components/nacl/browser... File components/nacl/browser/nacl_host_message_filter.cc (right): https://codereview.chromium.org/649603004/diff/460001/components/nacl/browser... components/nacl/browser/nacl_host_message_filter.cc:157: content::RenderViewHost* rvh = content::RenderViewHost::FromID( You're doing things in this PostBlockingPoolTask() callback that I don't think are valid to do on this thread. e.g. How is use of content::RenderViewHost thread-safe? What keeps this reference valid? I think you'd need to do this before PostBlockingPoolTask(), and only call OpenNaClReadExecImpl() from the blocking-pool thread. https://codereview.chromium.org/649603004/diff/460001/components/nacl/browser... components/nacl/browser/nacl_host_message_filter.cc:182: true, // blocking Nit: "use_blocking_api" (for consistency) https://codereview.chromium.org/649603004/diff/460001/components/nacl/browser... components/nacl/browser/nacl_host_message_filter.cc:215: for (size_t i = 0; i < prefetched_resource_files_info.size(); ++i) { This error handling seems unnecessary. When would this PostTask() fail? https://codereview.chromium.org/649603004/diff/460001/components/nacl/browser... components/nacl/browser/nacl_host_message_filter.cc:237: // BatchOpenResourceFiles calls this function again with an empty It seems kind of hacky for a function to call itself back for another iteration. How about splitting this into two functions? https://codereview.chromium.org/649603004/diff/460001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/649603004/diff/460001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:903: if (uses_nonsfi_mode_) { See comment below. If you gate the handling of prefetched_resource_files_info_ on this, is that redundant with the checks in nacl_host_message_filter.cc? https://codereview.chromium.org/649603004/diff/460001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:938: CHECK(prefetched_resource_files_info_.empty()); It's not clear whether this would allow the renderer to crash the browser. (That depends on what nacl_host_message_filter.cc does, I think.) How about making this a DCHECK instead? https://codereview.chromium.org/649603004/diff/460001/components/nacl/common/... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/649603004/diff/460001/components/nacl/common/... components/nacl/common/nacl_types.h:61: struct NaClResourceFileInfo { Add comment: Represents a single prefetched file that's listed in the "files" section of a NaCl manifest file. https://codereview.chromium.org/649603004/diff/460001/components/nacl/common/... components/nacl/common/nacl_types.h:69: base::FilePath file_path; // a key for validation caching How about "file_path_metadata", to match the naming of the field in NaClStartParams? https://codereview.chromium.org/649603004/diff/460001/components/nacl/common/... components/nacl/common/nacl_types.h:136: std::vector<std::pair<std::string, std::string> > prefetched_resource_files; Maybe "resource_files_to_prefetch" (as in other comment)? https://codereview.chromium.org/649603004/diff/460001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_listener.cc (right): https://codereview.chromium.org/649603004/diff/460001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_listener.cc:160: std::map<std::string, int> key_fd_map; Why not use "new" to allocate this here and pass it by pointer to RegisterPreopenedDescriptorsNonSfi()? That saves a copy. https://codereview.chromium.org/649603004/diff/460001/components/nacl/rendere... File components/nacl/renderer/json_manifest.cc (right): https://codereview.chromium.org/649603004/diff/460001/components/nacl/rendere... components/nacl/renderer/json_manifest.cc:451: const std::vector<std::string>& keys = files.getMemberNames(); The other callers of getMemberNames() in this file don't use a reference. https://codereview.chromium.org/649603004/diff/460001/components/nacl/rendere... File components/nacl/renderer/plugin/service_runtime.h (left): https://codereview.chromium.org/649603004/diff/460001/components/nacl/rendere... components/nacl/renderer/plugin/service_runtime.h:22: struct NaClFileInfo; This line is removed in tip-of-tree. Have you run trybots for this change? https://codereview.chromium.org/649603004/diff/460001/components/nacl/rendere... File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/649603004/diff/460001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:409: std::string /*url*/, std::string /*key*/> > prefetched_resource_files; How about "resource_files_to_prefetch", since they're not prefetched yet? https://codereview.chromium.org/649603004/diff/460001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:419: // IMPORTANT SECURITY CHECK: See the comment in OpenNaClResources(). OpenNaClResources() no longer exists, I think. https://codereview.chromium.org/649603004/diff/460001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:420: if (!security_origin.canRequest(gurl)) { Duplicating the two checks that OpenNaClExecutable() worries me a little in case they get out of sync. Could you split the checks into a separate function and use that here? I suspect that plugin_instance->GetContainer()->element().document().securityOrigin() isn't expensive enough that calling it once per file would matter if this is split out.
Please take another look. https://codereview.chromium.org/649603004/diff/460001/components/nacl/browser... File components/nacl/browser/nacl_host_message_filter.cc (right): https://codereview.chromium.org/649603004/diff/460001/components/nacl/browser... components/nacl/browser/nacl_host_message_filter.cc:157: content::RenderViewHost* rvh = content::RenderViewHost::FromID( On 2015/02/25 20:01:23, Mark Seaborn wrote: > You're doing things in this PostBlockingPoolTask() callback that I don't think > are valid to do on this thread. > > e.g. How is use of content::RenderViewHost thread-safe? What keeps this > reference valid? > > I think you'd need to do this before PostBlockingPoolTask(), and only call > OpenNaClReadExecImpl() from the blocking-pool thread. No, it's not valid. RVH::FromID has to be called in the UI thread (like nacl_file_host.cc does). Thanks for catching this. Fixed. https://codereview.chromium.org/649603004/diff/460001/components/nacl/browser... components/nacl/browser/nacl_host_message_filter.cc:166: if (launch_params.uses_nonsfi_mode) { removed the IF https://codereview.chromium.org/649603004/diff/460001/components/nacl/browser... components/nacl/browser/nacl_host_message_filter.cc:182: true, // blocking On 2015/02/25 20:01:23, Mark Seaborn wrote: > Nit: "use_blocking_api" (for consistency) Done. https://codereview.chromium.org/649603004/diff/460001/components/nacl/browser... components/nacl/browser/nacl_host_message_filter.cc:215: for (size_t i = 0; i < prefetched_resource_files_info.size(); ++i) { On 2015/02/25 20:01:23, Mark Seaborn wrote: > This error handling seems unnecessary. When would this PostTask() fail? Removed. Looks like PostTask always returns true as long as a message loop for the thread is available. https://codereview.chromium.org/649603004/diff/460001/components/nacl/browser... components/nacl/browser/nacl_host_message_filter.cc:237: // BatchOpenResourceFiles calls this function again with an empty On 2015/02/25 20:01:23, Mark Seaborn wrote: > It seems kind of hacky for a function to call itself back for another iteration. > How about splitting this into two functions? Done. https://codereview.chromium.org/649603004/diff/460001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/649603004/diff/460001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:903: if (uses_nonsfi_mode_) { On 2015/02/25 20:01:23, Mark Seaborn wrote: > See comment below. If you gate the handling of prefetched_resource_files_info_ > on this, is that redundant with the checks in nacl_host_message_filter.cc? Done. https://codereview.chromium.org/649603004/diff/460001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:938: CHECK(prefetched_resource_files_info_.empty()); On 2015/02/25 20:01:23, Mark Seaborn wrote: > It's not clear whether this would allow the renderer to crash the browser. > (That depends on what nacl_host_message_filter.cc does, I think.) > > How about making this a DCHECK instead? Done. https://codereview.chromium.org/649603004/diff/460001/components/nacl/common/... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/649603004/diff/460001/components/nacl/common/... components/nacl/common/nacl_types.h:61: struct NaClResourceFileInfo { On 2015/02/25 20:01:24, Mark Seaborn wrote: > Add comment: Represents a single prefetched file that's listed in the "files" > section of a NaCl manifest file. Done. https://codereview.chromium.org/649603004/diff/460001/components/nacl/common/... components/nacl/common/nacl_types.h:69: base::FilePath file_path; // a key for validation caching On 2015/02/25 20:01:24, Mark Seaborn wrote: > How about "file_path_metadata", to match the naming of the field in > NaClStartParams? Done. https://codereview.chromium.org/649603004/diff/460001/components/nacl/common/... components/nacl/common/nacl_types.h:136: std::vector<std::pair<std::string, std::string> > prefetched_resource_files; On 2015/02/25 20:01:24, Mark Seaborn wrote: > Maybe "resource_files_to_prefetch" (as in other comment)? Done. https://codereview.chromium.org/649603004/diff/460001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_listener.cc (right): https://codereview.chromium.org/649603004/diff/460001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_listener.cc:160: std::map<std::string, int> key_fd_map; On 2015/02/25 20:01:24, Mark Seaborn wrote: > Why not use "new" to allocate this here and pass it by pointer to > RegisterPreopenedDescriptorsNonSfi()? That saves a copy. Done. https://codereview.chromium.org/649603004/diff/460001/components/nacl/rendere... File components/nacl/renderer/json_manifest.cc (right): https://codereview.chromium.org/649603004/diff/460001/components/nacl/rendere... components/nacl/renderer/json_manifest.cc:451: const std::vector<std::string>& keys = files.getMemberNames(); On 2015/02/25 20:01:24, Mark Seaborn wrote: > The other callers of getMemberNames() in this file don't use a reference. Done. https://codereview.chromium.org/649603004/diff/460001/components/nacl/rendere... File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/649603004/diff/460001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:409: std::string /*url*/, std::string /*key*/> > prefetched_resource_files; On 2015/02/25 20:01:24, Mark Seaborn wrote: > How about "resource_files_to_prefetch", since they're not prefetched yet? Done. https://codereview.chromium.org/649603004/diff/460001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:419: // IMPORTANT SECURITY CHECK: See the comment in OpenNaClResources(). On 2015/02/25 20:01:24, Mark Seaborn wrote: > OpenNaClResources() no longer exists, I think. Removed the comment https://codereview.chromium.org/649603004/diff/460001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:420: if (!security_origin.canRequest(gurl)) { On 2015/02/25 20:01:24, Mark Seaborn wrote: > Duplicating the two checks that OpenNaClExecutable() worries me a little in case > they get out of sync. Could you split the checks into a separate function and > use that here? > > I suspect that > plugin_instance->GetContainer()->element().document().securityOrigin() isn't > expensive enough that calling it once per file would matter if this is split > out. Done.
You didn't have trybot runs for the change, so I kicked some off. There are some failures on Windows -- can you check whether they might be related to the change? Otherwise this is very very close.
LGTM with the changes below, and please check that the trybot runs' test failures aren't flakiness caused by this change. https://codereview.chromium.org/649603004/diff/480001/components/nacl/browser... File components/nacl/browser/nacl_process_host.h (right): https://codereview.chromium.org/649603004/diff/480001/components/nacl/browser... components/nacl/browser/nacl_process_host.h:54: // nexe_token: A cache validation token for nexe_file. Nit: update this comment with the extra arg https://codereview.chromium.org/649603004/diff/480001/components/nacl/common/... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/649603004/diff/480001/components/nacl/common/... components/nacl/common/nacl_types.h:122: // A pair of resource URL and its manifest key. Nit: put the comment on the field instead? Would it be more conventional to put the manifest key first? That would also match the std::pair you use in the loader process, which contains (key, FD). https://codereview.chromium.org/649603004/diff/480001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_listener.cc (right): https://codereview.chromium.org/649603004/diff/480001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_listener.cc:164: params.prefetched_resource_files[i].file_key, Nit: Can you make it consistent whether we use (key, value) or (value, key)? https://codereview.chromium.org/649603004/diff/480001/components/nacl/rendere... File components/nacl/renderer/json_manifest.h (right): https://codereview.chromium.org/649603004/diff/480001/components/nacl/rendere... components/nacl/renderer/json_manifest.h:43: // Gets all the URLs and their keys in the "files" section that are Nit: similar comment about (key, value) vs. (value, key). https://codereview.chromium.org/649603004/diff/480001/components/nacl/rendere... File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/649603004/diff/480001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:92: bool CheckSecurityOrigin(content::PepperPluginInstance* plugin_instance, If the SchemeIs("chrome-extension") check moves here, this could be named CanOpenViaFastPath(), perhaps? https://codereview.chromium.org/649603004/diff/480001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:421: if (process_type == kNativeNaClProcessType) { This should be conditionalised on Non-SFI mode for now. Otherwise Chromium will potentially be doing some unnecessary work to pre-open these files in SFI mode. (I think it was conditionalised before but that got lost in the refactoring?) https://codereview.chromium.org/649603004/diff/480001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:427: DCHECK(gurl.SchemeIs("chrome-extension")); I had in mind that this check would move into CheckSecurityOrigin() and be shared. https://codereview.chromium.org/649603004/diff/480001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:428: // IMPORTANT SECURITY CHECK. DO NOT REMOVE. Nit: doesn't really need caps. :-) The caps are kind of an implicit way of saying "we don't have test coverage for this security check, in the style of 'goto fail'". :-/ (Obviously that's not your fault, and we don't have the infrastructure to test for this straightforwardly.) https://codereview.chromium.org/649603004/diff/480001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:749: if (!gurl.SchemeIs("chrome-extension")) Ditto: I had in mind that this check would move into CheckSecurityOrigin() and be shared.
Thanks. I've confirmed all try bots are green. https://codereview.chromium.org/649603004/diff/480001/components/nacl/browser... File components/nacl/browser/nacl_process_host.h (right): https://codereview.chromium.org/649603004/diff/480001/components/nacl/browser... components/nacl/browser/nacl_process_host.h:54: // nexe_token: A cache validation token for nexe_file. On 2015/03/04 05:07:30, Mark Seaborn wrote: > Nit: update this comment with the extra arg Done. https://codereview.chromium.org/649603004/diff/480001/components/nacl/common/... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/649603004/diff/480001/components/nacl/common/... components/nacl/common/nacl_types.h:122: // A pair of resource URL and its manifest key. On 2015/03/04 05:07:30, Mark Seaborn wrote: > Nit: put the comment on the field instead? > > Would it be more conventional to put the manifest key first? That would also > match the std::pair you use in the loader process, which contains (key, FD). Done. https://codereview.chromium.org/649603004/diff/480001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_listener.cc (right): https://codereview.chromium.org/649603004/diff/480001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_listener.cc:164: params.prefetched_resource_files[i].file_key, On 2015/03/04 05:07:30, Mark Seaborn wrote: > Nit: Can you make it consistent whether we use (key, value) or (value, key)? I'd use (key, value) everywhere. Done. (this part is unchanged though) https://codereview.chromium.org/649603004/diff/480001/components/nacl/rendere... File components/nacl/renderer/json_manifest.h (right): https://codereview.chromium.org/649603004/diff/480001/components/nacl/rendere... components/nacl/renderer/json_manifest.h:43: // Gets all the URLs and their keys in the "files" section that are On 2015/03/04 05:07:30, Mark Seaborn wrote: > Nit: similar comment about (key, value) vs. (value, key). Done. https://codereview.chromium.org/649603004/diff/480001/components/nacl/rendere... File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/649603004/diff/480001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:92: bool CheckSecurityOrigin(content::PepperPluginInstance* plugin_instance, On 2015/03/04 05:07:31, Mark Seaborn wrote: > If the SchemeIs("chrome-extension") check moves here, this could be named > CanOpenViaFastPath(), perhaps? Done. https://codereview.chromium.org/649603004/diff/480001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:421: if (process_type == kNativeNaClProcessType) { On 2015/03/04 05:07:31, Mark Seaborn wrote: > This should be conditionalised on Non-SFI mode for now. Otherwise Chromium will > potentially be doing some unnecessary work to pre-open these files in SFI mode. > > (I think it was conditionalised before but that got lost in the refactoring?) Done. https://codereview.chromium.org/649603004/diff/480001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:427: DCHECK(gurl.SchemeIs("chrome-extension")); On 2015/03/04 05:07:30, Mark Seaborn wrote: > I had in mind that this check would move into CheckSecurityOrigin() and be > shared. Done. Removed. https://codereview.chromium.org/649603004/diff/480001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:428: // IMPORTANT SECURITY CHECK. DO NOT REMOVE. On 2015/03/04 05:07:31, Mark Seaborn wrote: > Nit: doesn't really need caps. :-) > > The caps are kind of an implicit way of saying "we don't have test coverage for > this security check, in the style of 'goto fail'". :-/ (Obviously that's not > your fault, and we don't have the infrastructure to test for this > straightforwardly.) Done. https://codereview.chromium.org/649603004/diff/480001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:749: if (!gurl.SchemeIs("chrome-extension")) On 2015/03/04 05:07:30, Mark Seaborn wrote: > Ditto: I had in mind that this check would move into CheckSecurityOrigin() and > be shared. Done.
yusukes@chromium.org changed reviewers: + jln@chromium.org
Julien, could you take a look at this CL? This is for ARC, and adds a parameter to NaClHostMsg_LaunchNaCl and NaClProcessMsg_Start.
LGTM
Julien: ping?
*_messages.h lgtm, mostly based on Mark's review.
The CQ bit was checked by yusukes@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from teravest@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/649603004/#ps520001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/649603004/520001
Message was sent while issue was closed.
Committed patchset #20 (id:520001)
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/67e8acdac9e043dd710058ef2e01ca7ad2b29825 Cr-Commit-Position: refs/heads/master@{#319931} |
