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

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

BUG=64388
TEST=none



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=69516

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

Added ananta, because of the TODO comment I refer to in my comments below...

http://codereview.chromium.org/5845004/diff/1/chrome_frame/metrics_service.cc
File chrome_frame/metrics_service.cc (right):

http://codereview.chromium.org/5845004/diff/1/chrome_frame/metrics_service.cc...
chrome_frame/metrics_service.cc:418: g_metrics_upload_thread_.Get().Stop();
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?

[ananta, 2010-12-16 19:58:00]

The change itself LGTM. However currently we don't call MetricsService::Stop as
we don't have a good enough heuristic of when to call it.

[Vitaly Buka (NO REVIEWS), 2010-12-16 20:40:30]

On 2010/12/16 19:58:00, ananta wrote:
> The change itself LGTM. However currently we don't call MetricsService::Stop
as
> we don't have a good enough heuristic of when to call it.

Actually this if fix for CEEE. You can find place where to call Stop in future.

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

http://codereview.chromium.org/5845004/diff/1/chrome_frame/metrics_service.cc
File chrome_frame/metrics_service.cc (right):

http://codereview.chromium.org/5845004/diff/1/chrome_frame/metrics_service.cc...
chrome_frame/metrics_service.cc:418: g_metrics_upload_thread_.Get().Stop();
On 2010/12/16 19:51:59, MAD wrote:
> 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

Done.

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

SetReporting does this check always.

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

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:36]

LGTM...

Thanks!

BYE
MAD

http://codereview.chromium.org/5845004/diff/1/chrome_frame/metrics_service.cc
File chrome_frame/metrics_service.cc (right):

http://codereview.chromium.org/5845004/diff/1/chrome_frame/metrics_service.cc...
chrome_frame/metrics_service.cc:418: g_metrics_upload_thread_.Get().Stop();
On 2010/12/16 20:40:36, Vitaly Buka wrote:
> On 2010/12/16 19:51:59, MAD wrote:
> > 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
> 
> Done.
> 
> > thread. 
> > 
> > Also, maybe we could add a DCHECK that we are running in the appropriate
> > thread...
> > 
> 
> SetReporting does this check always.
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... 
> 
> > 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?
> 
> 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);)
>