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

base/sys_info_win.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/sys_info.h"
 
 #include <windows.h>
 
 #include "base/file_path.h"
 #include "base/logging.h"
@@ skipping 77 matching lines @@
 
 // static
 size_t SysInfo::VMAllocationGranularity() {
   SYSTEM_INFO sysinfo;
   GetSystemInfo(&sysinfo);
 
   return sysinfo.dwAllocationGranularity;
 }
 
 // static
-void SysInfo::OperatingSystemVersionNumbers(int32 *major_version,
-                                            int32 *minor_version,
-                                            int32 *bugfix_version) {
-  OSVERSIONINFO info = {0};
-  info.dwOSVersionInfoSize = sizeof(info);
-  GetVersionEx(&info);
-  *major_version = info.dwMajorVersion;
-  *minor_version = info.dwMinorVersion;
+void SysInfo::OperatingSystemVersionNumbers(int32* major_version,
+                                            int32* minor_version,
+                                            int32* bugfix_version) {
+  static bool checked_version = false;
+  static int32 major = 0, minor = 0;
+  if (!checked_version) {

[brettw, 2011/03/24 20:19:03]

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]

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]

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]

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)?

+    checked_version = true;
+    OSVERSIONINFO info = {0};
+    info.dwOSVersionInfoSize = sizeof(info);
+    GetVersionEx(&info);
+    major = info.dwMajorVersion;
+    minor = info.dwMinorVersion;
+  }
+  *major_version = major;
+  *minor_version = minor;
   *bugfix_version = 0;
 }
 
 }  // namespace base