Reduce resource leak code for StartNaClExecution.

Reduce resource leak code for StartNaClExecution.

Specifically on error handling code, there are some resource
leak code path or potential crash code path.
The root cause is; currently the code to open resources used by
a plugin is distributed. So, if an error is found after some
resources in the plugin is created, it would be leaked.
With this CL, those resource opening is delayed until we decide
to send an IPC to plugin (in StartNaClExecution), and whenever
we decide to send (i.e. once we start opening resources
for the plugin), always the IPC is sent even some error (like
resource opening failure) is found by deferring the error handling
to the plugin.
Note that the biggest code change of this CL is moving
BatchOpenResourceFiles from nacl_host_message_filter to nacl_process_host.

TEST=Ran bots.
BUG=https://code.google.com/p/nativeclient/issues/detail?id=3802

[hidehiko, 2015-04-30 13:39:03]

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

[hidehiko, 2015-04-30 13:39:04]

This is the remaining part of
https://codereview.chromium.org/1085583005/#ps20001.

PTAL.
- hidehiko

[hidehiko, 2015-05-01 16:27:48]

Friendly ping?

On 2015/04/30 13:39:04, hidehiko wrote:
> This is the remaining part of
> https://codereview.chromium.org/1085583005/#ps20001.
> 
> PTAL.
> - hidehiko

[hidehiko, 2015-05-04 16:23:10]

Friendly ping?

On 2015/05/01 16:27:48, hidehiko wrote:
> Friendly ping?
> 
> On 2015/04/30 13:39:04, hidehiko wrote:
> > This is the remaining part of
> > https://codereview.chromium.org/1085583005/#ps20001.
> > 
> > PTAL.
> > - hidehiko

[hidehiko, 2015-05-06 13:16:58]

Friendly ping?

On 2015/05/04 16:23:10, hidehiko wrote:
> Friendly ping?
> 
> On 2015/05/01 16:27:48, hidehiko wrote:
> > Friendly ping?
> > 
> > On 2015/04/30 13:39:04, hidehiko wrote:
> > > This is the remaining part of
> > > https://codereview.chromium.org/1085583005/#ps20001.
> > > 
> > > PTAL.
> > > - hidehiko

[Mark Seaborn, 2015-05-08 00:02:02]

