1037445 - stdout doesn't propagate from the child to parent in Windows XP builds

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Jim Mathies [:jimm]]

Aside from debugging headaches, we can't run e10s enabled tests on XP opt builds due to this.

http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/process_util_win.cc#276

Comment 2

[Jim Mathies [:jimm]]

I think we determined this also affects debug builds.

Comment 3

[Bob Owen (:bobowen)]

Attachment: When pre-Vista, for testing purposes allow std handles to be inherited by child process when an env var is set.

Joel, I discussed this with Ted on Friday, but he's away for a week, so I hope you don't mind standing in. :)

I want child processes to inherit stdout/err on Windows XP when running certain tests.
Normally we don't do this pre-Vista, because you can't limit what other handles get inherited.
We eventually decided on setting a MOZ_RUNNING_TESTS environment variable.

However, while I was writing a warning comment along the lines of "only use this as a last resort ..." and "it might not be set by all test harnesses ...", I decided I was creating a foot-gun with a hair-trigger.
So, I've changed it to an env var specific to this use case: MOZ_INHERIT_STD_HANDLES.

I'll be getting a separate review for the ipc changes, just trying to find the best person.

Comment 4

[Jim Mathies [:jimm]]

Comment on attachment 8504067 [details] [diff] [review]
When pre-Vista, for testing purposes allow std handles to be inherited by child process when an env var is set.

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

This comment should get an update I think - 

http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/process_util_win.cc#273

::: testing/mochitest/runtests.py
@@ +1238,5 @@
>      # via the commandline at your own risk.
>      browserEnv["XPCOM_DEBUG_BREAK"] = "stack"
>  
> +    # When creating child processes, we need to inherit stdout/err handles for
> +    # test logging and log parsing purposes.

I would suggest mentioning Win XP here so when we drop support, releng will know this can come out.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8504067 [details] [diff] [review]
When pre-Vista, for testing purposes allow std handles to be inherited by child process when an env var is set.

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

I think this is ok, but I really don't understand; Maybe some more context or information would help me feel comfortable with a r+!

::: ipc/chromium/src/base/process_util_win.cc
@@ +326,4 @@
>      startup_info.dwFlags |= STARTF_USESTDHANDLES;
>      startup_info.hStdOutput = ::GetStdHandle(STD_OUTPUT_HANDLE);
>      startup_info.hStdError = ::GetStdHandle(STD_ERROR_HANDLE);
>      startup_info.hStdInput = INVALID_HANDLE_VALUE;

I am not an expert in win32 process creation flags, this code leaves me with some questions (ignore if they are really n00b):
* what is handleCount?  is that something that isn't set on windows?  winxp?
* before we were using startup_info.cb and dwCreationFlags, now we are not for the winxp case?

maybe a comment here why and what cases use MOZ_INHERIT_STD_HANDLES

::: testing/mochitest/runtests.py
@@ +1239,5 @@
>      browserEnv["XPCOM_DEBUG_BREAK"] = "stack"
>  
> +    # When creating child processes, we need to inherit stdout/err handles for
> +    # test logging and log parsing purposes.
> +    browserEnv["MOZ_INHERIT_STD_HANDLES"] = "1"

so this will be for all desktop platforms?

Comment 6

[Bob Owen (:bobowen)]

(In reply to Joel Maher (:jmaher) from comment #5)

Thanks for looking at this.

> ::: ipc/chromium/src/base/process_util_win.cc
> @@ +326,4 @@
> >      startup_info.dwFlags |= STARTF_USESTDHANDLES;
> >      startup_info.hStdOutput = ::GetStdHandle(STD_OUTPUT_HANDLE);
> >      startup_info.hStdError = ::GetStdHandle(STD_ERROR_HANDLE);
> >      startup_info.hStdInput = INVALID_HANDLE_VALUE;
> 
> I am not an expert in win32 process creation flags, this code leaves me with
> some questions (ignore if they are really n00b):
> * what is handleCount?  is that something that isn't set on windows?  winxp?

This is just used to track how many handles are inheritable and will be set up in the Thread Attribute Handle List.
This is the list of handles that we want to inherit and the docs say they must be inheritable, which is why I assume the checks are there.
You can't limit the handles that are inherited in this way for WinXP, which is why we don't normally inherit at all. So, this isn't relevant for pre-Vista.

> * before we were using startup_info.cb and dwCreationFlags, now we are not
> for the winxp case?

You can't use extended startup info (STARTUPINFOEX) pre-Vista.
To make this work for XP and later, the chromium guys have used a STARTUPINFOEX and then grabbed the STARTUPINFO that exists within it.
Setting the cb (size of structure in bytes) to the length of the STARTUPINFO.
This means that XP just sees it as a normal STARTUPINFO.

If we are Vista or later and we need to use the extended information to limit the inherited handles, we set the size back to that of STARTUPINFOEX and set the flag to say the extended information is there.

I hope that makes sense. :)

