The Windows portion of Native Messagaing

The Windows portion of Native Messagaing

Native Messaging is a new api that allows extensions to communicate with registered hosts on the native machine.

See these two documents for more information:
https://docs.google.com/a/chromium.org/document/d/1sjmIO2rT6ZptB4O5WHu5C0oNCCIdPmCk7lnDGFi5Pm4/edit
https://docs.google.com/a/chromium.org/document/d/1tDmeyo6hbAmuU5jYAAmKFt6YGBTVBc-5MHqaIOPa04E/edit

BUG=142915

[Aaron Boodman, 2012-10-24 00:28:01]

Matt, can you take this? (-self)

[Matt Perry, 2012-10-24 23:40:31]

Is the CL description still accurate?

http://codereview.chromium.org/10918255/diff/18001/base/process_util_win.cc
File base/process_util_win.cc (right):

http://codereview.chromium.org/10918255/diff/18001/base/process_util_win.cc#n...
base/process_util_win.cc:351: ::SetLastError(0);
Is this needed?

http://codereview.chromium.org/10918255/diff/18001/chrome/browser/extensions/...
File chrome/browser/extensions/api/messaging/native_message_process_host.h
(right):

http://codereview.chromium.org/10918255/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_message_process_host.h:116:
READ_STATE_WAITING_FOR_MESSGAE
typo

http://codereview.chromium.org/10918255/diff/18001/chrome/browser/extensions/...
File
chrome/browser/extensions/api/messaging/native_message_process_host_unittest.cc
(right):