https://codereview.chromium.org/1117883002/diff/1/components/nacl/browser/nac...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/1117883002/diff/1/components/nacl/browser/nac...
components/nacl/browser/nacl_process_host.cc:947: params.irt_handle =
IPC::GetFileHandleForProcess(
So you're changing the browser process to no longer handle errors if these
various GetFileHandleForProcess()/ShareToProcess() calls fail.

If one of these does fail, what do you expect to happen?  Are you expecting
nacl_listener.cc (in the loader process) to either exit if the FD/handle is
missing or carry on without the FD/handle?

[hidehiko, 2015-05-11 16:49:58]

Patchset #2 (id:20001) has been deleted

[hidehiko, 2015-05-11 16:50:07]

Patchset #2 (id:40001) has been deleted

[hidehiko, 2015-05-11 16:52:49]

Rebased, as some more changes around the code are landed.
PTAL.

https://codereview.chromium.org/1117883002/diff/1/components/nacl/browser/nac...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/1117883002/diff/1/components/nacl/browser/nac...
components/nacl/browser/nacl_process_host.cc:947: params.irt_handle =
IPC::GetFileHandleForProcess(
On 2015/05/08 00:02:02, Mark Seaborn wrote:
> So you're changing the browser process to no longer handle errors if these
> various GetFileHandleForProcess()/ShareToProcess() calls fail.
> 
> If one of these does fail, what do you expect to happen?  Are you expecting
> nacl_listener.cc (in the loader process) to either exit if the FD/handle is
> missing or carry on without the FD/handle?

My intention is "deferring the error handling to the peer (nacl_listener)".
Currently it handles errors with CHECK so that it would be aborted on failure.

[hidehiko, 2015-05-12 18:48:21]

Ping.

On 2015/05/11 16:52:49, hidehiko wrote:
> Rebased, as some more changes around the code are landed.
> PTAL.
> 
>
https://codereview.chromium.org/1117883002/diff/1/components/nacl/browser/nac...
> File components/nacl/browser/nacl_process_host.cc (right):
> 
>
https://codereview.chromium.org/1117883002/diff/1/components/nacl/browser/nac...
> components/nacl/browser/nacl_process_host.cc:947: params.irt_handle =
> IPC::GetFileHandleForProcess(
> On 2015/05/08 00:02:02, Mark Seaborn wrote:
> > So you're changing the browser process to no longer handle errors if these
> > various GetFileHandleForProcess()/ShareToProcess() calls fail.
> > 
> > If one of these does fail, what do you expect to happen?  Are you expecting
> > nacl_listener.cc (in the loader process) to either exit if the FD/handle is
> > missing or carry on without the FD/handle?
> 
> My intention is "deferring the error handling to the peer (nacl_listener)".
> Currently it handles errors with CHECK so that it would be aborted on failure.

[hidehiko, 2015-05-19 12:35:47]

CC+=yusukes@.

Mark, according to offline discussion with Julien and Justin,
as for resource (FDs/HANDLEs management), this CL's way seems
a better approach under the current code base.
Can you resume the review?

Note: Previously, the main goal of the CL was to consolidate a way to
handle errors about sending IPC with FDs/HANDLEs more properly in order to
remove socketpair() from nacl_helper_nonsfi.
Now, it's done without this CL. So it's out of scope.

However, I still think this CL is valuable because this CL
supports prefetch-FDs/HANDLEs in SFI-mode NaCl on Windows, too.
cf) https://code.google.com/p/nativeclient/issues/detail?id=3802
As yusukes@ is transferring to another team,
I'm taking over the remaining task, as this CL covers it
(and assuming this CL is somehow acceptable for NaCl team).

So, PTAL.

yusukes@, if you have a bit time, I'd appreciate if you could take a look at
this CL, too, to make sure this CL does what you planned.

Thanks,
- hidehiko

On 2015/05/12 18:48:21, hidehiko wrote:
> Ping.
> 
> On 2015/05/11 16:52:49, hidehiko wrote:
> > Rebased, as some more changes around the code are landed.
> > PTAL.
> > 
> >
>
https://codereview.chromium.org/1117883002/diff/1/components/nacl/browser/nac...
> > File components/nacl/browser/nacl_process_host.cc (right):
> > 
> >
>
https://codereview.chromium.org/1117883002/diff/1/components/nacl/browser/nac...
> > components/nacl/browser/nacl_process_host.cc:947: params.irt_handle =
> > IPC::GetFileHandleForProcess(
> > On 2015/05/08 00:02:02, Mark Seaborn wrote:
> > > So you're changing the browser process to no longer handle errors if these
> > > various GetFileHandleForProcess()/ShareToProcess() calls fail.
> > > 
> > > If one of these does fail, what do you expect to happen?  Are you
expecting
> > > nacl_listener.cc (in the loader process) to either exit if the FD/handle
is
> > > missing or carry on without the FD/handle?
> > 
> > My intention is "deferring the error handling to the peer (nacl_listener)".
> > Currently it handles errors with CHECK so that it would be aborted on
failure.

[Mark Seaborn, 2015-05-25 19:48:55]

https://codereview.chromium.org/1117883002/diff/1/components/nacl/browser/nac...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/1117883002/diff/1/components/nacl/browser/nac...
components/nacl/browser/nacl_process_host.cc:947: params.irt_handle =
IPC::GetFileHandleForProcess(
On 2015/05/11 16:52:49, hidehiko wrote:
> On 2015/05/08 00:02:02, Mark Seaborn wrote:
> > So you're changing the browser process to no longer handle errors if these
> > various GetFileHandleForProcess()/ShareToProcess() calls fail.
> > 
> > If one of these does fail, what do you expect to happen?  Are you expecting
> > nacl_listener.cc (in the loader process) to either exit if the FD/handle is
> > missing or carry on without the FD/handle?
> 
> My intention is "deferring the error handling to the peer (nacl_listener)".
> Currently it handles errors with CHECK so that it would be aborted on failure.

OK.  If you want to say that the NaClProcessMsg_Start message might be
incomplete and that nacl_listener.cc ought to check for that, then I think you
should make that clearer in nacl_listener.cc by putting the checks together,
with a comment.  (Similar to the checks you added earlier in
nonsfi/nonsfi_listener.cc.)


However, I've been thinking about this, and I think that cleaning up the IPC
layer to transfer Windows handles more cleanly might not be too hard.

Could we get the IPC layer to do the DuplicateHandle() calls *after* we do
channel->Send(message)?  This would involve changing:

 * ipc_message_attachment.h: Unconditionalise TakePlatformFile().

 * ipc/ipc_platform_file_attachment_posix.cc: Add a version for Windows handles.

 * ipc_message_attachment_set.h: Add Windows equivalent of
AddDescriptorsToOwn().

 * ipc_message_utils.cc: Add Windows equivalent of
ParamTraits<base::FileDescriptor>::Write() -- perhaps
ParamTraits<base::File>::Write().  We would allow putting base::Files in IPC
messages and have these transferred automatically without the user of the IPC
layer having to call IPC::Get/TakeFileHandleForProcess() or see
IPC::PlatformFileForTransits.

 * ipc_channel_win.cc: Add logic to tack duplicated handles onto a message,
similar to the code that uses msg->attachment_set()->PeekDescriptors() in
ipc_channel_posix.cc.  To start with, we'd just handle the case of sending
messages from the browser process to sandboxed processes.


This would make the error handling easier because all the DuplicateHandle()
calls would be done in one loop.  If one call fails, we can easily free the
remaining handles.

That would be easier to reason about than putting workarounds into users of the
IPC layer (such as nacl_process_host.cc), which is rather fragile because the
error paths don't get tested.

What do you think about that idea?

[hidehiko, 2015-05-26 14:32:59]

PTAL.

https://codereview.chromium.org/1117883002/diff/1/components/nacl/browser/nac...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/1117883002/diff/1/components/nacl/browser/nac...
components/nacl/browser/nacl_process_host.cc:947: params.irt_handle =
IPC::GetFileHandleForProcess(
On 2015/05/25 19:48:55, Mark Seaborn wrote:
> On 2015/05/11 16:52:49, hidehiko wrote:
> > On 2015/05/08 00:02:02, Mark Seaborn wrote:
> > > So you're changing the browser process to no longer handle errors if these
> > > various GetFileHandleForProcess()/ShareToProcess() calls fail.
> > > 
> > > If one of these does fail, what do you expect to happen?  Are you
expecting
> > > nacl_listener.cc (in the loader process) to either exit if the FD/handle
is
> > > missing or carry on without the FD/handle?
> > 
> > My intention is "deferring the error handling to the peer (nacl_listener)".
> > Currently it handles errors with CHECK so that it would be aborted on
failure.
> 
> OK.  If you want to say that the NaClProcessMsg_Start message might be
> incomplete and that nacl_listener.cc ought to check for that, then I think you
> should make that clearer in nacl_listener.cc by putting the checks together,
> with a comment.  (Similar to the checks you added earlier in
> nonsfi/nonsfi_listener.cc.)
> 

Done.

> 
> However, I've been thinking about this, and I think that cleaning up the IPC
> layer to transfer Windows handles more cleanly might not be too hard.
> 
> Could we get the IPC layer to do the DuplicateHandle() calls *after* we do
> channel->Send(message)?  This would involve changing:
> 
>  * ipc_message_attachment.h: Unconditionalise TakePlatformFile().
> 
>  * ipc/ipc_platform_file_attachment_posix.cc: Add a version for Windows
handles.
> 
>  * ipc_message_attachment_set.h: Add Windows equivalent of
> AddDescriptorsToOwn().
> 
>  * ipc_message_utils.cc: Add Windows equivalent of
> ParamTraits<base::FileDescriptor>::Write() -- perhaps
> ParamTraits<base::File>::Write().  We would allow putting base::Files in IPC
> messages and have these transferred automatically without the user of the IPC
> layer having to call IPC::Get/TakeFileHandleForProcess() or see
> IPC::PlatformFileForTransits.
> 
>  * ipc_channel_win.cc: Add logic to tack duplicated handles onto a message,
> similar to the code that uses msg->attachment_set()->PeekDescriptors() in
> ipc_channel_posix.cc.  To start with, we'd just handle the case of sending
> messages from the browser process to sandboxed processes.
> 
> 
> This would make the error handling easier because all the DuplicateHandle()
> calls would be done in one loop.  If one call fails, we can easily free the
> remaining handles.
> 
> That would be easier to reason about than putting workarounds into users of
the
> IPC layer (such as nacl_process_host.cc), which is rather fragile because the
> error paths don't get tested.
> 
> What do you think about that idea?

IIUC, this has same problem; if DuplicateHandle() fails, after some
DuplicateHandle() succeeds, in ipc_channel_win, it means some HANDLE is already
opened in the callee process.

I'm happy to continue the discussion if you'd like, but it should be separated
from this CL, IMO, because such a change needs to be applied to all IPC layers.
Probably, it's better to involve IPC experts and Win experts in the discussion.

On the other hand, the goal of this CL is to complete the yusukes@'s remaining
task for pre-FD-open on Win. I prefer making changes step-by-step.

[jschuh, 2015-05-27 04:28:24]

On 2015/05/26 14:32:59, hidehiko wrote:
> PTAL.
> 
>
https://codereview.chromium.org/1117883002/diff/1/components/nacl/browser/nac...
> File components/nacl/browser/nacl_process_host.cc (right):
> 
>
https://codereview.chromium.org/1117883002/diff/1/components/nacl/browser/nac...
> components/nacl/browser/nacl_process_host.cc:947: params.irt_handle =
> IPC::GetFileHandleForProcess(
> On 2015/05/25 19:48:55, Mark Seaborn wrote:
> > On 2015/05/11 16:52:49, hidehiko wrote:
> > > On 2015/05/08 00:02:02, Mark Seaborn wrote:
> > > > So you're changing the browser process to no longer handle errors if
these
> > > > various GetFileHandleForProcess()/ShareToProcess() calls fail.
> > > > 
> > > > If one of these does fail, what do you expect to happen?  Are you
> expecting
> > > > nacl_listener.cc (in the loader process) to either exit if the FD/handle
> is
> > > > missing or carry on without the FD/handle?
> > > 
> > > My intention is "deferring the error handling to the peer
(nacl_listener)".
> > > Currently it handles errors with CHECK so that it would be aborted on
> failure.
> > 
> > OK.  If you want to say that the NaClProcessMsg_Start message might be
> > incomplete and that nacl_listener.cc ought to check for that, then I think
you
> > should make that clearer in nacl_listener.cc by putting the checks together,
> > with a comment.  (Similar to the checks you added earlier in
> > nonsfi/nonsfi_listener.cc.)
> > 
> 
> Done.
> 
> > 
> > However, I've been thinking about this, and I think that cleaning up the IPC
> > layer to transfer Windows handles more cleanly might not be too hard.
> > 
> > Could we get the IPC layer to do the DuplicateHandle() calls *after* we do
> > channel->Send(message)?  This would involve changing:
> > 
> >  * ipc_message_attachment.h: Unconditionalise TakePlatformFile().
> > 
> >  * ipc/ipc_platform_file_attachment_posix.cc: Add a version for Windows
> handles.
> > 
> >  * ipc_message_attachment_set.h: Add Windows equivalent of
> > AddDescriptorsToOwn().
> > 
> >  * ipc_message_utils.cc: Add Windows equivalent of
> > ParamTraits<base::FileDescriptor>::Write() -- perhaps
> > ParamTraits<base::File>::Write().  We would allow putting base::Files in IPC
> > messages and have these transferred automatically without the user of the
IPC
> > layer having to call IPC::Get/TakeFileHandleForProcess() or see
> > IPC::PlatformFileForTransits.
> > 
> >  * ipc_channel_win.cc: Add logic to tack duplicated handles onto a message,
> > similar to the code that uses msg->attachment_set()->PeekDescriptors() in
> > ipc_channel_posix.cc.  To start with, we'd just handle the case of sending
> > messages from the browser process to sandboxed processes.
> > 
> > 
> > This would make the error handling easier because all the DuplicateHandle()
> > calls would be done in one loop.  If one call fails, we can easily free the
> > remaining handles.
> > 
> > That would be easier to reason about than putting workarounds into users of
> the
> > IPC layer (such as nacl_process_host.cc), which is rather fragile because
the
> > error paths don't get tested.
> > 
> > What do you think about that idea?
> 
> IIUC, this has same problem; if DuplicateHandle() fails, after some
> DuplicateHandle() succeeds, in ipc_channel_win, it means some HANDLE is
already
> opened in the callee process.

FWIW, DuplicateHandle with DUPLICATE_CLOSE_SOURCE will work to close the handle
in the target process from the browser process. And I can use that to add a
batch version of BrokerDuplicateHandle that cleans up after itself on a failure.


> I'm happy to continue the discussion if you'd like, but it should be separated
> from this CL, IMO, because such a change needs to be applied to all IPC
layers.
> Probably, it's better to involve IPC experts and Win experts in the
discussion.
> 
> On the other hand, the goal of this CL is to complete the yusukes@'s remaining
> task for pre-FD-open on Win. I prefer making changes step-by-step.

[Mark Seaborn, 2015-05-27 13:17:26]

On 27 May 2015 at 05:28, <jschuh@chromium.org> wrote:

> On 2015/05/26 14:32:59, hidehiko wrote:
>
>> > However, I've been thinking about this, and I think that cleaning up
>> the IPC
>>
> > layer to transfer Windows handles more cleanly might not be too hard.
>> >
>> > Could we get the IPC layer to do the DuplicateHandle() calls *after* we
>> do
>> > channel->Send(message)?  This would involve changing:
>> >
>> >  * ipc_message_attachment.h: Unconditionalise TakePlatformFile().
>> >
>> >  * ipc/ipc_platform_file_attachment_posix.cc: Add a version for
>> Windows handles.
>> >
>> >  * ipc_message_attachment_set.h: Add Windows equivalent of
>> > AddDescriptorsToOwn().
>> >
>> >  * ipc_message_utils.cc: Add Windows equivalent of
>> > ParamTraits<base::FileDescriptor>::Write() -- perhaps
>> > ParamTraits<base::File>::Write().  We would allow putting base::Files
>> in IPC
>> > messages and have these transferred automatically without the user of
>> the IPC
>
>  > layer having to call IPC::Get/TakeFileHandleForProcess() or see
>> > IPC::PlatformFileForTransits.
>> >
>> >  * ipc_channel_win.cc: Add logic to tack duplicated handles onto a
>> message,
>> > similar to the code that uses msg->attachment_set()->PeekDescriptors()
>> in
>> > ipc_channel_posix.cc.  To start with, we'd just handle the case of
>> sending
>> > messages from the browser process to sandboxed processes.
>> >
>> >
>> > This would make the error handling easier because all the
>> DuplicateHandle()
>> > calls would be done in one loop.  If one call fails, we can easily free
>> the
>> > remaining handles.
>> >
>> > That would be easier to reason about than putting workarounds into
>> users of the
>> > IPC layer (such as nacl_process_host.cc), which is rather fragile
>> because the
>
>  > error paths don't get tested.
>> >
>> > What do you think about that idea?
>>
>
>  IIUC, this has same problem; if DuplicateHandle() fails, after some
>> DuplicateHandle() succeeds, in ipc_channel_win, it means some HANDLE is
>> already
>
>  opened in the callee process.
>>
>
> FWIW, DuplicateHandle with DUPLICATE_CLOSE_SOURCE will work to close the
> handle
> in the target process from the browser process. And I can use that to add a
> batch version of BrokerDuplicateHandle that cleans up after itself on a
> failure.


Indeed, as Justin says, it's possible to close the handles that got sent to
the renderer if DuplicateHandle() fails.

Also, I would argue that closing those duplicated handles on Windows isn't
actually the important part.  The important part is how errors are handled
on Unix.

Firstly, if some handles got leaked in the renderer process, that's not too
bad, because it's not too disruptive if a renderer process crashes due to
filling up the handle table.  It's more important to make sure that no
FDs/handles are leaked in the browser process, because it's more disruptive
if the browser process crashes.

Secondly, there aren't many ways the DuplicateHandle() call could fail:
 * It could fail if the destination process died, in which case we don't
need to close the duplicated handles.
 * It could fail if the destination process's handle table is full, in
which case the destination process probably won't last long anyway.
 * It could fail if the handle we're duplicating is invalid, which is
grounds for aborting the current process anyway.

Oddly enough, the benefit of cleaning up Chrome's use of handle-passing on
Windows would be that it stops Chrome leaking FDs in the browser process
*on Unix*.  That's because if there's an error after calling
IPC::Get/TakeFileHandleForProcess(), there's currently no portable
interface for closing the Unix FDs that those functions return.  The idea
of the clean up would be that we don't need to use
IPC::Get/TakeFileHandleForProcess().

Cheers,
Mark

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

[hidehiko, 2015-05-27 17:42:25]

On 2015/05/27 13:17:26, Mark Seaborn wrote:
> On 27 May 2015 at 05:28, <mailto:jschuh@chromium.org> wrote:
> 
> > On 2015/05/26 14:32:59, hidehiko wrote:
> >
> >> > However, I've been thinking about this, and I think that cleaning up
> >> the IPC
> >>
> > > layer to transfer Windows handles more cleanly might not be too hard.
> >> >
> >> > Could we get the IPC layer to do the DuplicateHandle() calls *after* we
> >> do
> >> > channel->Send(message)?  This would involve changing:
> >> >
> >> >  * ipc_message_attachment.h: Unconditionalise TakePlatformFile().
> >> >
> >> >  * ipc/ipc_platform_file_attachment_posix.cc: Add a version for
> >> Windows handles.
> >> >
> >> >  * ipc_message_attachment_set.h: Add Windows equivalent of
> >> > AddDescriptorsToOwn().
> >> >
> >> >  * ipc_message_utils.cc: Add Windows equivalent of
> >> > ParamTraits<base::FileDescriptor>::Write() -- perhaps
> >> > ParamTraits<base::File>::Write().  We would allow putting base::Files
> >> in IPC
> >> > messages and have these transferred automatically without the user of
> >> the IPC
> >
> >  > layer having to call IPC::Get/TakeFileHandleForProcess() or see
> >> > IPC::PlatformFileForTransits.
> >> >
> >> >  * ipc_channel_win.cc: Add logic to tack duplicated handles onto a
> >> message,
> >> > similar to the code that uses msg->attachment_set()->PeekDescriptors()
> >> in
> >> > ipc_channel_posix.cc.  To start with, we'd just handle the case of
> >> sending
> >> > messages from the browser process to sandboxed processes.
> >> >
> >> >
> >> > This would make the error handling easier because all the
> >> DuplicateHandle()
> >> > calls would be done in one loop.  If one call fails, we can easily free
> >> the
> >> > remaining handles.
> >> >
> >> > That would be easier to reason about than putting workarounds into
> >> users of the
> >> > IPC layer (such as nacl_process_host.cc), which is rather fragile
> >> because the
> >
> >  > error paths don't get tested.
> >> >
> >> > What do you think about that idea?
> >>
> >
> >  IIUC, this has same problem; if DuplicateHandle() fails, after some
> >> DuplicateHandle() succeeds, in ipc_channel_win, it means some HANDLE is
> >> already
> >
> >  opened in the callee process.
> >>
> >
> > FWIW, DuplicateHandle with DUPLICATE_CLOSE_SOURCE will work to close the
> > handle
> > in the target process from the browser process. And I can use that to add a
> > batch version of BrokerDuplicateHandle that cleans up after itself on a
> > failure.
> 
> 
> Indeed, as Justin says, it's possible to close the handles that got sent to
> the renderer if DuplicateHandle() fails.
> 
> Also, I would argue that closing those duplicated handles on Windows isn't
> actually the important part.  The important part is how errors are handled
> on Unix.
> 
> Firstly, if some handles got leaked in the renderer process, that's not too
> bad, because it's not too disruptive if a renderer process crashes due to
> filling up the handle table.  It's more important to make sure that no
> FDs/handles are leaked in the browser process, because it's more disruptive
> if the browser process crashes.
> 
> Secondly, there aren't many ways the DuplicateHandle() call could fail:
>  * It could fail if the destination process died, in which case we don't
> need to close the duplicated handles.
>  * It could fail if the destination process's handle table is full, in
> which case the destination process probably won't last long anyway.
>  * It could fail if the handle we're duplicating is invalid, which is
> grounds for aborting the current process anyway.
> 
> Oddly enough, the benefit of cleaning up Chrome's use of handle-passing on
> Windows would be that it stops Chrome leaking FDs in the browser process
> *on Unix*.  That's because if there's an error after calling
> IPC::Get/TakeFileHandleForProcess(), there's currently no portable
> interface for closing the Unix FDs that those functions return.  The idea
> of the clean up would be that we don't need to use
> IPC::Get/TakeFileHandleForProcess().
> 

Thank you for comment.

But, I wouldn't like to discuss the IPC's ideal FDs/HANDLEs handling *here*,
because it is not the goal.
This CL is based on the current IPC framework, and, again, the goal is
completing yusukes@'s
remaining task, rather than implementing ideal IPC's RAII.

Mark, what's your concrete concern on this CL? Even on Unix, the code should
release FDs properly, IIUC.
If FD RAII is supported on IPC in future, that's great. I'm happy to join the
discussion.
Though, it's better to be discussed separately. Actually, supporting RAII on IPC
is not the focus of this CL.

Thanks,
- hidehiko


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

[hidehiko, 2015-05-29 06:01:13]

ping.


On 2015/05/27 17:42:25, hidehiko wrote:
> On 2015/05/27 13:17:26, Mark Seaborn wrote:
> > On 27 May 2015 at 05:28, <mailto:jschuh@chromium.org> wrote:
> > 
> > > On 2015/05/26 14:32:59, hidehiko wrote:
> > >
> > >> > However, I've been thinking about this, and I think that cleaning up
> > >> the IPC
> > >>
> > > > layer to transfer Windows handles more cleanly might not be too hard.
> > >> >
> > >> > Could we get the IPC layer to do the DuplicateHandle() calls *after* we
> > >> do
> > >> > channel->Send(message)?  This would involve changing:
> > >> >
> > >> >  * ipc_message_attachment.h: Unconditionalise TakePlatformFile().
> > >> >
> > >> >  * ipc/ipc_platform_file_attachment_posix.cc: Add a version for
> > >> Windows handles.
> > >> >
> > >> >  * ipc_message_attachment_set.h: Add Windows equivalent of
> > >> > AddDescriptorsToOwn().
> > >> >
> > >> >  * ipc_message_utils.cc: Add Windows equivalent of
> > >> > ParamTraits<base::FileDescriptor>::Write() -- perhaps
> > >> > ParamTraits<base::File>::Write().  We would allow putting base::Files
> > >> in IPC
> > >> > messages and have these transferred automatically without the user of
> > >> the IPC
> > >
> > >  > layer having to call IPC::Get/TakeFileHandleForProcess() or see
> > >> > IPC::PlatformFileForTransits.
> > >> >
> > >> >  * ipc_channel_win.cc: Add logic to tack duplicated handles onto a
> > >> message,
> > >> > similar to the code that uses msg->attachment_set()->PeekDescriptors()
> > >> in
> > >> > ipc_channel_posix.cc.  To start with, we'd just handle the case of
> > >> sending
> > >> > messages from the browser process to sandboxed processes.
> > >> >
> > >> >
> > >> > This would make the error handling easier because all the
> > >> DuplicateHandle()
> > >> > calls would be done in one loop.  If one call fails, we can easily free
> > >> the
> > >> > remaining handles.
> > >> >
> > >> > That would be easier to reason about than putting workarounds into
> > >> users of the
> > >> > IPC layer (such as nacl_process_host.cc), which is rather fragile
> > >> because the
> > >
> > >  > error paths don't get tested.
> > >> >
> > >> > What do you think about that idea?
> > >>
> > >
> > >  IIUC, this has same problem; if DuplicateHandle() fails, after some
> > >> DuplicateHandle() succeeds, in ipc_channel_win, it means some HANDLE is
> > >> already
> > >
> > >  opened in the callee process.
> > >>
> > >
> > > FWIW, DuplicateHandle with DUPLICATE_CLOSE_SOURCE will work to close the
> > > handle
> > > in the target process from the browser process. And I can use that to add
a
> > > batch version of BrokerDuplicateHandle that cleans up after itself on a
> > > failure.
> > 
> > 
> > Indeed, as Justin says, it's possible to close the handles that got sent to
> > the renderer if DuplicateHandle() fails.
> > 
> > Also, I would argue that closing those duplicated handles on Windows isn't
> > actually the important part.  The important part is how errors are handled
> > on Unix.
> > 
> > Firstly, if some handles got leaked in the renderer process, that's not too
> > bad, because it's not too disruptive if a renderer process crashes due to
> > filling up the handle table.  It's more important to make sure that no
> > FDs/handles are leaked in the browser process, because it's more disruptive
> > if the browser process crashes.
> > 
> > Secondly, there aren't many ways the DuplicateHandle() call could fail:
> >  * It could fail if the destination process died, in which case we don't
> > need to close the duplicated handles.
> >  * It could fail if the destination process's handle table is full, in
> > which case the destination process probably won't last long anyway.
> >  * It could fail if the handle we're duplicating is invalid, which is
> > grounds for aborting the current process anyway.
> > 
> > Oddly enough, the benefit of cleaning up Chrome's use of handle-passing on
> > Windows would be that it stops Chrome leaking FDs in the browser process
> > *on Unix*.  That's because if there's an error after calling
> > IPC::Get/TakeFileHandleForProcess(), there's currently no portable
> > interface for closing the Unix FDs that those functions return.  The idea
> > of the clean up would be that we don't need to use
> > IPC::Get/TakeFileHandleForProcess().
> > 
> 
> Thank you for comment.
> 
> But, I wouldn't like to discuss the IPC's ideal FDs/HANDLEs handling *here*,
> because it is not the goal.
> This CL is based on the current IPC framework, and, again, the goal is
> completing yusukes@'s
> remaining task, rather than implementing ideal IPC's RAII.
> 
> Mark, what's your concrete concern on this CL? Even on Unix, the code should
> release FDs properly, IIUC.
> If FD RAII is supported on IPC in future, that's great. I'm happy to join the
> discussion.
> Though, it's better to be discussed separately. Actually, supporting RAII on
IPC
> is not the focus of this CL.
> 
> Thanks,
> - hidehiko
> 
> 
> > Cheers,
> > Mark
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[hidehiko, 2015-06-03 05:09:24]

ping.

On 2015/05/29 06:01:13, hidehiko wrote:
> ping.
> 
> 
> On 2015/05/27 17:42:25, hidehiko wrote:
> > On 2015/05/27 13:17:26, Mark Seaborn wrote:
> > > On 27 May 2015 at 05:28, <mailto:jschuh@chromium.org> wrote:
> > > 
> > > > On 2015/05/26 14:32:59, hidehiko wrote:
> > > >
> > > >> > However, I've been thinking about this, and I think that cleaning up
> > > >> the IPC
> > > >>
> > > > > layer to transfer Windows handles more cleanly might not be too hard.
> > > >> >
> > > >> > Could we get the IPC layer to do the DuplicateHandle() calls *after*
we
> > > >> do
> > > >> > channel->Send(message)?  This would involve changing:
> > > >> >
> > > >> >  * ipc_message_attachment.h: Unconditionalise TakePlatformFile().
> > > >> >
> > > >> >  * ipc/ipc_platform_file_attachment_posix.cc: Add a version for
> > > >> Windows handles.
> > > >> >
> > > >> >  * ipc_message_attachment_set.h: Add Windows equivalent of
> > > >> > AddDescriptorsToOwn().
> > > >> >
> > > >> >  * ipc_message_utils.cc: Add Windows equivalent of
> > > >> > ParamTraits<base::FileDescriptor>::Write() -- perhaps
> > > >> > ParamTraits<base::File>::Write().  We would allow putting base::Files
> > > >> in IPC
> > > >> > messages and have these transferred automatically without the user of
> > > >> the IPC
> > > >
> > > >  > layer having to call IPC::Get/TakeFileHandleForProcess() or see
> > > >> > IPC::PlatformFileForTransits.
> > > >> >
> > > >> >  * ipc_channel_win.cc: Add logic to tack duplicated handles onto a
> > > >> message,
> > > >> > similar to the code that uses
msg->attachment_set()->PeekDescriptors()
> > > >> in
> > > >> > ipc_channel_posix.cc.  To start with, we'd just handle the case of
> > > >> sending
> > > >> > messages from the browser process to sandboxed processes.
> > > >> >
> > > >> >
> > > >> > This would make the error handling easier because all the
> > > >> DuplicateHandle()
> > > >> > calls would be done in one loop.  If one call fails, we can easily
free
> > > >> the
> > > >> > remaining handles.
> > > >> >
> > > >> > That would be easier to reason about than putting workarounds into
> > > >> users of the
> > > >> > IPC layer (such as nacl_process_host.cc), which is rather fragile
> > > >> because the
> > > >
> > > >  > error paths don't get tested.
> > > >> >
> > > >> > What do you think about that idea?
> > > >>
> > > >
> > > >  IIUC, this has same problem; if DuplicateHandle() fails, after some
> > > >> DuplicateHandle() succeeds, in ipc_channel_win, it means some HANDLE is
> > > >> already
> > > >
> > > >  opened in the callee process.
> > > >>
> > > >
> > > > FWIW, DuplicateHandle with DUPLICATE_CLOSE_SOURCE will work to close the
> > > > handle
> > > > in the target process from the browser process. And I can use that to
add
> a
> > > > batch version of BrokerDuplicateHandle that cleans up after itself on a
> > > > failure.
> > > 
> > > 
> > > Indeed, as Justin says, it's possible to close the handles that got sent
to
> > > the renderer if DuplicateHandle() fails.
> > > 
> > > Also, I would argue that closing those duplicated handles on Windows isn't
> > > actually the important part.  The important part is how errors are handled
> > > on Unix.
> > > 
> > > Firstly, if some handles got leaked in the renderer process, that's not
too
> > > bad, because it's not too disruptive if a renderer process crashes due to
> > > filling up the handle table.  It's more important to make sure that no
> > > FDs/handles are leaked in the browser process, because it's more
disruptive
> > > if the browser process crashes.
> > > 
> > > Secondly, there aren't many ways the DuplicateHandle() call could fail:
> > >  * It could fail if the destination process died, in which case we don't
> > > need to close the duplicated handles.
> > >  * It could fail if the destination process's handle table is full, in
> > > which case the destination process probably won't last long anyway.
> > >  * It could fail if the handle we're duplicating is invalid, which is
> > > grounds for aborting the current process anyway.
> > > 
> > > Oddly enough, the benefit of cleaning up Chrome's use of handle-passing on
> > > Windows would be that it stops Chrome leaking FDs in the browser process
> > > *on Unix*.  That's because if there's an error after calling
> > > IPC::Get/TakeFileHandleForProcess(), there's currently no portable
> > > interface for closing the Unix FDs that those functions return.  The idea
> > > of the clean up would be that we don't need to use
> > > IPC::Get/TakeFileHandleForProcess().
> > > 
> > 
> > Thank you for comment.
> > 
> > But, I wouldn't like to discuss the IPC's ideal FDs/HANDLEs handling *here*,
> > because it is not the goal.
> > This CL is based on the current IPC framework, and, again, the goal is
> > completing yusukes@'s
> > remaining task, rather than implementing ideal IPC's RAII.
> > 
> > Mark, what's your concrete concern on this CL? Even on Unix, the code should
> > release FDs properly, IIUC.
> > If FD RAII is supported on IPC in future, that's great. I'm happy to join
the
> > discussion.
> > Though, it's better to be discussed separately. Actually, supporting RAII on
> IPC
> > is not the focus of this CL.
> > 
> > Thanks,
> > - hidehiko
> > 
> > 
> > > Cheers,
> > > Mark
> > > 
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.

[hidehiko, 2015-06-04 14:47:09]

ping.

On 2015/06/03 05:09:24, hidehiko wrote:
> ping.
> 
> On 2015/05/29 06:01:13, hidehiko wrote:
> > ping.
> > 
> > 
> > On 2015/05/27 17:42:25, hidehiko wrote:
> > > On 2015/05/27 13:17:26, Mark Seaborn wrote:
> > > > On 27 May 2015 at 05:28, <mailto:jschuh@chromium.org> wrote:
> > > > 
> > > > > On 2015/05/26 14:32:59, hidehiko wrote:
> > > > >
> > > > >> > However, I've been thinking about this, and I think that cleaning
up
> > > > >> the IPC
> > > > >>
> > > > > > layer to transfer Windows handles more cleanly might not be too
hard.
> > > > >> >
> > > > >> > Could we get the IPC layer to do the DuplicateHandle() calls
*after*
> we
> > > > >> do
> > > > >> > channel->Send(message)?  This would involve changing:
> > > > >> >
> > > > >> >  * ipc_message_attachment.h: Unconditionalise TakePlatformFile().
> > > > >> >
> > > > >> >  * ipc/ipc_platform_file_attachment_posix.cc: Add a version for
> > > > >> Windows handles.
> > > > >> >
> > > > >> >  * ipc_message_attachment_set.h: Add Windows equivalent of
> > > > >> > AddDescriptorsToOwn().
> > > > >> >
> > > > >> >  * ipc_message_utils.cc: Add Windows equivalent of
> > > > >> > ParamTraits<base::FileDescriptor>::Write() -- perhaps
> > > > >> > ParamTraits<base::File>::Write().  We would allow putting
base::Files
> > > > >> in IPC
> > > > >> > messages and have these transferred automatically without the user
of
> > > > >> the IPC
> > > > >
> > > > >  > layer having to call IPC::Get/TakeFileHandleForProcess() or see
> > > > >> > IPC::PlatformFileForTransits.
> > > > >> >
> > > > >> >  * ipc_channel_win.cc: Add logic to tack duplicated handles onto a
> > > > >> message,
> > > > >> > similar to the code that uses
> msg->attachment_set()->PeekDescriptors()
> > > > >> in
> > > > >> > ipc_channel_posix.cc.  To start with, we'd just handle the case of
> > > > >> sending
> > > > >> > messages from the browser process to sandboxed processes.
> > > > >> >
> > > > >> >
> > > > >> > This would make the error handling easier because all the
> > > > >> DuplicateHandle()
> > > > >> > calls would be done in one loop.  If one call fails, we can easily
> free
> > > > >> the
> > > > >> > remaining handles.
> > > > >> >
> > > > >> > That would be easier to reason about than putting workarounds into
> > > > >> users of the
> > > > >> > IPC layer (such as nacl_process_host.cc), which is rather fragile
> > > > >> because the
> > > > >
> > > > >  > error paths don't get tested.
> > > > >> >
> > > > >> > What do you think about that idea?
> > > > >>
> > > > >
> > > > >  IIUC, this has same problem; if DuplicateHandle() fails, after some
> > > > >> DuplicateHandle() succeeds, in ipc_channel_win, it means some HANDLE
is
> > > > >> already
> > > > >
> > > > >  opened in the callee process.
> > > > >>
> > > > >
> > > > > FWIW, DuplicateHandle with DUPLICATE_CLOSE_SOURCE will work to close
the
> > > > > handle
> > > > > in the target process from the browser process. And I can use that to
> add
> > a
> > > > > batch version of BrokerDuplicateHandle that cleans up after itself on
a
> > > > > failure.
> > > > 
> > > > 
> > > > Indeed, as Justin says, it's possible to close the handles that got sent
> to
> > > > the renderer if DuplicateHandle() fails.
> > > > 
> > > > Also, I would argue that closing those duplicated handles on Windows
isn't
> > > > actually the important part.  The important part is how errors are
handled
> > > > on Unix.
> > > > 
> > > > Firstly, if some handles got leaked in the renderer process, that's not
> too
> > > > bad, because it's not too disruptive if a renderer process crashes due
to
> > > > filling up the handle table.  It's more important to make sure that no
> > > > FDs/handles are leaked in the browser process, because it's more
> disruptive
> > > > if the browser process crashes.
> > > > 
> > > > Secondly, there aren't many ways the DuplicateHandle() call could fail:
> > > >  * It could fail if the destination process died, in which case we don't
> > > > need to close the duplicated handles.
> > > >  * It could fail if the destination process's handle table is full, in
> > > > which case the destination process probably won't last long anyway.
> > > >  * It could fail if the handle we're duplicating is invalid, which is
> > > > grounds for aborting the current process anyway.
> > > > 
> > > > Oddly enough, the benefit of cleaning up Chrome's use of handle-passing
on
> > > > Windows would be that it stops Chrome leaking FDs in the browser process
> > > > *on Unix*.  That's because if there's an error after calling
> > > > IPC::Get/TakeFileHandleForProcess(), there's currently no portable
> > > > interface for closing the Unix FDs that those functions return.  The
idea
> > > > of the clean up would be that we don't need to use
> > > > IPC::Get/TakeFileHandleForProcess().
> > > > 
> > > 
> > > Thank you for comment.
> > > 
> > > But, I wouldn't like to discuss the IPC's ideal FDs/HANDLEs handling
*here*,
> > > because it is not the goal.
> > > This CL is based on the current IPC framework, and, again, the goal is
> > > completing yusukes@'s
> > > remaining task, rather than implementing ideal IPC's RAII.
> > > 
> > > Mark, what's your concrete concern on this CL? Even on Unix, the code
should
> > > release FDs properly, IIUC.
> > > If FD RAII is supported on IPC in future, that's great. I'm happy to join
> the
> > > discussion.
> > > Though, it's better to be discussed separately. Actually, supporting RAII
on
> > IPC
> > > is not the focus of this CL.
> > > 
> > > Thanks,
> > > - hidehiko
> > > 
> > > 
> > > > Cheers,
> > > > Mark
> > > > 
> > > > To unsubscribe from this group and stop receiving emails from it, send
an
> > > email
> > > > to mailto:chromium-reviews+unsubscribe@chromium.org.