Closed Bug 1037445 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: DOM: Content Processes, defect)

x86_64
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jimm, Assigned: bobowen)

References

Details

Attachments

(1 file, 1 obsolete file)

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
Depends on: 875210
Blocks: 1026133
Duplicate of this bug: 989520
Assignee: nobody → bobowencode
I think we determined this also affects debug builds.
Summary: stdout doesn't propagate from the child to parent in Windows XP opt builds → stdout doesn't propagate from the child to parent in Windows XP builds
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.
Attachment #8504067 - Flags: review?(jmaher)
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 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?
Attachment #8504067 - Flags: review?(jmaher) → review-
(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?
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.
(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.
Attachment #8504067 - Attachment is obsolete: true
Attachment #8504626 - Flags: review?(jmaher)
Attachment #8504626 - Flags: review?(dvander)
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!
Attachment #8504626 - Flags: review?(jmaher) → review+
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?
Attachment #8504626 - Flags: review?(dvander) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/522b1d79b69e

Thanks to all for the reviews.
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/522b1d79b69e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Blocks: 1083701
Depends on: 1330496
You need to log in before you can comment on or make changes to this bug.