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

chrome_frame/metrics_service.cc

Index: chrome_frame/metrics_service.cc
===================================================================
--- chrome_frame/metrics_service.cc	(revision 69412)
+++ chrome_frame/metrics_service.cc	(working copy)
@@ -413,6 +413,9 @@
 
   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) {