test: add -no_rand_screen to s_client opts on Win

[shigeki]

RAND_screen() causes stability issues in invoking openssl-cli s_client on win2008r2 in CI. Disable to use it by adding -no_rand_screen options to all tls tests that use common.opensslCli.

The result of CI is https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/173/. There are existed errors on Ubuntu15 and arm but tls test errors on Win2008 are resolved as I've also made several tests on iojs+pr+win.

Fixes: #2150

R= @joaocgreis, @orangemocha or @rvagg

[Fishrock123]

We added -no_rand_screen, right?

How does enabling a flag we added help address an issue we've only recently been having? (I think?)

[shigeki]

How does enabling a flag we added help address an issue we've only recently been having? (I think?)

I think it would be hard.

Another approach is to build openssl-Cli without RAND_screen() by default on Win by adding and having such as -DNO_RAND_SCREEN so that no one would care about RAND_screen() issues in writing tls tests in the future. Or we change the flag name -with_rand_screen and the default value is not using RAND_screen().

[Fishrock123]

I think I should re-phrase: do we know any bits why RAND_screen() is unstable on win200r2 or why this has only popped up in like, the last month-ish?

[shigeki]

I think I should re-phrase: do we know any bits why RAND_screen() is unstable on win200r2 or why this has only popped up in like, the last month-ish?

I don't know it. The debug tests only show that GetDIBits() API gets crash. There seems something wrong on obtaining screen bitmap data at y=528 on that machine.

[Fishrock123]

I don't know it. The debug tests only show that GetDIBits() API gets crash. There seems something wrong on obtaining screen bitmap data at y=528 on that machine.

@nodejs/build how many windows2008r2 machines do we have? Can we try removing the machine that has this problem if if it seems to only be one? If so, maybe it is faulty?

[jbergstroem]

@Fishrock123 two of them. iojs-rackspace-win2008r2-2 fails in one case but seems to pass in another.

[Fishrock123]

@shigeki any idea if RAND_screen() / GetDIBits() was changed in a recent-ish OpenSSL upgrade?

[shigeki]

@Fishrock123 openssl/openssl@5015a93#diff-805d74d112f0f5faa66b6e5676afa328 is the commit of that and it was about 10 months ago. iojs has it since upgrading 1.0.2a in this April.

[rvagg]

We've changed the way we're logging in for jenkins on the windows machines (2008 and 2012), we've gone from running jenkins as a service to running jenkins manually from a cmd session via RDP and now we're running an auto-login session and starting jenkins in a terminal there. I'd be willing to bet this can be traced somehow to that change.

I'm +1 on this solution @shigeki, it's more robust to exclude RAND_screen() anyway although it does add noise to the tests but that's just one of the costs Windows adds so lgtm.

[joaocgreis]

LGTM

It would be good to find the root cause of this, but both @shigeki and me have spent some time investigating (#2150) and so far we couldn't find it. This openssl-cli is only used in the tests as a tool to test io.js, and RAND_screen() has been an issue before. For now, unblocking CI is a priority, and I think this patch is the correct way to do it. We should leave the original issue open and investigate it further.

[Fishrock123]

👍 then.

[thefourtheye]

Can we use common.isWindows everywhere in the tests?

[shigeki]

@thefourtheye Yes, we can but there are other 50 tests using process.platform === 'win32'. It should be another PR to replace them all.

[thefourtheye]

@shigeki Oh okay. I ll take care of them once this lands. But I feel that at least the new changes should use common. But that should not block this PR from landing.

[shigeki]

Thanks reviewing, everyone. Landed in ac7d3fa.
@thefourtheye Thanks. I don't want to make a kind of refactoring after reviewing unless it has a bug.