Fix --enable-logging after VS2013 switch

Fix --enable-logging after VS2013 switch

After https://src.chromium.org/viewvc/chrome?view=rev&revision=178656
a valid fileno of stdout or stderr abort connection of the console to
stdio on Windows.

Looking at the VS CRT between 2010 and 2013, it seems the behavior
that r178656 was relying on changed:

(in __initstdio)

C:\Program Files (x86)>diff "Microsoft Visual Studio 10.0\vc\crt\src\_file.c" "Microsoft Visual Studio 12.0\vc\crt\src\_file.c"
144,157d143
<         for ( i = 0 ; i < 3 ; i++ ) {
<             if ( (_osfhnd(i) == (intptr_t)INVALID_HANDLE_VALUE) ||
<                  (_osfhnd(i) == _NO_CONSOLE_FILENO) ||
<                  (_osfhnd(i) == 0) )
<             {
<             /*
<              * For stdin, stdout & stderr, we use _NO_CONSOLE_FILENO (a value
<              * different from _INVALID_HANDLE_VALUE to distinguish between
<              * a failure in opening a file & a program run without a console.
<              */
<                 _iob[i]._file = _NO_CONSOLE_FILENO;
<             }
<         }
<
194a181
>         __piob = NULL;


There's an open bug

http://connect.microsoft.com/VisualStudio/feedback/details/785119/

but as it was broken in 2012 and not fixed in 2013, it doesn't look like it's
going to be fixed. (The documentation still suggests that r178656 is correct:
http://msdn.microsoft.com/en-us/library/zs6wbdhx(v=vs.100).aspx vs
http://msdn.microsoft.com/en-us/library/zs6wbdhx(v=vs.120).aspx ) but that's
little consolation.

So, confirm that the underlying HANDLE is valid before aborting.

TBR=brettw@chromium.org
R=mseaborn@chromium.org
BUG=169941, 309197, 358267
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260738

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261253

[scottmg, 2014-03-31 18:26:28]

Hi Mark, I'm a bit unclear on the specifics of what your previous patch was
addressing, but does this seem reasonable?

[Mark Seaborn, 2014-03-31 18:39:08]

The change looks OK.  Did you check that Chrome still produces logging when run
under Cygwin's terminal (with and without "--enable-logging")?

If you don't have Cygwin, it should be equivalent to check whether:
  chrome --enable-logging >log 2>&1
sends logging to the 'log' file.

https://codereview.chromium.org/219813003/diff/1/base/process/launch_win.cc
File base/process/launch_win.cc (right):

https://codereview.chromium.org/219813003/diff/1/base/process/launch_win.cc#n...
base/process/launch_win.cc:70: intptr_t stdout_handle =
_get_osfhandle(_fileno(stdout));
Does intptr_t work on the MSVC toolchains that Chrome supports?  (I don't see
trybot runs for this change. :-) )  stdint.h used not to be available in MSVC.

[scottmg, 2014-03-31 18:46:04]

Thanks.

On 2014/03/31 18:39:08, Mark Seaborn wrote:
> The change looks OK.  Did you check that Chrome still produces logging when
run
> under Cygwin's terminal (with and without "--enable-logging")?
> 
> If you don't have Cygwin, it should be equivalent to check whether:
>   chrome --enable-logging >log 2>&1
> sends logging to the 'log' file.

Yes, that seems to work OK with and without --enable-logging.

> 
> https://codereview.chromium.org/219813003/diff/1/base/process/launch_win.cc
> File base/process/launch_win.cc (right):
> 
>
https://codereview.chromium.org/219813003/diff/1/base/process/launch_win.cc#n...
> base/process/launch_win.cc:70: intptr_t stdout_handle =
> _get_osfhandle(_fileno(stdout));
> Does intptr_t work on the MSVC toolchains that Chrome supports?  (I don't see
> trybot runs for this change. :-) )  stdint.h used not to be available in MSVC.

[scottmg, 2014-03-31 18:46:13]

https://codereview.chromium.org/219813003/diff/1/base/process/launch_win.cc
File base/process/launch_win.cc (right):

https://codereview.chromium.org/219813003/diff/1/base/process/launch_win.cc#n...
base/process/launch_win.cc:70: intptr_t stdout_handle =
_get_osfhandle(_fileno(stdout));
On 2014/03/31 18:39:08, Mark Seaborn wrote:
> Does intptr_t work on the MSVC toolchains that Chrome supports?  (I don't see
> trybot runs for this change. :-) )  stdint.h used not to be available in MSVC.

