875214 - Child processes should connect to parent's console

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

Attachment: Attach to the parent's console

It would be useful for debugging on Windows that child processes are attached to their parent's console.  This means that if the parent has a console and the child writes to stdout/stderr, the output appears in the console.

Attaching a patch that does this, taken from the "-attach-console" support in nsNativeAppSupportWin.cpp.  This patch also relies on bug 875210 to work effectively.

Same CCs as bug 875210, and same request for either a review or a suggestion for a reviewer.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 753122 [details] [diff] [review]
Attach to the parent's console

Review of attachment 753122 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I just changed my mind on this.

I'm not convinced that this patch is necessary (same for bug 875210).

http://msdn.microsoft.com/en-us/library/windows/desktop/ms683463%28v=vs.85%29.aspx specifies that the console and standard handles are implicitly inherited by child processes, unless the child was started as a detached process. I have verified this myself using a scratch program that I threw together.

If we are having problems with the inheritance of standard handles, I'd like to have a better idea why this is happening since by default it is supposed to work.

I am also concerned because this patch enables handle inheritance in CreateProcess, which could open the floodgates for inadvertently sharing any other kernel handles whose inheritable flag has been turned on.

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

More info about standard handle inheritance here:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms682075%28v=vs.85%29.aspx

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

Hmmm, when the parent process's binary is set to run in the WINDOWS subsystem but the child process's binary is set to run in the CONSOLE subsystem, Windows creates a new console for the child (and hence it receives a new set of standard handles), even if the parent already had a console.

Lots more info about console allocation and inheritance here:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms682528%28v=vs.85%29.aspx

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

Okay, having read a bunch of additional info about Windows' behavior, I am amenable to reconsidering my review once we know why the console isn't being inherited in our case.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

(In reply to Aaron Klotz [:aklotz] from comment #4)
> Okay, having read a bunch of additional info about Windows' behavior, I am
> amenable to reconsidering my review once we know why the console isn't being
> inherited in our case.

So - TIL that plugin-container is built as a "console" program in debug builds and a gui program otherwise :)  This means that things do indeed work as expected in debug builds - just not release.  This probably explains Benjamin's comment that he has seen that output in the past.

The fact things don't work as expected in release builds is fairly simple to explain: we have a parent program with standard handles referencing a console, and it is starting a "GUI" child process - so the standard handles can't be automatically attached to that console.

The fact the standard handles aren't inherited when both processes are "GUI" applications can, I think, be explained by the link you pointed at above (http://msdn.microsoft.com/en-us/library/windows/desktop/ms683463%28v=vs.85%29.aspx) - which says:

| Additionally, to inherit the standard input, standard output, and 
| standard error handles, the dwFlags member of the STARTUPINFO structure 
| must include STARTF_USESTDHANDLES.

And the docs for STARTUPINFO explicitly say that when this flag is used bInheritHandles must be TRUE - which is the patch I uploaded to bug 875210.  IOW, it seems that CreateProcess makes no promises about automatically inheriting standard handles in any situation - it's just that the CRT startup will attach the standard handles to a console if one is available - a console which may be shared with parents.  However, one thing I can not understand is that debug builds when run with output redirected *does* see child process output whereas release builds still do not - ie, a debug child process automatically inherits the std handles but release builds do not, even when those handles are to a file rather than the console.  I have confirmed that even in this case the debug builds are still attached to the console, so I guess this has something to do with it.

Another option we could consider here is to always build plugin-container.exe as a console program - even in release builds - then have process_util_win.cc specify DETACHED_PROCESS when it detects it has no console, with the end result being that in release builds the console will typically be hidden.  This would prevent the need for the child to explicitly AttachConsole - but it seems like 6 of one, 1/2 a dozen of the other to me.  A quick experiment shows this does indeed work, but my preference is still leaning slightly towards the patch here rather than doing a DETACHED_PROCESS dance...

So the upshot of all this seems to be that for release builds we *do* need the patch here and in bug 875210 to see output from the child process.  We probably do not need to do all this in debug builds, but OTOH, this work doesn't actually hurt in debug builds either, and it avoids the code making assumptions about how the executables are linked.

Please let me know if any of the above doesn't make sense or needs further clarification or investigation...

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 753122 [details] [diff] [review]
Attach to the parent's console

Review of attachment 753122 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Mark Hammond (:markh) from comment #5)
Yeah, I think that the console inheritance rules are much more complicated than the docs would suggest. Having said that, I think that we are definitely dealing with side effects of how we set the subsystem flags. I still have some remarks about bug 875210, but I think we have a good enough understanding of the causes at this point to move ahead.

Thanks for the additional info, Mark!

Comment 7

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/725acabf9df3

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/725acabf9df3