Fixes for a couple of ChromeFrame crashes seen in the latest dev channel build. These

crashes occur in the Chrome HTTP stack which is used for uploading UMA data. I could not
repro these crashes though and they seem to occur while posting tasks to a deleted message
loop. Currently we create an io thread on the fly for uploading the uma data and destroy
it when we are done. To workaround this issue we are attempting to create one IO thread
and leave it running. At this point this thread object is leaked as we don't have a good
way of stopping this from the IO thread. Added a TODO in the code to this effect.

I also added a check for whether the ChromeFrameMetricsDataUploader::Initialize function
succeeds as it appears that there are failures on the field due to failure in creating
the unnamed window.

The ChromeFrame metrics service object is no longer a ThreadLocal object.

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

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

2 files changed, 70 insertions, 25 deletions

diff --git a/chrome_frame/metrics_service.cc b/chrome_frame/metrics_service.cc
index 79ceace..6cd4516 100644
--- a/chrome_frame/metrics_service.cc
+++ b/chrome_frame/metrics_service.cc
@@ -52,6 +52,7 @@
 #endif
 
 #include "base/file_version_info.h"
+#include "base/lock.h"
 #include "base/string_util.h"
 #include "base/thread.h"
 #include "base/string_number_conversions.h"
@@ -90,8 +91,37 @@ static const int kInitialUMAUploadTimeoutMilliSeconds = 30000;
 // Default to one UMA upload per 10 mins.
 static const int kMinMilliSecondsPerUMAUpload = 600000;
 
-base::LazyInstance<base::ThreadLocalPointer<MetricsService> >
-    MetricsService::g_metrics_instance_(base::LINKER_INITIALIZED);
+base::LazyInstance<MetricsService>
+    g_metrics_instance_(base::LINKER_INITIALIZED);
+
+// Traits to create an instance of the ChromeFrame upload thread.
+struct UploadThreadInstanceTraits
+    : public base::DefaultLazyInstanceTraits<base::Thread> {
+  static base::Thread* New(void* instance) {
+    // Use placement new to initialize our instance in our preallocated space.
+    // The parenthesis is very important here to force POD type initialization.
+    base::Thread* upload_thread =
+        new (instance) base::Thread("ChromeFrameUploadThread");
+    base::Thread::Options options;
+    options.message_loop_type = MessageLoop::TYPE_IO;
+    bool ret = upload_thread->StartWithOptions(options);
+    if (!ret) {
+      NOTREACHED() << "Failed to start upload thread";
+    }
+    return upload_thread;
+  }
+};
+
+// ChromeFrame UMA uploads occur on this thread. This thread is started on the
+// IE UI thread. This thread needs to be stopped on the same thread it was
+// started on. We don't have a good way of achieving this at this point. This
+// thread object is currently leaked.
+// TODO(ananta)
+// Fix this.
+base::LazyInstance<base::Thread, UploadThreadInstanceTraits>
+    g_metrics_upload_thread_(base::LINKER_INITIALIZED);
+
+Lock g_metrics_service_lock;
 
 extern base::LazyInstance<StatisticsRecorder> g_statistics_recorder_;
 
@@ -209,8 +239,7 @@ class ChromeFrameMetricsDataUploader
   END_MSG_MAP()
 
   ChromeFrameMetricsDataUploader()