Assuming it works for the various compilers of base that aren't MSVC, it should
be ok, we only build with 2013 now. trybots started now anyway. :)

[scottmg, 2014-03-31 18:58:39]

+brettw

[Mark Seaborn, 2014-03-31 19:37:13]

On 31 March 2014 11:46, <scottmg@chromium.org> wrote:

> https://codereview.chromium.org/219813003/diff/1/base/
> process/launch_win.cc
> File base/process/launch_win.cc (right):
>
> https://codereview.chromium.org/219813003/diff/1/base/
> process/launch_win.cc#newcode70
> base/process/launch_win.cc:70: intptr_t stdout_handle =
> _get_osfhandle(_fileno(stdout));
> On 2014/03/31 18:39:08, Mark Seaborn wrote:
>
>> Does intptr_t work on the MSVC toolchains that Chrome supports?  (I
>>
> don't see
>
>> trybot runs for this change. :-) )  stdint.h used not to be available
>>
> in MSVC.
>
> Assuming it works for the various compilers of base that aren't MSVC, it
> should be ok, we only build with 2013 now. trybots started now anyway.
> :)


OK, LGTM, thanks.

Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[scottmg, 2014-03-31 21:54:30]

tbr Brett

[scottmg, 2014-03-31 21:54:34]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-03-31 21:54:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/219813003/20001

[commit-bot: I haz the power, 2014-04-01 01:47:38]

Change committed as 260738

[tkent, 2014-04-01 07:23:37]

A revert of this CL has been created in
https://codereview.chromium.org/220653002/ by tkent@chromium.org.

The reason for reverting is: Speculative revert for massive failures of XP
browser_tests.

http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/builds...
.

[tkent, 2014-04-01 08:59:55]

On 2014/04/01 07:23:37, tkent wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/220653002/ by mailto:tkent@chromium.org.
> 
> The reason for reverting is: Speculative revert for massive failures of XP
> browser_tests.
> 
>
http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/builds...
> .

Also, this CL broke nacl_integration test, and the revert fixed the failure.

http://build.chromium.org/p/chromium.win/builders/NaCl%20Tests%20%28x86-64%29...

[tkent, 2014-04-01 10:32:10]

On 2014/04/01 07:23:37, tkent wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/220653002/ by mailto:tkent@chromium.org.
> 
> The reason for reverting is: Speculative revert for massive failures of XP
> browser_tests.
> 
>
http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/builds...
> .

Conformed this CL was the culprit of XP browser_tests failures.

[scottmg, 2014-04-01 17:06:15]

Thanks for reverting, it looks NaCl-related in the browser_tests run too.


On Tue, Apr 1, 2014 at 12:23 AM, <tkent@chromium.org> wrote:

> A revert of this CL has been created in
> https://codereview.chromium.org/220653002/ by tkent@chromium.org.
>
> The reason for reverting is: Speculative revert for massive failures of XP
> browser_tests.
>
> http://build.chromium.org/p/chromium.win/builders/XP%
> 20Tests%20%281%29/builds/30725
> .
>
> https://codereview.chromium.org/219813003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[scottmg, 2014-04-02 00:36:00]

After poking at this most of the day, I'm just going to try relanding without
running this code on XP. I have no idea why it's problematic; details at
https://code.google.com/p/chromium/issues/detail?id=358267#c4 .

[scottmg, 2014-04-02 00:36:23]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-04-02 00:36:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/219813003/60001

[commit-bot: I haz the power, 2014-04-02 00:41:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-02 00:41:31]

Internal error: failed to checkout. Please try again.

[scottmg, 2014-04-02 00:43:23]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-04-02 00:47:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/219813003/60001

[commit-bot: I haz the power, 2014-04-02 02:12:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/219813003/60001

[commit-bot: I haz the power, 2014-04-02 10:17:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-02 10:17:57]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel
  tryserver.chromium on linux_chromium_rel

[scottmg, 2014-04-02 16:44:22]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-04-02 16:45:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/219813003/60001

[commit-bot: I haz the power, 2014-04-03 01:34:37]

Change committed as 261253

[scottmg, 2014-04-03 06:27:59]

A revert of this CL has been created in
https://codereview.chromium.org/223353002/ by scottmg@chromium.org.

The reason for reverting is: Possibly causing failures on nacl_integration
x86-64

http://build.chromium.org/p/chromium.win/builders/NaCl%20Tests%20%28x86-64%29....