NaCl: Stop sending resource FDs with NaClProcessMsg_Start

NaCl: Stop sending resource FDs with NaClProcessMsg_Start

Instead, define NaClProcessMsg_AddPrefetchedResource and send them
with multiple NaClProcessMsg_AddPrefetchedResource IPCs. That way,
we can send an arbitrary number of FDs to a plugin process ignoring
the IPC::MessageAttachmentSet::kMaxDescriptorsPerMessage limit.

BUG=nativeclient:3802
BUG=348232
TEST=browser_tests (*PackagedAppTest.SuccessfulLoad)
TEST=manually tested SFI ARC, non-SFI ARC, and PNaCl demo apps

Committed: https://crrev.com/68b681edd264a42275353fe4abeb563e0f1cbce9
Cr-Commit-Position: refs/heads/master@{#329030}

[Yusuke Sato, 2015-05-06 17:57:08]

Patchset #2 (id:20001) has been deleted

[Yusuke Sato, 2015-05-06 18:01:21]

yusukes@chromium.org changed reviewers:
+ hidehiko@chromium.org, mseaborn@chromium.org

[Yusuke Sato, 2015-05-06 18:01:22]

Please take a look.

I won't submit this until https://codereview.chromium.org/1127913002/ is
submitted, but I'd like to start code review now. I ran git-cl try for this CL +
https://codereview.chromium.org/1127913002/, and all tests passed (see Patch Set
1).

Re performance, you're right. I compared the current tot Chrome with this CL +
https://codereview.chromium.org/1127913002/ using ARC's benchmark tool, and
didn't see any performance regression. Thanks for the good suggestion.

[Yusuke Sato, 2015-05-06 19:52:28]

https://codereview.chromium.org/1127913002/ has been submitted. Rebased this on
top of the IPC change and ran try bots again, just in case.

[Mark Seaborn, 2015-05-08 00:35:36]

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/browser...
File components/nacl/browser/nacl_host_message_filter.cc (left):

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/browser...
components/nacl/browser/nacl_host_message_filter.cc:234: if
(prefetched_resource_files.size() >= kMaxPreOpenResourceFiles)
I think it would be prudent to keep some limit on the number of FDs/handles, as
a sanity check.  Otherwise, untrusted code could attempt to prefetch hundreds or
thousands of files, which would definitely exhaust the FD/handle table, which
could expose error handling bugs.

