Make the windows_version.h functions threadsafe by using a singleton.

Make the windows_version.h functions threadsafe by using a singleton.  Add accessors to the singleton for more values that various code wants, then convert almost everyone using OSVERSIONINFO or SYSTEM_INFO structs to calling these accessors.  Declare an AtExitManager in the out-of-process test runner since it didn't have one and that breaks singleton-using code in the test executable (as opposed to in chrome.dll).

A few other minor cleanups along the way (binding of "*", shorter code, etc.).  Because I ran into problems with it while modifying gcapi.cc, I cleaned up our usage of strsafe.h a bit, so that files that don't need it don't include it and files that do use STRSAFE_NO_DEPRECATE instead of a modified #include order.

BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=80819

[brettw, 2011-03-24 20:19:02]

http://codereview.chromium.org/6713107/diff/1/base/sys_info_win.cc
File base/sys_info_win.cc (right):

http://codereview.chromium.org/6713107/diff/1/base/sys_info_win.cc#newcode103
base/sys_info_win.cc:103: if (!checked_version) {
This makes this function no longer thread safe. I would probably expect to be
able to call this from a background thread. Is this what we want?

[Peter Kasting, 2011-03-24 20:59:02]

http://codereview.chromium.org/6713107/diff/1/base/sys_info_win.cc
File base/sys_info_win.cc (right):

http://codereview.chromium.org/6713107/diff/1/base/sys_info_win.cc#newcode103
base/sys_info_win.cc:103: if (!checked_version) {
On 2011/03/24 20:19:03, brettw wrote:
> This makes this function no longer thread safe.

Good point.  The reason I did this is because rvargas told me GetVersionEx()
might be slow, and we use similar non-threadsafe caching code in the other
callsites for it in windows_version.cc.  It seems like all should be threadsafe
or else none need to be.

I don't know the right answer.  Scanning the codebase for some uses of this it
looks like most should be changed to calling the windows_version code instead. 
But if those calls are on background threads that could introduce thread safety
problems too.

[brettw, 2011-03-24 23:47:37]

http://codereview.chromium.org/6713107/diff/1/base/sys_info_win.cc
File base/sys_info_win.cc (right):

http://codereview.chromium.org/6713107/diff/1/base/sys_info_win.cc#newcode103
base/sys_info_win.cc:103: if (!checked_version) {
You're more expert on the compilers here than I am, but it seems like as long as
checked_version is set after the major and minor are set, it should be safe.
Would it be sufficient to set checked_version at the end of the block and mark
it volatile?

[Peter Kasting, 2011-03-25 00:39:19]

http://codereview.chromium.org/6713107/diff/1/base/sys_info_win.cc
File base/sys_info_win.cc (right):

http://codereview.chromium.org/6713107/diff/1/base/sys_info_win.cc#newcode103
base/sys_info_win.cc:103: if (!checked_version) {
On 2011/03/24 23:47:37, brettw wrote:
> You're more expert on the compilers here than I am, but it seems like as long
as
> checked_version is set after the major and minor are set, it should be safe.
> Would it be sufficient to set checked_version at the end of the block and mark
> it volatile?

volatile isn't what you want, that means "can be changed by external factors,
e.g. for memory mapped hardware signals" and doesn't guarantee thread safety
(see the end of http://en.wikipedia.org/wiki/Memory_barrier).  You'd want a
hardware memory barrier, which is a big hassle.  Probably easier to write a
critical section.

It's still not obvious to me whether these really need to be threadsafe.  Maybe
the onus is on me to audit all existing callers (and all callers of the
windows_version stuff.  Ugh)?

[brettw, 2011-03-26 18:28:13]

I found at least one case in the Geolocation code where the windows_version
stuff is used from a background thread. Maybe we should make windows_version
threadsafe and cache the results, and just have sys_info call into that?

[Peter Kasting, 2011-04-05 21:11:52]

OK, I converted to using a singleton, which is threadsafe, caches all the
appropriate data at construction time, and let me add accessors for a number of
additional data items without adding lots of global-scope "BASE_API GetXXX();"
functions.

To avoid having to change 150 or so calls to GetVersion() to be much more
verbose and run across multiple lines, I left the Version enum and a
GetVersion() wrapper function outside the singleton scope.

[tfarina, 2011-04-05 23:10:47]

http://codereview.chromium.org/6713107/diff/12005/chrome/browser/importer/imp...
File chrome/browser/importer/importer_unittest.cc (right):

http://codereview.chromium.org/6713107/diff/12005/chrome/browser/importer/imp...
chrome/browser/importer/importer_unittest.cc:28: #include
"base/win/windows_version.h"
This is already included at line 42.

[Peter Kasting, 2011-04-05 23:45:32]

http://codereview.chromium.org/6713107/diff/12005/chrome/browser/importer/imp...
File chrome/browser/importer/importer_unittest.cc (right):

http://codereview.chromium.org/6713107/diff/12005/chrome/browser/importer/imp...
chrome/browser/importer/importer_unittest.cc:28: #include
"base/win/windows_version.h"
On 2011/04/05 23:10:47, tfarina wrote:
> This is already included at line 42.

Ah, missed that, thanks.

[Peter Kasting, 2011-04-06 00:10:54]

New snap up that avoids adding a dependency from gcapi.cc onto base/, per some
discussion with gwilson.

[Peter Kasting, 2011-04-06 01:34:37]

Another new snap up, fixing several problems I noticed in a self-review.  Random
notes:

* Changing the OperatingSystemVersion() string affects diagnostic mode printout,
which I cleared with cpu, as well as the metrics service (no actual .cc file
touched), which cleared with ifette.
* The one line where I flipped a "< Vista" to ">= Vista" was intentional, to fix
an erroneous change the opposite direction in a previous CL.

[brettw, 2011-04-06 04:27:47]

http://codereview.chromium.org/6713107/diff/17001/chrome/installer/gcapi/gcap...
File chrome/installer/gcapi/gcapi.cc (right):

http://codereview.chromium.org/6713107/diff/17001/chrome/installer/gcapi/gcap...
chrome/installer/gcapi/gcapi.cc:175: return (version_info.dwMajorVersion < 5) ||
I can't follow this at all, and I found the old version much easier. Can you
unroll this a lot?

http://codereview.chromium.org/6713107/diff/17001/sandbox/src/sandbox_utils.cc
File sandbox/src/sandbox_utils.cc (right):

http://codereview.chromium.org/6713107/diff/17001/sandbox/src/sandbox_utils.c...
sandbox/src/sandbox_utils.cc:60: base::win::Version version =
base::win::GetVersion();
This ends up getting the singleton twice. This probably isn't a big deal, but it
seems nicer to just get the OSInfo and then use it in both places.

[Peter Kasting, 2011-04-06 04:52:43]

http://codereview.chromium.org/6713107/diff/17001/chrome/installer/gcapi/gcap...
File chrome/installer/gcapi/gcapi.cc (right):

http://codereview.chromium.org/6713107/diff/17001/chrome/installer/gcapi/gcap...
chrome/installer/gcapi/gcapi.cc:175: return (version_info.dwMajorVersion < 5) ||
On 2011/04/06 04:27:47, brettw wrote:
> I can't follow this at all, and I found the old version much easier. Can you
> unroll this a lot?

Um... OK... the old code happened to be wrong, though, which is part of what
made it simpler (it erroneously treated Windows Server 2003 <SP2 as
unsupported).

I will give it a shot.

http://codereview.chromium.org/6713107/diff/17001/sandbox/src/sandbox_utils.cc
File sandbox/src/sandbox_utils.cc (right):

http://codereview.chromium.org/6713107/diff/17001/sandbox/src/sandbox_utils.c...
sandbox/src/sandbox_utils.cc:60: base::win::Version version =
base::win::GetVersion();
On 2011/04/06 04:27:47, brettw wrote:
> This ends up getting the singleton twice. This probably isn't a big deal, but
it
> seems nicer to just get the OSInfo and then use it in both places.

That would make the code noticeably more verbose for no efficiency win (as the
second singleton access is guaranteed to just return the pointer) -- if we even
access the singleton a second time at all.

[brettw, 2011-04-06 05:14:35]

http://codereview.chromium.org/6713107/diff/17001/sandbox/src/sandbox_utils.cc
File sandbox/src/sandbox_utils.cc (right):

http://codereview.chromium.org/6713107/diff/17001/sandbox/src/sandbox_utils.c...
sandbox/src/sandbox_utils.cc:60: base::win::Version version =
base::win::GetVersion();
Okay, I don't actually care about this.

[Peter Kasting, 2011-04-06 18:27:13]

New snap up.  I tried to make gcapi.cc more comprehensible.  I also added a
solution for the hanging test issues by way of out_of_proc_test_runner.cc, which
I am scared of.

[Paweł Hajdan Jr., 2011-04-06 19:20:13]

Drive-by with testing comments.

Can we delay landing this change until we're reasonably sure what's going on?
The thing is getting dangerously hacky... If there is no other way I guess we'll
have to do something like that, but let's consider all options first.

I'll reply with more details in the off-review e-mail thread.

[Peter Kasting, 2011-04-06 20:06:48]

On 2011/04/06 19:20:13, Paweł Hajdan Jr. wrote:
> Drive-by with testing comments.
> 
> Can we delay landing this change until we're reasonably sure what's going on?
> The thing is getting dangerously hacky.

Replied in the mail thread.  I've uploaded a new snap that syncs to ToT and
revises the comment now that I feel more sure of myself.

[Peter Kasting, 2011-04-06 20:24:50]

On 2011/04/06 20:06:48, Peter Kasting wrote:
> I've uploaded a new snap

Well, actually, I haven't yet (missed that it was waiting for me to reply to a
presubmit check, now I'm testing some local changes).

[cpu_(ooo_6.6-7.5), 2011-04-06 21:11:42]

I suggest to add rvargas for the wow64 sandbox changes even though they seem
trivial.

http://codereview.chromium.org/6713107/diff/20010/chrome/test/out_of_proc_tes...
File chrome/test/out_of_proc_test_runner.cc (right):

http://codereview.chromium.org/6713107/diff/20010/chrome/test/out_of_proc_tes...
chrome/test/out_of_proc_test_runner.cc:588: base::AtExitManager exit_manager;
seem correct to me. Different modules need different atexit managers, at least
on windows.

[rvargas (doing something else), 2011-04-06 23:28:05]

Drive by.

http://codereview.chromium.org/6713107/diff/20010/base/win/windows_version.h
File base/win/windows_version.h (right):

http://codereview.chromium.org/6713107/diff/20010/base/win/windows_version.h#...
base/win/windows_version.h:35: // not be the same as the processor's "native
architecture".  For example,
nit: could you remove the part about the "processor's "native architecture""?.
It took me a while to decode why the example agrees with the comment, and that
would have been obvious if said piece was not there.

http://codereview.chromium.org/6713107/diff/20010/base/win/windows_version.h#...
base/win/windows_version.h:61: // The next two functions return arrays of
values, [major, minor(, build)].
Have you considered defining a couple of structs here and returning a pointer to
that instead? Having the callers just indexing an array feels wrong.

http://codereview.chromium.org/6713107/diff/20010/sandbox/src/sandbox_utils.cc
File sandbox/src/sandbox_utils.cc (right):

http://codereview.chromium.org/6713107/diff/20010/sandbox/src/sandbox_utils.c...
sandbox/src/sandbox_utils.cc:62: ((version == base::win::VERSION_XP) &&
nit: I think this should be indented like an if (under version)

http://codereview.chromium.org/6713107/diff/20010/sandbox/tests/common/contro...
File sandbox/tests/common/controller.cc (right):

http://codereview.chromium.org/6713107/diff/20010/sandbox/tests/common/contro...
sandbox/tests/common/controller.cc:59: base::win::OSInfo::WOW64_ENABLED) ?
nit: The previous structure was easier to read... but I suspect it's caused by
the indentation.

[brettw, 2011-04-06 23:38:26]

LGTM from my end, I like Ricardo's suggestion about structs.

[Peter Kasting, 2011-04-07 00:29:19]

I intend to submit this once the trybots OK it.

http://codereview.chromium.org/6713107/diff/20010/base/win/windows_version.h
File base/win/windows_version.h (right):

http://codereview.chromium.org/6713107/diff/20010/base/win/windows_version.h#...
base/win/windows_version.h:35: // not be the same as the processor's "native
architecture".  For example,
On 2011/04/06 23:28:05, rvargas wrote:
> nit: could you remove the part about the "processor's "native architecture""?.
> It took me a while to decode why the example agrees with the comment, and that
> would have been obvious if said piece was not there.

Done.

http://codereview.chromium.org/6713107/diff/20010/base/win/windows_version.h#...
base/win/windows_version.h:61: // The next two functions return arrays of
values, [major, minor(, build)].
On 2011/04/06 23:28:05, rvargas wrote:
> Have you considered defining a couple of structs here and returning a pointer
to
> that instead? Having the callers just indexing an array feels wrong.

Good idea!  Done.

http://codereview.chromium.org/6713107/diff/20010/chrome/test/out_of_proc_tes...
File chrome/test/out_of_proc_test_runner.cc (right):

http://codereview.chromium.org/6713107/diff/20010/chrome/test/out_of_proc_tes...
chrome/test/out_of_proc_test_runner.cc:588: base::AtExitManager exit_manager;
On 2011/04/06 21:11:42, cpu wrote:
> Different modules need different atexit managers, at least
> on windows.

This comment led me to clarify the comment block here even more.

http://codereview.chromium.org/6713107/diff/20010/sandbox/src/sandbox_utils.cc
File sandbox/src/sandbox_utils.cc (right):

http://codereview.chromium.org/6713107/diff/20010/sandbox/src/sandbox_utils.c...
sandbox/src/sandbox_utils.cc:62: ((version == base::win::VERSION_XP) &&
On 2011/04/06 23:28:05, rvargas wrote:
> nit: I think this should be indented like an if (under version)

ifs are indent-4 too.  It coincidentally happens that "if (" is 4 characters. 
To make sure, I asked Brett how I should indent here, and he said to do it how I
have it currently.

http://codereview.chromium.org/6713107/diff/20010/sandbox/tests/common/contro...
File sandbox/tests/common/controller.cc (right):

http://codereview.chromium.org/6713107/diff/20010/sandbox/tests/common/contro...
sandbox/tests/common/controller.cc:59: base::win::OSInfo::WOW64_ENABLED) ?
On 2011/04/06 23:28:05, rvargas wrote:
> nit: The previous structure was easier to read... but I suspect it's caused by
> the indentation.

Yeah, having to linebreak + indent the conditional really mucks this up :(

[Paweł Hajdan Jr., 2011-04-07 14:59:26]

Code I commented in the drive-by LGTM, thanks for additional tests and
explanations in the e-mail.