Fix a ChromeFrame crash which occured in the Histogram code while adding a histogram. The histogram

macros basically instantiate a static Histogram instance which is then tracked. The static initialization
is not thread safe and thus could cause a crash if there is a race between multiple threads trying
to access the object at the same time. 

Fix based on a discussion with Jim is to add thread safe versions of the macros we use for ChromeFrame.

Fixes bug http://code.google.com/p/chromium/issues/detail?id=37470

Bug=37470

Review URL: http://codereview.chromium.org/669135

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@40757 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 44 insertions, 19 deletions

diff --git a/chrome_frame/chrome_active_document.cc b/chrome_frame/chrome_active_document.cc
index 64cd750..4da4056e 100644
--- a/chrome_frame/chrome_active_document.cc
+++ b/chrome_frame/chrome_active_document.cc
@@ -239,8 +239,8 @@ STDMETHODIMP ChromeActiveDocument::Load(BOOL fully_avalable,
     url_fetcher_.UseMonikerForUrl(moniker_name, bind_context, url);
   }
 
-  UMA_HISTOGRAM_CUSTOM_COUNTS("ChromeFrame.FullTabLaunchType",
-                              is_chrome_protocol, 0, 1, 2);
+  THREAD_SAFE_UMA_HISTOGRAM_CUSTOM_COUNTS("ChromeFrame.FullTabLaunchType",
+                                          is_chrome_protocol, 0, 1, 2);
   return S_OK;
 }
 
diff --git a/chrome_frame/chrome_frame_activex_base.h b/chrome_frame/chrome_frame_activex_base.h
index 4fe5e04..1826fd5 100644
--- a/chrome_frame/chrome_frame_activex_base.h
+++ b/chrome_frame/chrome_frame_activex_base.h
@@ -236,11 +236,11 @@ END_MSG_MAP()
     // Used to perform one time tasks.
     if (g_first_launch_by_process_) {
       g_first_launch_by_process_ = false;
-      UMA_HISTOGRAM_CUSTOM_COUNTS("ChromeFrame.IEVersion",
-                                  GetIEVersion(),
-                                  IE_INVALID,
-                                  IE_8,
-                                  IE_8 + 1);
+      THREAD_SAFE_UMA_HISTOGRAM_CUSTOM_COUNTS("ChromeFrame.IEVersion",
+                                              GetIEVersion(),
+                                              IE_INVALID,
+                                              IE_8,
+                                              IE_8 + 1);
     }
     return S_OK;
   }
diff --git a/chrome_frame/chrome_frame_automation.cc b/chrome_frame/chrome_frame_automation.cc
index 321d09d..94a82a0 100644
--- a/chrome_frame/chrome_frame_automation.cc
+++ b/chrome_frame/chrome_frame_automation.cc
@@ -9,6 +9,7 @@
 #include "base/compiler_specific.h"
 #include "base/file_util.h"
 #include "base/file_version_info.h"
+#include "base/lock.h"
 #include "base/logging.h"
 #include "base/path_service.h"
 #include "base/process_util.h"
@@ -36,6 +37,11 @@ int kDefaultSendUMADataInterval = 20000;  // in milliseconds.
 
 static const wchar_t kUmaSendIntervalValue[] = L"UmaSendInterval";
 