Especially because you're open()ing all the files and then Send()ing them,
rather than open()ing and Send()ing each file in turn.

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:77:
static_assert(kMaxDescriptorsPerMessage ==
You could do:

#if defined(OS_POSIX)
static const size_t kMaxDescriptorsPerMessage =
IPC::MessageAttachmentSet::kMaxDescriptorsPerMessage;
#elif defined(OS_WIN)
// Windows imposes no limit...
static const size_t kMaxDescriptorsPerMessage = 100;
#endif

This way the OS_POSIX number isn't duplicated here.

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:895: if
(prefetched_resource_files_to_send.size() >= kMaxDescriptorsPerMessage ||
Can you check whether this batching of multiple FDs per message affects
performance at all?  If it doesn't, it would be slightly simpler to send one FD
per message.

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/common/...
File components/nacl/common/nacl_messages.h (right):

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/common/...
components/nacl/common/nacl_messages.h:50: // Sends prefetched resource files to
a NaCl loader process.
Can you document the ordering here?  Which messages can be sent multiple times
and which only once?

This message comes before NaClProcessMsg_Start, right?  It would be good to
order it before in the file, too.

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/common/...
components/nacl/common/nacl_messages.h:51:
IPC_MESSAGE_CONTROL1(NaClProcessMsg_SetPrefetchedResource,
Naming nit: How about "Add" rather than "Set"?

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/loader/...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:312: void
NaClListener::OnSetPrefetchedResource(
It might be worth adding a sanity check to ensure this isn't called after
OnStart().

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/loader/...
File components/nacl/loader/nonsfi/nonsfi_listener.cc (right):

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_listener.cc:40: 
Nit: don't add an empty line here

[Yusuke Sato, 2015-05-08 15:12:35]

Thanks for the review. Please take another look.

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/browser...
File components/nacl/browser/nacl_host_message_filter.cc (left):

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/browser...
components/nacl/browser/nacl_host_message_filter.cc:234: if
(prefetched_resource_files.size() >= kMaxPreOpenResourceFiles)
On 2015/05/08 00:35:36, Mark Seaborn wrote:
> I think it would be prudent to keep some limit on the number of FDs/handles,
as
> a sanity check.  Otherwise, untrusted code could attempt to prefetch hundreds
or
> thousands of files, which would definitely exhaust the FD/handle table, which
> could expose error handling bugs.
> 
> Especially because you're open()ing all the files and then Send()ing them,
> rather than open()ing and Send()ing each file in turn.

Done.

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:77:
static_assert(kMaxDescriptorsPerMessage ==
Removed the code. Done.

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:895: if
(prefetched_resource_files_to_send.size() >= kMaxDescriptorsPerMessage ||
On 2015/05/08 00:35:36, Mark Seaborn wrote:
> Can you check whether this batching of multiple FDs per message affects
> performance at all?  If it doesn't, it would be slightly simpler to send one
FD
> per message.

Done.

Checked performance, and didn't see any difference between 1 per message and 7
per message. Changed to 1.

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/common/...
File components/nacl/common/nacl_messages.h (right):

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/common/...
components/nacl/common/nacl_messages.h:50: // Sends prefetched resource files to
a NaCl loader process.
On 2015/05/08 00:35:36, Mark Seaborn wrote:
> Can you document the ordering here?  Which messages can be sent multiple times
> and which only once?
> 
> This message comes before NaClProcessMsg_Start, right?  It would be good to
> order it before in the file, too.

Done.

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/common/...
components/nacl/common/nacl_messages.h:51:
IPC_MESSAGE_CONTROL1(NaClProcessMsg_SetPrefetchedResource,
On 2015/05/08 00:35:36, Mark Seaborn wrote:
> Naming nit: How about "Add" rather than "Set"?

Done.

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/loader/...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:312: void
NaClListener::OnSetPrefetchedResource(
On 2015/05/08 00:35:36, Mark Seaborn wrote:
> It might be worth adding a sanity check to ensure this isn't called after
> OnStart().

Done.

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/loader/...
File components/nacl/loader/nonsfi/nonsfi_listener.cc (right):

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_listener.cc:40: 
On 2015/05/08 00:35:36, Mark Seaborn wrote:
> Nit: don't add an empty line here

Done.

[Mark Seaborn, 2015-05-08 17:18:24]

LGTM.  Note to IPC message reviewers: I did review the change to nacl_messages.h
for security.

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:895: if
(prefetched_resource_files_to_send.size() >= kMaxDescriptorsPerMessage ||
On 2015/05/08 15:12:34, Yusuke Sato wrote:
> On 2015/05/08 00:35:36, Mark Seaborn wrote:
> > Can you check whether this batching of multiple FDs per message affects
> > performance at all?  If it doesn't, it would be slightly simpler to send
> > one FD per message.
> 
> Done.
> 
> Checked performance, and didn't see any difference between 1 per message and 7
> per message. Changed to 1.

Great, thanks for checking!

Does this version perform as well as when Chromium was sending ~100 FDs in a
single IPC message?

https://codereview.chromium.org/1125043002/diff/80001/components/nacl/browser...
File components/nacl/browser/nacl_host_message_filter.cc (right):

https://codereview.chromium.org/1125043002/diff/80001/components/nacl/browser...
components/nacl/browser/nacl_host_message_filter.cc:27: // The maximum number of
resource file handles the browser process accepts. Use
Please explain in the comment why we're imposing a maximum, i.e. to prevent
untrusted code filling the FD/handle table.

https://codereview.chromium.org/1125043002/diff/80001/components/nacl/browser...
components/nacl/browser/nacl_host_message_filter.cc:29: const size_t
kMaxPreOpenResourceFiles = 192;
How about making this a round number, like 200?

https://codereview.chromium.org/1125043002/diff/80001/components/nacl/loader/...
File components/nacl/loader/nonsfi/nonsfi_listener.h (right):

https://codereview.chromium.org/1125043002/diff/80001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_listener.h:54: bool is_started_;
You don't really need the is_started_ check in the Non-SFI version because
there's no security concern here: this code is untrusted.  (I should have
mentioned that before.)

[Yusuke Sato, 2015-05-08 18:41:17]

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/1125043002/diff/60001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:895: if
(prefetched_resource_files_to_send.size() >= kMaxDescriptorsPerMessage ||
On 2015/05/08 17:18:24, Mark Seaborn wrote:
> On 2015/05/08 15:12:34, Yusuke Sato wrote:
> > On 2015/05/08 00:35:36, Mark Seaborn wrote:
> > > Can you check whether this batching of multiple FDs per message affects
> > > performance at all?  If it doesn't, it would be slightly simpler to send
> > > one FD per message.
> > 
> > Done.
> > 
> > Checked performance, and didn't see any difference between 1 per message and
7
> > per message. Changed to 1.
> 
> Great, thanks for checking!
> 
> Does this version perform as well as when Chromium was sending ~100 FDs in a
> single IPC message?

Yes it does.

https://codereview.chromium.org/1125043002/diff/80001/components/nacl/browser...
File components/nacl/browser/nacl_host_message_filter.cc (right):

https://codereview.chromium.org/1125043002/diff/80001/components/nacl/browser...
components/nacl/browser/nacl_host_message_filter.cc:27: // The maximum number of
resource file handles the browser process accepts. Use
On 2015/05/08 17:18:24, Mark Seaborn wrote:
> Please explain in the comment why we're imposing a maximum, i.e. to prevent
> untrusted code filling the FD/handle table.

Done.

https://codereview.chromium.org/1125043002/diff/80001/components/nacl/browser...
components/nacl/browser/nacl_host_message_filter.cc:29: const size_t
kMaxPreOpenResourceFiles = 192;
On 2015/05/08 17:18:24, Mark Seaborn wrote:
> How about making this a round number, like 200?

Done.

https://codereview.chromium.org/1125043002/diff/80001/components/nacl/loader/...
File components/nacl/loader/nonsfi/nonsfi_listener.h (right):

https://codereview.chromium.org/1125043002/diff/80001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_listener.h:54: bool is_started_;
On 2015/05/08 17:18:24, Mark Seaborn wrote:
> You don't really need the is_started_ check in the Non-SFI version because
> there's no security concern here: this code is untrusted.  (I should have
> mentioned that before.)

Removed, done.

[Yusuke Sato, 2015-05-08 18:41:55]

yusukes@chromium.org changed reviewers:
+ jln@chromium.org

[Yusuke Sato, 2015-05-08 18:41:56]

Julien, could you take a look at components/nacl/common/nacl_messages.h?

[jln (very slow on Chromium), 2015-05-08 20:53:01]

lgtm based on a quick read and Mark's review.

[Yusuke Sato, 2015-05-08 21:21:19]

The CQ bit was checked by yusukes@chromium.org

[Yusuke Sato, 2015-05-08 21:21:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
mseaborn@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1125043002/#ps100001 (title: "address
comments")

[commit-bot: I haz the power, 2015-05-08 21:25:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1125043002/100001

[commit-bot: I haz the power, 2015-05-09 01:47:53]

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

[commit-bot: I haz the power, 2015-05-09 01:47:54]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)

[Yusuke Sato, 2015-05-09 01:56:52]

The CQ bit was checked by yusukes@chromium.org

[commit-bot: I haz the power, 2015-05-09 02:09:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1125043002/100001

[commit-bot: I haz the power, 2015-05-09 02:49:06]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-05-09 02:50:51]

Patchset 5 (id:??) landed as
https://crrev.com/68b681edd264a42275353fe4abeb563e0f1cbce9
Cr-Commit-Position: refs/heads/master@{#329030}