Make sure the timer is stopped and destroyed on the thread it was created on.

Make sure the elevated controller release timer is stopped and destroyed on the thread it was created on.

BUG=125364
TEST=manual


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

[alexeypa (please no reviews), 2012-05-01 16:54:03]

PTAL

[Sergey Ulanov, 2012-05-01 17:26:04]

LGTM with one suggestion that you can ignore.

https://chromiumcodereview.appspot.com/10272029/diff/1/remoting/host/plugin/d...
File remoting/host/plugin/daemon_controller_win.cc (right):

https://chromiumcodereview.appspot.com/10272029/diff/1/remoting/host/plugin/d...
remoting/host/plugin/daemon_controller_win.cc:258: release_timer_.reset(new
base::OneShotTimer<DaemonControllerWin>());
Maybe use DelayTimer here and reset it ever time this method is called (even
when the controller_ already exists)?
That will mean that we will drop it 15 minutes after the last use instead of 15
minutes after it is created.

[simonmorris, 2012-05-01 17:29:11]

http://codereview.chromium.org/10272029/diff/1/remoting/host/plugin/daemon_co...
File remoting/host/plugin/daemon_controller_win.cc (right):

http://codereview.chromium.org/10272029/diff/1/remoting/host/plugin/daemon_co...
remoting/host/plugin/daemon_controller_win.cc:83: // Activates an elevated
instance of the controller and stores the pointer
"...and caches it." Or "stores" it?

http://codereview.chromium.org/10272029/diff/1/remoting/host/plugin/daemon_co...
remoting/host/plugin/daemon_controller_win.cc:87: // Releases the cached
instance of the evelated controller.
evelated -> elevated
cached -> stored?

http://codereview.chromium.org/10272029/diff/1/remoting/host/plugin/daemon_co...
remoting/host/plugin/daemon_controller_win.cc:167: base::Unretained(this)));
How does this guarantee that ReleaseElevatedController is run before the rest of
DaemonControllerWin's destructor destructs release_timer? Or before the next
line stops the worker thread?

[alexeypa (please no reviews), 2012-05-01 17:32:30]

FYI

https://chromiumcodereview.appspot.com/10272029/diff/1/remoting/host/plugin/d...
File remoting/host/plugin/daemon_controller_win.cc (right):

https://chromiumcodereview.appspot.com/10272029/diff/1/remoting/host/plugin/d...
remoting/host/plugin/daemon_controller_win.cc:258: release_timer_.reset(new
base::OneShotTimer<DaemonControllerWin>());
On 2012/05/01 17:26:05, sergeyu wrote:
> That will mean that we will drop it 15 minutes after the last use instead of
15
> minutes after it is created.

I intentionally implemented it this way. I.e. it is supposed to re-prompt UAC 15
minutes after the _first_ use even if a user keeps clicking the button. It 
feels a little bit safer that the timeout is fixed.

[commit-bot: I haz the power, 2012-05-01 17:32:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/10272029/1

[alexeypa (please no reviews), 2012-05-01 18:00:01]

FYI. Simon, please lgtm if you are ok with the change. I've stopped CQ for now.

http://codereview.chromium.org/10272029/diff/1/remoting/host/plugin/daemon_co...
File remoting/host/plugin/daemon_controller_win.cc (right):

http://codereview.chromium.org/10272029/diff/1/remoting/host/plugin/daemon_co...
remoting/host/plugin/daemon_controller_win.cc:83: // Activates an elevated
instance of the controller and stores the pointer
On 2012/05/01 17:29:12, simonmorris wrote:
> "...and caches it." Or "stores" it?

Done.

http://codereview.chromium.org/10272029/diff/1/remoting/host/plugin/daemon_co...
remoting/host/plugin/daemon_controller_win.cc:87: // Releases the cached
instance of the evelated controller.
On 2012/05/01 17:29:12, simonmorris wrote:
> evelated -> elevated
> cached -> stored?

Done.

http://codereview.chromium.org/10272029/diff/1/remoting/host/plugin/daemon_co...
remoting/host/plugin/daemon_controller_win.cc:167: base::Unretained(this)));
On 2012/05/01 17:29:12, simonmorris wrote:
> How does this guarantee that ReleaseElevatedController is run before the rest
of
> DaemonControllerWin's destructor destructs release_timer? Or before the next
> line stops the worker thread?

worker_thread_.Stop() waits synchronously until all pending tasks were processed
by the thread.

[simonmorris, 2012-05-01 18:05:14]

On 2012/05/01 18:00:01, alexeypa wrote:
> FYI. Simon, please lgtm if you are ok with the change. I've stopped CQ for
now.
> 
>
http://codereview.chromium.org/10272029/diff/1/remoting/host/plugin/daemon_co...
> File remoting/host/plugin/daemon_controller_win.cc (right):
> 
>
http://codereview.chromium.org/10272029/diff/1/remoting/host/plugin/daemon_co...
> remoting/host/plugin/daemon_controller_win.cc:83: // Activates an elevated
> instance of the controller and stores the pointer
> On 2012/05/01 17:29:12, simonmorris wrote:
> > "...and caches it." Or "stores" it?
> 
> Done.
> 
>
http://codereview.chromium.org/10272029/diff/1/remoting/host/plugin/daemon_co...
> remoting/host/plugin/daemon_controller_win.cc:87: // Releases the cached
> instance of the evelated controller.
> On 2012/05/01 17:29:12, simonmorris wrote:
> > evelated -> elevated
> > cached -> stored?
> 
> Done.
> 
>
http://codereview.chromium.org/10272029/diff/1/remoting/host/plugin/daemon_co...
> remoting/host/plugin/daemon_controller_win.cc:167: base::Unretained(this)));
> On 2012/05/01 17:29:12, simonmorris wrote:
> > How does this guarantee that ReleaseElevatedController is run before the
rest
> of
> > DaemonControllerWin's destructor destructs release_timer? Or before the next
> > line stops the worker thread?
> 
> worker_thread_.Stop() waits synchronously until all pending tasks were
processed
> by the thread.

lgtm

[commit-bot: I haz the power, 2012-05-01 18:08:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/10272029/8003

[commit-bot: I haz the power, 2012-05-01 20:30:00]

Change committed as 134760