http://codereview.chromium.org/10918255/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_message_process_host_unittest.cc:127:
for (int nap_times = 0; nap_times < kMaxNapTimes && !got_message_;
Please don't use polling in tests. It results in flakiness.

Can you instead do message_loop_.Run(); and have event listeners that call
message_loop_.Quit() when error/success happens?

http://codereview.chromium.org/10918255/diff/18001/chrome/browser/extensions/...
File chrome/browser/extensions/api/messaging/native_message_process_host_win.cc
(right):

http://codereview.chromium.org/10918255/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_message_process_host_win.cc:54: {
nit: common chrome style is to put the { on the same line as the case.

http://codereview.chromium.org/10918255/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_message_process_host_win.cc:74:
if (bytes_transfered != pending_message_.size()) {
Are you guaranteed that the entire contents is read in one go?

http://codereview.chromium.org/10918255/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_message_process_host_win.cc:126:
MessageLoopForIO::current()->WaitForIOCompletion(INFINITE, this);
We generally don't want to block any thread. Is this call necessary?

http://codereview.chromium.org/10918255/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_message_process_host_win.cc:135:
// with a non-zero status. We have to circumvent ScopedHandle::Close().
I don't follow this comment.

http://codereview.chromium.org/10918255/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/api/messaging/native_message_process_host_win.cc:154:
// Not used by win.
Then why not #ifdef it out in the header? ReadMessage seems unused on win as
well.

[eaugusti, 2012-10-30 22:03:12]

The CL description is now correct :)

Brettw, I see that you some of the most resent edits in some ipc_channel_win
that makes similar windows calls.
Could you review the windows portion?

https://codereview.chromium.org/10918255/diff/18001/base/process_util_win.cc
File base/process_util_win.cc (right):

https://codereview.chromium.org/10918255/diff/18001/base/process_util_win.cc#...
base/process_util_win.cc:351: ::SetLastError(0);
On 2012/10/24 23:40:31, Matt Perry wrote:
> Is this needed?

Stray edit.

https://codereview.chromium.org/10918255/diff/18001/chrome/browser/extensions...
File chrome/browser/extensions/api/messaging/native_message_process_host.h
(right):

https://codereview.chromium.org/10918255/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/native_message_process_host.h:116:
READ_STATE_WAITING_FOR_MESSGAE
On 2012/10/24 23:40:31, Matt Perry wrote:
> typo

Done.

https://codereview.chromium.org/10918255/diff/18001/chrome/browser/extensions...
File
chrome/browser/extensions/api/messaging/native_message_process_host_unittest.cc
(right):

https://codereview.chromium.org/10918255/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/native_message_process_host_unittest.cc:127:
for (int nap_times = 0; nap_times < kMaxNapTimes && !got_message_;
On 2012/10/24 23:40:31, Matt Perry wrote:
> Please don't use polling in tests. It results in flakiness.
> 
> Can you instead do message_loop_.Run(); and have event listeners that call
> message_loop_.Quit() when error/success happens?

Done.

https://codereview.chromium.org/10918255/diff/18001/chrome/browser/extensions...
File chrome/browser/extensions/api/messaging/native_message_process_host_win.cc
(right):

https://codereview.chromium.org/10918255/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/native_message_process_host_win.cc:54: {
On 2012/10/24 23:40:31, Matt Perry wrote:
> nit: common chrome style is to put the { on the same line as the case.

Done.

https://codereview.chromium.org/10918255/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/native_message_process_host_win.cc:74:
if (bytes_transfered != pending_message_.size()) {
On 2012/10/24 23:40:31, Matt Perry wrote:
> Are you guaranteed that the entire contents is read in one go?

According to the documentation, it should be. But, I am going to bring in a
windows expert.

https://codereview.chromium.org/10918255/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/native_message_process_host_win.cc:126:
MessageLoopForIO::current()->WaitForIOCompletion(INFINITE, this);
On 2012/10/24 23:40:31, Matt Perry wrote:
> We generally don't want to block any thread. Is this call necessary?

Yeah, I think it is. ipc_channel_win does it and it is suggested in
message_loop_win.

https://codereview.chromium.org/10918255/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/native_message_process_host_win.cc:135:
// with a non-zero status. We have to circumvent ScopedHandle::Close().
On 2012/10/24 23:40:31, Matt Perry wrote:
> I don't follow this comment.

Done.

https://codereview.chromium.org/10918255/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/native_message_process_host_win.cc:154:
// Not used by win.
On 2012/10/24 23:40:31, Matt Perry wrote:
> Then why not #ifdef it out in the header? ReadMessage seems unused on win as
> well.

Done.

[Matt Perry, 2012-10-30 22:59:52]

lgtm

[rvargas (doing something else), 2012-11-29 01:19:23]

I just skimmed through some of the files and there are some serious blockers.

The CL description is completely missing. There's only a title and a bug number
that doesn't provide any input into what this CL does.

https://codereview.chromium.org/10918255/diff/43001/base/process_util.h
File base/process_util.h (right):

https://codereview.chromium.org/10918255/diff/43001/base/process_util.h#newco...
base/process_util.h:240: stdin_handle(kNullProcessHandle),
This is assigning a "process handle" to a file handle. The invalid handle value
for those types of handles are different.

https://codereview.chromium.org/10918255/diff/43001/base/process_util_win.cc
File base/process_util_win.cc (right):

https://codereview.chromium.org/10918255/diff/43001/base/process_util_win.cc#...
base/process_util_win.cc:310: startup_info.dwFlags |= STARTF_USESTDHANDLES;
Unused handles should be set to INVALID_HANDLE_VALUE when this flag is set.

https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/api/messaging/message_service.cc (right):

https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/message_service.cc:219:
content::BrowserThread::IO,
Why the IO thread?

https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/api/messaging/native_process_launcher_win.cc
(right):

https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/native_process_launcher_win.cc:24: bool
NativeProcessLauncher::LaunchNativeProcess(
We cannot do this on the IO thread. There's a bunch of code here that blocks.

https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/native_process_launcher_win.cc:58:
NOTREACHED() << "Failed to create pipe";
we should not really dcheck on the return value of a system call. It will fail
at some point.

https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/native_process_launcher_win.cc:99:
options.inherit_handles = true;
am I reading this correctly? it looks like the new process will inherit the
browser handles. That's a no-no.

[Sergey Ulanov, 2012-12-01 01:22:59]

Thanks, Recardo. I've addressed some of your comments in crrev.com/11419267 .

https://codereview.chromium.org/10918255/diff/43001/base/process_util.h
File base/process_util.h (right):

https://codereview.chromium.org/10918255/diff/43001/base/process_util.h#newco...
base/process_util.h:240: stdin_handle(kNullProcessHandle),
On 2012/11/29 01:19:23, rvargas wrote:
> This is assigning a "process handle" to a file handle. The invalid handle
value
> for those types of handles are different.

Addressed this in crrev.com/11419267

https://codereview.chromium.org/10918255/diff/43001/base/process_util_win.cc
File base/process_util_win.cc (right):

https://codereview.chromium.org/10918255/diff/43001/base/process_util_win.cc#...
base/process_util_win.cc:310: startup_info.dwFlags |= STARTF_USESTDHANDLES;
On 2012/11/29 01:19:23, rvargas wrote:
> Unused handles should be set to INVALID_HANDLE_VALUE when this flag is set.

Done in crrev.com/11419267

https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/api/messaging/native_process_launcher_win.cc
(right):

https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/native_process_launcher_win.cc:58:
NOTREACHED() << "Failed to create pipe";
On 2012/11/29 01:19:23, rvargas wrote:
> we should not really dcheck on the return value of a system call. It will fail
> at some point.

I think NOTREACHED() is the right thing to do here. It fails in debug builds
allowing to detect if there is an issue, yet handles the problem properly in
Release builds. base code does it in some places as well.

https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/native_process_launcher_win.cc:99:
options.inherit_handles = true;
On 2012/11/29 01:19:23, rvargas wrote:
> am I reading this correctly? it looks like the new process will inherit the
> browser handles. That's a no-no.

I'm not an expect, but according to MSDN (
http://msdn.microsoft.com/en-us/library/windows/desktop/ms724466(v=vs.85).aspx )
this flag doesn't really mean that the child process inherits all handles. Just
the ones that we explicitly allow to be inherited.

[rvargas (doing something else), 2012-12-01 01:57:24]

https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/api/messaging/native_process_launcher_win.cc
(right):

https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/native_process_launcher_win.cc:58:
NOTREACHED() << "Failed to create pipe";
On 2012/12/01 01:23:00, sergeyu wrote:
> On 2012/11/29 01:19:23, rvargas wrote:
> > we should not really dcheck on the return value of a system call. It will
fail
> > at some point.
> 
> I think NOTREACHED() is the right thing to do here. It fails in debug builds
> allowing to detect if there is an issue, yet handles the problem properly in
> Release builds. base code does it in some places as well.

This pattern makes the developer who writes the code confident that it doesn't
fail because it works fine on a local machine, release builds never report an
issue from the field, and random developers once in a while get DCHECKs without
any actual action item because it really requires some planetary alignment to
cause the OS to fail the operation.

NOTREACHED should check preconditions of the code itself (under direct control
of the programmer). System calls are not that.

If you are seriously worried about some related change causing this code to
fail, a DLOG somewhere sounds like a better solution (although DLOGs will bloat
the code more than NOTREACHED).

https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/native_process_launcher_win.cc:99:
options.inherit_handles = true;
On 2012/12/01 01:23:00, sergeyu wrote:
> On 2012/11/29 01:19:23, rvargas wrote:
> > am I reading this correctly? it looks like the new process will inherit the
> > browser handles. That's a no-no.
> 
> I'm not an expect, but according to MSDN (
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms724466%28v=vs.85%29...
)
> this flag doesn't really mean that the child process inherits all handles.
Just
> the ones that we explicitly allow to be inherited.

Correct, only handles marked for inheritance are inherited, but we still don't
use potentially dangerous patterns with the browser.

[Sergey Ulanov, 2012-12-04 23:50:43]

https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/api/messaging/native_process_launcher_win.cc
(right):

https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/native_process_launcher_win.cc:99:
options.inherit_handles = true;
On 2012/12/01 01:57:24, rvargas wrote:
> On 2012/12/01 01:23:00, sergeyu wrote:
> > On 2012/11/29 01:19:23, rvargas wrote:
> > > am I reading this correctly? it looks like the new process will inherit
the
> > > browser handles. That's a no-no.
> > 
> > I'm not an expect, but according to MSDN (
> >
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms724466%2528v=vs.85%...
> )
> > this flag doesn't really mean that the child process inherits all handles.
> Just
> > the ones that we explicitly allow to be inherited.
> 
> Correct, only handles marked for inheritance are inherited, but we still don't
> use potentially dangerous patterns with the browser.

Do you have any suggestions on how we can launch an external process with
redirected stdin and stdout without using this flag?

In chromium code bInheritHandle is set to true for file descriptors only in a
few places and in most cases it's specifically to allow stdio redirection, so it
should be safe to use this flag here, but I agree that it's better to be careful
about it.

[rvargas (doing something else), 2012-12-05 20:59:05]

https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/api/messaging/native_process_launcher_win.cc
(right):

https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/native_process_launcher_win.cc:99:
options.inherit_handles = true;
On 2012/12/04 23:50:43, sergeyu wrote:
> On 2012/12/01 01:57:24, rvargas wrote:
> > On 2012/12/01 01:23:00, sergeyu wrote:
> > > On 2012/11/29 01:19:23, rvargas wrote:
> > > > am I reading this correctly? it looks like the new process will inherit
> the
> > > > browser handles. That's a no-no.
> > > 
> > > I'm not an expect, but according to MSDN (
> > >
> >
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms724466%252528v=vs.8...
> > )
> > > this flag doesn't really mean that the child process inherits all handles.
> > Just
> > > the ones that we explicitly allow to be inherited.
> > 
> > Correct, only handles marked for inheritance are inherited, but we still
don't
> > use potentially dangerous patterns with the browser.
> 
> Do you have any suggestions on how we can launch an external process with
> redirected stdin and stdout without using this flag?

Yeah, that's a pain in the back. I only see using an intermediate process to
launch the target app: only inherit handles from that intermediate process,
which by definition has very little code to go over. The additional overhead
would be fine considering the use case. But it requires setting up a regular IPC
to pass the handles to the intermediate process :(

While it is true that the native process probably has access to do anything it
wants, so it may be able to extract whatever it wishes from the browser process,
I still think it is better to make sure we are not giving direct access to
something that we don't want to, and inspecting the code now looking for
inheritable handles doesn't mean we'll be fine in six months.

Now I'm not so sure the extra code is worth it.

> 
> In chromium code bInheritHandle is set to true for file descriptors only in a
> few places and in most cases it's specifically to allow stdio redirection, so
it
> should be safe to use this flag here, but I agree that it's better to be
careful
> about it.

[rvargas (doing something else), 2012-12-06 19:15:06]

https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/api/messaging/native_process_launcher_win.cc
(right):

https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/native_process_launcher_win.cc:99:
options.inherit_handles = true;
> Yeah, that's a pain in the back. I only see using an intermediate process to
> launch the target app: only inherit handles from that intermediate process,
> which by definition has very little code to go over. The additional overhead
> would be fine considering the use case. But it requires setting up a regular
IPC
> to pass the handles to the intermediate process :(
> 
> While it is true that the native process probably has access to do anything it
> wants, so it may be able to extract whatever it wishes from the browser
process,
> I still think it is better to make sure we are not giving direct access to
> something that we don't want to, and inspecting the code now looking for
> inheritable handles doesn't mean we'll be fine in six months.
> 
> Now I'm not so sure the extra code is worth it.

Note that a better option is to use another IPC mechanism instead of redirection
of standard IO. For example, passing a named pipe in the command line is trivial
and very robust (both from the point of view of our code and the native app),
and AFAIK it is the mechanism selected for Pepper (uniformity being another
plus).

[eaugusti, 2012-12-06 23:22:45]

On 2012/12/06 19:15:06, rvargas wrote:
>
https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
> File chrome/browser/extensions/api/messaging/native_process_launcher_win.cc
> (right):
> 
>
https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
> chrome/browser/extensions/api/messaging/native_process_launcher_win.cc:99:
> options.inherit_handles = true;
> > Yeah, that's a pain in the back. I only see using an intermediate process to
> > launch the target app: only inherit handles from that intermediate process,
> > which by definition has very little code to go over. The additional overhead
> > would be fine considering the use case. But it requires setting up a regular
> IPC
> > to pass the handles to the intermediate process :(
> > 
> > While it is true that the native process probably has access to do anything
it
> > wants, so it may be able to extract whatever it wishes from the browser
> process,
> > I still think it is better to make sure we are not giving direct access to
> > something that we don't want to, and inspecting the code now looking for
> > inheritable handles doesn't mean we'll be fine in six months.
> > 
> > Now I'm not so sure the extra code is worth it.
> 
> Note that a better option is to use another IPC mechanism instead of
redirection
> of standard IO. For example, passing a named pipe in the command line is
trivial
> and very robust (both from the point of view of our code and the native app),
> and AFAIK it is the mechanism selected for Pepper (uniformity being another
> plus).

I'm a little hesitant about passing the named pipe instead of redirection just
because now we would be enforcing two different conventions, one for POSIX and
one for Windows.

[eaugusti, 2012-12-07 02:21:07]

I merged in sergeyu process_util cl.

https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/api/messaging/message_service.cc (right):

https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/message_service.cc:219:
content::BrowserThread::IO,
Because Windows' FILE thread has a MessageLoopForIO that has a type of TYPE_UI
(because of some restrictions with Windows notifications) and we need to do
async IO that requires a MessageLoopForIO with TYPE_IO. See
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/browser_ma...

On 2012/11/29 01:19:23, rvargas wrote:
> Why the IO thread?

https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/api/messaging/native_process_launcher_win.cc
(right):

https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/native_process_launcher_win.cc:24: bool
NativeProcessLauncher::LaunchNativeProcess(
On 2012/11/29 01:19:23, rvargas wrote:
> We cannot do this on the IO thread. There's a bunch of code here that blocks.

Ok, the NativeProcessHost itself lives on the IO thread (see comment in
message_service.cc) but we already have to move to the FILE thread for a path
check. So we can launch there.

https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/native_process_launcher_win.cc:58:
NOTREACHED() << "Failed to create pipe";
On 2012/12/01 01:57:24, rvargas wrote:
> On 2012/12/01 01:23:00, sergeyu wrote:
> > On 2012/11/29 01:19:23, rvargas wrote:
> > > we should not really dcheck on the return value of a system call. It will
> fail
> > > at some point.
> > 
> > I think NOTREACHED() is the right thing to do here. It fails in debug builds
> > allowing to detect if there is an issue, yet handles the problem properly in
> > Release builds. base code does it in some places as well.
> 
> This pattern makes the developer who writes the code confident that it doesn't
> fail because it works fine on a local machine, release builds never report an
> issue from the field, and random developers once in a while get DCHECKs
without
> any actual action item because it really requires some planetary alignment to
> cause the OS to fail the operation.
> 
> NOTREACHED should check preconditions of the code itself (under direct control
> of the programmer). System calls are not that.
> 
> If you are seriously worried about some related change causing this code to
> fail, a DLOG somewhere sounds like a better solution (although DLOGs will
bloat
> the code more than NOTREACHED).

What about using a DPLOG?
It makes sense not to use NOTREACHED() because it is used to catch programmer
errors, and not system errors. My main concern is that something happens on some
trybot and someone is left without any indication of what failed in the this
sequence of system calls.

Is the bloat an issue here? (Forgive my ignorance, I though D*LOG was pretty
cheap).

[jschuh, 2012-12-08 00:59:00]

On 2012/12/06 23:22:45, eaugusti wrote:
> On 2012/12/06 19:15:06, rvargas wrote:
> >
>
https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
> > File chrome/browser/extensions/api/messaging/native_process_launcher_win.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
> > chrome/browser/extensions/api/messaging/native_process_launcher_win.cc:99:
> > options.inherit_handles = true;
> > > Yeah, that's a pain in the back. I only see using an intermediate process
to
> > > launch the target app: only inherit handles from that intermediate
process,
> > > which by definition has very little code to go over. The additional
overhead
> > > would be fine considering the use case. But it requires setting up a
regular
> > IPC
> > > to pass the handles to the intermediate process :(
> > > 
> > > While it is true that the native process probably has access to do
anything
> it
> > > wants, so it may be able to extract whatever it wishes from the browser
> > process,
> > > I still think it is better to make sure we are not giving direct access to
> > > something that we don't want to, and inspecting the code now looking for
> > > inheritable handles doesn't mean we'll be fine in six months.
> > > 
> > > Now I'm not so sure the extra code is worth it.
> > 
> > Note that a better option is to use another IPC mechanism instead of
> redirection
> > of standard IO. For example, passing a named pipe in the command line is
> trivial
> > and very robust (both from the point of view of our code and the native
app),
> > and AFAIK it is the mechanism selected for Pepper (uniformity being another
> > plus).
> 
> I'm a little hesitant about passing the named pipe instead of redirection just
> because now we would be enforcing two different conventions, one for POSIX and
> one for Windows.

Unfortunately, inherited stdio handles are not an option from the browser
process on Windows XP. On Vista+ you can safely pass just the handles you want
in STARTUPINFOEX, but on Windows XP you can pick up a mess of junk you don't
want (e.g. any open socket handles). Those handles you accidentally inherit then
hang around for the life of the child process, and can prevent you from
accessing and/or removing the resources since the object entry is still alive.
If you want to use stdio handle inheritance on XP your best option is probably
Rick's suggestion of an intermediate process with a clean handle space.

The other caveat you need to be aware of is that the C/CPP standard runtime is
going to perform buffering on any C/C++ IO. generally it will flush on newlines,
so if you build them into your wire protocol you should be safe.

[rvargas (doing something else), 2012-12-12 00:20:22]

https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
File chrome/browser/extensions/api/messaging/native_process_launcher_win.cc
(right):

https://codereview.chromium.org/10918255/diff/43001/chrome/browser/extensions...
chrome/browser/extensions/api/messaging/native_process_launcher_win.cc:58:
NOTREACHED() << "Failed to create pipe";
> What about using a DPLOG?
> It makes sense not to use NOTREACHED() because it is used to catch programmer
> errors, and not system errors. My main concern is that something happens on
some
> trybot and someone is left without any indication of what failed in the this
> sequence of system calls.
> 
> Is the bloat an issue here? (Forgive my ignorance, I though D*LOG was pretty
> cheap).

You may be surprised by the amount of code inserted by a log... definitely looks
more innocuous than what it is.

D(P)LOGs are ok, but I would log once from the caller as opposed to adding 5 log
points from this code.