Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(386)

Issue 223353002: Revert of Fix --enable-logging after VS2013 switch (Closed)

Created:
6 years, 8 months ago by scottmg
Modified:
6 years, 8 months ago
Reviewers:
Mark Seaborn, brettw
CC:
chromium-reviews, erikwright+watch_chromium.org
Visibility:
Public.

Description

Revert of Fix --enable-logging after VS2013 switch (https://codereview.chromium.org/219813003/) Reason for revert: Possibly causing failures on nacl_integration x86-64 http://build.chromium.org/p/chromium.win/builders/NaCl%20Tests%20%28x86-64%29/builds/11185/steps/nacl_integration/logs/stdio#failure1 Original issue's description: > 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 TBR=mseaborn@chromium.org,brettw@chromium.org NOTREECHECKS=true NOTRY=true BUG=169941, 309197, 358267 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261286

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2 lines, -16 lines) Patch
M base/process/launch_win.cc View 1 chunk +2 lines, -16 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
scottmg
Created Revert of Fix --enable-logging after VS2013 switch
6 years, 8 months ago (2014-04-03 06:28:00 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scottmg@chromium.org/223353002/1
6 years, 8 months ago (2014-04-03 06:28:16 UTC) #2
commit-bot: I haz the power
6 years, 8 months ago (2014-04-03 06:31:06 UTC) #3
Message was sent while issue was closed.
Change committed as 261286

Powered by Google App Engine
This is Rietveld 408576698