+// This lock ensures that histograms created by ChromeFrame are thread safe.
+// The histograms created in ChromeFrame can be initialized on multiple
+// threads.
+Lock g_ChromeFrameHistogramLock;
+
 class TabProxyNotificationMessageFilter
     : public IPC::ChannelProxy::MessageFilter {
  public:
@@ -133,7 +139,8 @@ struct LaunchTimeStats {
 
   void Dump() {
     base::TimeDelta launch_time = base::Time::Now() - launch_time_begin_;
-    HISTOGRAM_TIMES("ChromeFrame.AutomationServerLaunchTime", launch_time);
+    THREAD_SAFE_UMA_HISTOGRAM_TIMES("ChromeFrame.AutomationServerLaunchTime",
+                                    launch_time);
     const int64 launch_milliseconds = launch_time.InMilliseconds();
     if (launch_milliseconds > kAutomationServerReasonableLaunchDelay) {
       LOG(WARNING) << "Automation server launch took longer than expected: " <<
@@ -302,18 +309,18 @@ void ProxyFactory::CreateProxy(ProxyFactory::ProxyCacheEntry* entry,
         base::TimeTicks::Now() - automation_server_launch_start_time_;
 
     if (entry->launch_result == AUTOMATION_SUCCESS) {
-      UMA_HISTOGRAM_TIMES("ChromeFrame.AutomationServerLaunchSuccessTime",
-                          delta);
+      THREAD_SAFE_UMA_HISTOGRAM_TIMES(
+          "ChromeFrame.AutomationServerLaunchSuccessTime", delta);
     } else {
-      UMA_HISTOGRAM_TIMES("ChromeFrame.AutomationServerLaunchFailedTime",
-                          delta);
+      THREAD_SAFE_UMA_HISTOGRAM_TIMES(
+          "ChromeFrame.AutomationServerLaunchFailedTime", delta);
     }
 
-    UMA_HISTOGRAM_CUSTOM_COUNTS("ChromeFrame.LaunchResult",
-                                entry->launch_result,
-                                AUTOMATION_SUCCESS,
-                                AUTOMATION_CREATE_TAB_FAILED,
-                                AUTOMATION_CREATE_TAB_FAILED + 1);
+    THREAD_SAFE_UMA_HISTOGRAM_CUSTOM_COUNTS("ChromeFrame.LaunchResult",
+                                            entry->launch_result,
+                                            AUTOMATION_SUCCESS,
+                                            AUTOMATION_CREATE_TAB_FAILED,
+                                            AUTOMATION_CREATE_TAB_FAILED + 1);
   }
 
   // Finally set the proxy.
@@ -761,10 +768,10 @@ void ChromeFrameAutomationClient::CreateExternalTab() {
     chrome_launch_params_.referrer,
   };
 
-  UMA_HISTOGRAM_CUSTOM_COUNTS(
+  THREAD_SAFE_UMA_HISTOGRAM_CUSTOM_COUNTS(
       "ChromeFrame.HostNetworking", !use_chrome_network_, 0, 1, 2);
 
-  UMA_HISTOGRAM_CUSTOM_COUNTS(
+  THREAD_SAFE_UMA_HISTOGRAM_CUSTOM_COUNTS(
       "ChromeFrame.HandleTopLevelRequests", handle_top_level_requests_, 0, 1,
       2);
 
diff --git a/chrome_frame/utils.h b/chrome_frame/utils.h
index 16fa836..adbd65e 100644
--- a/chrome_frame/utils.h
+++ b/chrome_frame/utils.h
@@ -11,6 +11,8 @@
 #include <urlmon.h>
 
 #include "base/basictypes.h"
+#include "base/histogram.h"
+#include "base/lock.h"
 #include "base/logging.h"
 #include "base/thread.h"
 
@@ -349,5 +351,21 @@ std::wstring GetActualUrlFromMoniker(IMoniker* moniker,
 // Checks if a window is a top level window
 bool IsTopLevelWindow(HWND window);
 
+extern Lock g_ChromeFrameHistogramLock;
+
+// Thread safe versions of the UMA histogram macros we use for ChromeFrame.
+// These should be used for histograms in ChromeFrame. If other histogram
+// macros from base/histogram.h are needed then thread safe versions of those
+// should be defined and used.
+#define THREAD_SAFE_UMA_HISTOGRAM_CUSTOM_COUNTS(name, sample, min, max, \
+                                                bucket_count) { \
+  AutoLock lock(g_ChromeFrameHistogramLock); \
+  UMA_HISTOGRAM_CUSTOM_COUNTS(name, sample, min, max, bucket_count); \
+}
+
+#define THREAD_SAFE_UMA_HISTOGRAM_TIMES(name, sample) { \
+  AutoLock lock(g_ChromeFrameHistogramLock); \
+  UMA_HISTOGRAM_TIMES(name, sample); \
+}
 
 #endif  // CHROME_FRAME_UTILS_H_