-      : fetcher_(NULL),
-        upload_thread_("ChromeFrameMetricsUploadThread") {
+      : fetcher_(NULL) {
     DLOG(INFO) << __FUNCTION__;
     creator_thread_id_ = PlatformThread::CurrentId();
   }
@@ -225,20 +254,25 @@ class ChromeFrameMetricsDataUploader
   }
 
   bool Initialize() {
+    bool ret = false;
+
     if (!Create(NULL, NULL, NULL, WS_OVERLAPPEDWINDOW)) {
       NOTREACHED() << "Failed to create window";
-      return false;
+      return ret;
     }
     DCHECK(IsWindow());
 
+    if (!g_metrics_upload_thread_.Get().IsRunning()) {
+      NOTREACHED() << "Upload thread is not running";
+      return ret;
+    }
+
+    ret = true;
     // Grab a reference to the current instance which ensures that it stays
     // around until the HTTP request initiated below completes.
     // Corresponding Release is in OnFinalMessage.
     AddRef();
-
-    base::Thread::Options options;
-    options.message_loop_type = MessageLoop::TYPE_IO;
-    return upload_thread_.StartWithOptions(options);
+    return ret;
   }
 
   bool Uninitialize() {
@@ -251,18 +285,28 @@ class ChromeFrameMetricsDataUploader
     scoped_refptr<ChromeFrameMetricsDataUploader> data_uploader =
         new ChromeFrameMetricsDataUploader();
 
-    data_uploader->Initialize();
-    DCHECK(data_uploader->upload_thread_.message_loop());
+    if (!data_uploader->Initialize()) {
+      NOTREACHED() << "Failed to initialize ChromeFrameMetricsDataUploader";
+      return E_FAIL;
+    }
+
+    MessageLoop* io_loop = g_metrics_upload_thread_.Get().message_loop();
+    if (!io_loop) {
+      NOTREACHED() << "Failed to initialize ChromeFrame UMA upload thread";
+      return E_FAIL;
+    }
 
-    data_uploader->upload_thread_.message_loop()->PostTask(FROM_HERE,
+    io_loop->PostTask(
+        FROM_HERE,
         NewRunnableMethod(data_uploader.get(),
                           &ChromeFrameMetricsDataUploader::UploadData,
-                          upload_data));
+                          upload_data, io_loop));
     return S_OK;
   }
 
-  void UploadData(const std::string& upload_data) {
+  void UploadData(const std::string& upload_data, MessageLoop* message_loop) {
     DCHECK(fetcher_ == NULL);
+    DCHECK(message_loop != NULL);
 
     BrowserDistribution* dist = ChromeFrameDistribution::GetDistribution();
     DCHECK(dist != NULL);
@@ -271,7 +315,7 @@ class ChromeFrameMetricsDataUploader
                               URLFetcher::POST, this);
 
     fetcher_->set_request_context(new ChromeFrameUploadRequestContextGetter(
-        upload_thread_.message_loop()));
+        message_loop));
     fetcher_->set_upload_data(kMetricsType, upload_data);
     fetcher_->Start();
   }
@@ -296,17 +340,13 @@ class ChromeFrameMetricsDataUploader
   }
 
  private:
-  base::Thread upload_thread_;
   URLFetcher* fetcher_;
   PlatformThreadId creator_thread_id_;
 };
 
 MetricsService* MetricsService::GetInstance() {
-  if (g_metrics_instance_.Pointer()->Get())
-    return g_metrics_instance_.Pointer()->Get();
-
-  g_metrics_instance_.Pointer()->Set(new MetricsService);
-  return g_metrics_instance_.Pointer()->Get();
+  AutoLock lock(g_metrics_service_lock);
+  return &g_metrics_instance_.Get();
 }
 
 MetricsService::MetricsService()
@@ -343,6 +383,12 @@ void MetricsService::InitializeMetricsState() {
 
   // Ensure that an instance of the StatisticsRecorder object is created.
   g_statistics_recorder_.Get();
+
+  if (user_permits_upload_) {
+    // Ensure that an instance of the metrics upload thread is created.
+    g_metrics_upload_thread_.Get();
+  }
+
   CrashMetricsReporter::GetInstance()->set_active(true);
 }
 
@@ -363,7 +409,6 @@ void MetricsService::Stop() {
 }
 
 void MetricsService::SetRecording(bool enabled) {
-  DCHECK_EQ(thread_, PlatformThread::CurrentId());
   if (enabled == recording_active_)
     return;
 
diff --git a/chrome_frame/metrics_service.h b/chrome_frame/metrics_service.h
index dd71483..252f716 100644
--- a/chrome_frame/metrics_service.h
+++ b/chrome_frame/metrics_service.h
@@ -35,6 +35,9 @@ class MetricsService : public MetricsServiceBase {
   void InitializeMetricsState();
 
  private:
+  // To enable the default traits object to create an instance of this class.
+  friend struct base::DefaultLazyInstanceTraits<MetricsService>;
+
   MetricsService();
   virtual ~MetricsService();
   // The MetricsService has a lifecycle that is stored as a state.
@@ -135,9 +138,6 @@ class MetricsService : public MetricsServiceBase {
   // A number that identifies the how many times the app has been launched.
   int session_id_;
 
-  static base::LazyInstance<base::ThreadLocalPointer<MetricsService> >
-      g_metrics_instance_;
-
   PlatformThreadId thread_;
 
   // Indicates if this is the first uma upload from this instance.