> maybe a comment here why and what cases use MOZ_INHERIT_STD_HANDLES

I could say that this is only used for mochitests, but I didn't want to be too specific, otherwise we'd have to update this comment if we find other cases where we need to inherit them.

> ::: testing/mochitest/runtests.py
> @@ +1239,5 @@
> >      browserEnv["XPCOM_DEBUG_BREAK"] = "stack"
> >  
> > +    # When creating child processes, we need to inherit stdout/err handles for
> > +    # test logging and log parsing purposes.
> > +    browserEnv["MOZ_INHERIT_STD_HANDLES"] = "1"
> 
> so this will be for all desktop platforms?

I was thinking that this might be useful for other platforms, but looking at the test runs on Holly, this really is just an issue for Windows XP.
So, maybe MOZ_WIN_INHERIT_STD_HANDLES or to be really specific MOZ_WIN_INHERIT_STD_HANDLES_PRE_VISTA?

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

Thanks for explaining this in more detail, I think I got what you are doing now.

MOZ_WIN_INHERIT_STD_HANDLES_PRE_VISTA has the most context, but it is sure a long env var :)  Either do that or drop the PRE_VISTA.

Comment 8

[Bob Owen (:bobowen)]

Attachment: When pre-Vista, for testing purposes allow std handles to be inherited by child process when an env var is set. v2

(In reply to Joel Maher (:jmaher) from comment #7)

> MOZ_WIN_INHERIT_STD_HANDLES_PRE_VISTA has the most context, but it is sure a
> long env var :)  Either do that or drop the PRE_VISTA.

Well it is, but given Jim's point about making it clear that this can go once we stop supporting pre-Vista releases, I think that the extra context is worth the extra characters. :)

I've also just set it for one flavour of mochitests as these appear to be the ones that need it.
We may have to add other flavours in the future, but it is probably best to test without inheriting the handles if we can.

Here's a mochitest try push with e10s turned on for WinXP and Win7:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c7e0ed7c18f8

The two differences between WinXP and Win7 are not related to this problem.

(In reply to Jim Mathies [:jimm] from comment #4)

> This comment should get an update I think - 
> 
> http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/
> process_util_win.cc#273

Changed the end of the comment to take account of the new behaviour, thanks.

> > +    # When creating child processes, we need to inherit stdout/err handles for
> > +    # test logging and log parsing purposes.
> 
> I would suggest mentioning Win XP here so when we drop support, releng will
> know this can come out.

I've made this comment and the env var much more specific, as this does just appear to be a Windows XP / pre-Vista issue.

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8504626 [details] [diff] [review]
When pre-Vista, for testing purposes allow std handles to be inherited by child process when an env var is set. v2

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

Thanks Bob!

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8504626 [details] [diff] [review]
When pre-Vista, for testing purposes allow std handles to be inherited by child process when an env var is set. v2

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

lgtm

::: ipc/chromium/src/base/process_util_win.cc
@@ +325,3 @@
>      startup_info.dwFlags |= STARTF_USESTDHANDLES;
>      startup_info.hStdOutput = ::GetStdHandle(STD_OUTPUT_HANDLE);
>      startup_info.hStdError = ::GetStdHandle(STD_ERROR_HANDLE);

nit: Can these be stdOut/stdErr from above?

Comment 11

[Bob Owen (:bobowen)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/522b1d79b69e

Thanks to all for the reviews.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/522b1d79b69e