Stop upload thread when stopping service. Leaked thread may crash process on...

chrome_frame/metrics_service.cc

 // Copyright (c) 2010 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.
 
 
 //------------------------------------------------------------------------------
 // Description of the life cycle of a instance of MetricsService.
 //
 //  OVERVIEW
 //
@@ skipping 395 matching lines @@
   GetInstance()->SetRecording(true);
   GetInstance()->SetReporting(true);
 }
 
 // static
 void MetricsService::Stop() {
   AutoLock lock(metrics_service_lock_);
 
   GetInstance()->SetReporting(false);
   GetInstance()->SetRecording(false);
+
+  if (GetInstance()->user_permits_upload_)
+    g_metrics_upload_thread_.Get().Stop();

[MAD, 2010/12/16 19:51:59]

Is this in adherence with the following comment around line 120:
// 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.

If so, maybe we could update the comment, now that we have a way to stop this
thread. 

Also, maybe we could add a DCHECK that we are running in the appropriate
thread...

And finally, is it safe to stop this thread from within a metrics_service_lock_?
If we ever add code to the CleanUp method of the thread that needs to acquire
this lock, I think we will deadlock, right?

[Vitaly Buka (NO REVIEWS), 2010/12/16 20:40:36]

Done.
SetReporting does this check always.
I guess it safe now, and future cleanups should not call code locking this
object.

However moving outside lock is safe too if consider that multiple Start()
unsupported. (it has DCHECK(state_ == INITIALIZED);)

[mad-corp, 2010/12/17 00:36:37]

Yeah, now, but what if it changes? And it's not obvious as we read the code here
that we are ensuring that it is executed in the same thread... 

 }
 
 void MetricsService::SetRecording(bool enabled) {
   if (enabled == recording_active_)
     return;
 
   if (enabled) {
     if (client_id_.empty()) {
       client_id_ = GenerateClientID();
       // Save client id somewhere.
@@ skipping 178 matching lines @@
     version += "-F";
     if (!version_info.IsOfficialBuild())
       version.append("-devel");
     return version;
   } else {
     NOTREACHED() << "Unable to retrieve version string.";
   }
 
   return std::string();
 }