Closed Bug 875210 Opened 8 years ago Closed 8 years ago

stdout/stderr is lost in child processes

Categories

(Core :: IPC, defect)

All
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: markh, Assigned: markh)

References

Details

Attachments

(1 file, 1 obsolete file)

If an IPC child process writes to stderr/stdout (eg, assertions, dump(), etc), the output can't be seen, making debugging etc difficult.

The following patch solves the problem - it arranges for the child process to inherit the standard handles from the parent.  Note however that this is only effective when output from Firefox is redirected to a file - if Firefox is attached to a console the output is still not seen.  I've another patch for that, which I'll put in another bug - that other patch also relies on this patch to work, so I'll mark that one as depending on this.

Adding a few people to the CC who 'hg log' tells me have reviewed patches to this file in the past - if anyone can offer a review, or a better person to review, that would be great!
Attachment #753117 - Flags: review?
Blocks: 875214
Comment on attachment 753117 [details] [diff] [review]
Have the child process inherit the parent's std handles

interesting, I swear I've seen this output.
Attachment #753117 - Flags: review? → review?(aklotz)
Comment on attachment 753117 [details] [diff] [review]
Have the child process inherit the parent's std handles

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

Please see https://bugzilla.mozilla.org/show_bug.cgi?id=875214#c1.
Attachment #753117 - Flags: review?(aklotz) → review-
As I mentioned in the bug 875214, I think that Mark has convinced me that this is a worthwhile endeavor.

Having said that, I am still uneasy about setting bInheritHandles to TRUE, especially for content processes. Starting with Windows Vista, there are facilities that allow the parent process to explicitly declare which handles should be inherited (see http://blogs.msdn.com/b/oldnewthing/archive/2011/12/16/10248328.aspx). Could we add a wrapper around those facilities such that they fall through to regular CreateProcess on Windows XP but apply when running on Vista or newer? Then we could integrate that with this patch.
Flags: needinfo?(mhammond)
I didn't know that capability existed!  The following patch does exactly that.  I also discovered the chromium updates to offer the same facilities (eg, https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/chromium-checkins/BsnHSxyhJH4) and took a couple of bits of inspiration from that:

* Don't inherit stdin
* Don't attempt to inherit actual console handles as they aren't inheritable by all accounts.

It all works fine in all my testing and the above changes should make it a little clearer/easier if/when we update the chromium code in our tree to their latest (which I guess is likely as that is also where they implement "sandboxes processes IIUC)

Try is currently down - when it comes back up, I'll push a windows only bc run - I happen to know that the tree has at least one bc test suite (toolkit/components/thumbnails) which use these sub-processes - the try will not verify the output is seen, but should at least verify things don't die on XP etc...)
Attachment #753117 - Attachment is obsolete: true
Attachment #754188 - Flags: review?(aklotz)
Flags: needinfo?(mhammond)
Comment on attachment 754188 [details] [diff] [review]
Use ThreadAttributeList functions to only inherit the handles we request.

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

This looks good. Please fix HeapAlloc/HeapFree stuff, then r=me.

::: ipc/chromium/src/base/process_util_win.cc
@@ +234,5 @@
> +  if (!(*InitializeProcThreadAttributeListPtr)(NULL, 1, 0, &threadAttrSize) &&
> +      GetLastError() != ERROR_INSUFFICIENT_BUFFER)
> +    goto fail;
> +  lpAttributeList = reinterpret_cast<LPPROC_THREAD_ATTRIBUTE_LIST>
> +                              (HeapAlloc(GetProcessHeap(), 0, threadAttrSize));

Can we use malloc or new instead of HeapAlloc here, so we don't bypass any custom allocator stuff?

@@ +251,5 @@
> +  return lpAttributeList;
> +
> +fail:
> +  if (lpAttributeList)
> +    HeapFree(GetProcessHeap(), 0, lpAttributeList);

Same goes for HeapFree.

@@ +260,5 @@
> +void FreeThreadAttributeList(LPPROC_THREAD_ATTRIBUTE_LIST lpAttributeList) {
> +  // must be impossible to get a NULL DeleteProcThreadAttributeListPtr, as
> +  // we already checked it existed when creating the data we are now freeing.
> +  (*DeleteProcThreadAttributeListPtr)(lpAttributeList);
> +  HeapFree(GetProcessHeap(), 0, lpAttributeList);

Same goes for HeapFree.
Attachment #754188 - Flags: review?(aklotz) → review+
> This looks good. Please fix HeapAlloc/HeapFree stuff, then r=me.

I was thinking - we can almost certainly do away with the allocation completely - the buffer size returned is always the same size and fairly small (~32 bytes).  We could declare a (say) 64-byte buffer on the stack and use this - a worst-case scenario would be that windows would fail to use the buffer (ie, saying it is too small) and we'd just get no handle inheritance at all - but given we are only using one attribute list and that only ever gets exactly 1 or 2 handles, the risk even of that seems small.

Do you think that is worthwhile?  If so, I'll re-attach a patch that does that (and if not I'll just adjust this patch to use malloc/free and check it in)
Status: NEW → UNCONFIRMED
Ever confirmed: false
oops - I meant to set the status to "assigned" in the last comment, and forgot to add needinfo.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(aklotz)
I'm thinking that we should opt for future-proofing this code, so we shouldn't rely on Windows returning any particular structure size. Let's use malloc/free.
Flags: needinfo?(aklotz)
https://hg.mozilla.org/mozilla-central/rev/38fa9951ce6b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Comment on attachment 754188 [details] [diff] [review]
Use ThreadAttributeList functions to only inherit the handles we request.

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

::: ipc/chromium/src/base/process_util_win.cc
@@ +272,5 @@
> +  // blindly have all handles inherited.  Vista and later has a technique
> +  // where only specified handles are inherited - so we use this technique if
> +  // we can.  If that technique isn't available (or it fails), we just don't
> +  // inherit anything.  This means that dump() etc isn't going to be seen on
> +  // XP release builds, but that's OK (developers who really care can run a

Actually it's not ok :) since this prevents test output from making it to the parent and as such all e10s tests on xp fail unceremoniously due to a lack of output.

https://tbpl.mozilla.org/?tree=Holly

I'll be looking at fixing this in bug 1026133.
Blocks: 1026133
Blocks: 1037445
No longer blocks: 1026133
Depends on: 1321875
You need to log in before you can comment on or make changes to this bug.