Replace ScopedThreadProxy with MessageLoopProxy & WeakPtrs.

Replace ScopedThreadProxy with MessageLoopProxy & WeakPtrs.

This affects the following classes:
* ChromotingClient
* ChromotingInstance
* HostUserInterface
* It2MeHostUserInterface

The MessageLoopProxy/WeakPtr combination requires that the WeakPtr is created on the thread referred to by the proxy; code in which that is hard to arrange usually has subtle race-conditions.

TEST=Existing unit-tests, and manual testing.


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

[Sergey Ulanov, 2012-05-29 18:58:29]

http://codereview.chromium.org/10454040/diff/1/remoting/client/plugin/chromot...
File remoting/client/plugin/chromoting_instance.cc (right):

http://codereview.chromium.org/10454040/diff/1/remoting/client/plugin/chromot...
remoting/client/plugin/chromoting_instance.cc:100: static
scoped_refptr<base::MessageLoopProxy> g_logging_message_loop;
This adds a static finalizer. Use CR_DEFINE_STATIC_LOCAL() here.

http://codereview.chromium.org/10454040/diff/1/remoting/client/plugin/chromot...
remoting/client/plugin/chromoting_instance.cc:100: static
scoped_refptr<base::MessageLoopProxy> g_logging_message_loop;
SingleThreadTaskRunner please.

http://codereview.chromium.org/10454040/diff/1/remoting/client/plugin/chromot...
remoting/client/plugin/chromoting_instance.cc:101: static
base::WeakPtr<ChromotingInstance> g_logging_instance;
This adds a static finalizer. Use CR_DEFINE_STATIC_LOCAL() to avoid it.

http://codereview.chromium.org/10454040/diff/1/remoting/client/plugin/chromot...
remoting/client/plugin/chromoting_instance.cc:661:
(!g_logging_message_loop->BelongsToCurrentThread() ||
Races are still possible in this code. The logging thread may reset
|g_logging_message_loop| after we verify g_logging_message_loop.get() and this
line. One way to solve it is to copy refptr to the stack and then use the copy
here. Or maybe add a lock, then |g_logging_message_loop| wouldn't have to be
global.

http://codereview.chromium.org/10454040/diff/1/remoting/host/host_user_interf...
File remoting/host/host_user_interface.cc (right):

http://codereview.chromium.org/10454040/diff/1/remoting/host/host_user_interf...
remoting/host/host_user_interface.cc:80: // TODO(wez): This isn't safe, since
the callback gets called on UI thread.
I think we can just remove this line. The disconnection callback handler should
be able to handle the case when the host has been shut down already.

http://codereview.chromium.org/10454040/diff/1/remoting/host/host_user_interf...
remoting/host/host_user_interface.cc:89: DisconnectSession();
Do we need DisconnectSession() method at all?

http://codereview.chromium.org/10454040/diff/1/remoting/host/it2me_host_user_...
File remoting/host/it2me_host_user_interface.cc (right):

http://codereview.chromium.org/10454040/diff/1/remoting/host/it2me_host_user_...
remoting/host/it2me_host_user_interface.cc:106:
ui_message_loop()->PostDelayedTask(
Now that we have ThreadTaskRunnerHandle, we can use base::Timer on UI thread -
maybe better to do that here.

http://codereview.chromium.org/10454040/diff/1/remoting/host/it2me_host_user_...
File remoting/host/it2me_host_user_interface.h (right):

http://codereview.chromium.org/10454040/diff/1/remoting/host/it2me_host_user_...
remoting/host/it2me_host_user_interface.h:73:
base::WeakPtrFactory<It2MeHostUserInterface> weak_factory_;
maybe call it timer_weak_factory_ to indicate that it should not be used as a
regular weak factory.

[Wez, 2012-05-30 00:18:39]

http://codereview.chromium.org/10454040/diff/1/remoting/client/plugin/chromot...
File remoting/client/plugin/chromoting_instance.cc (right):

http://codereview.chromium.org/10454040/diff/1/remoting/client/plugin/chromot...
remoting/client/plugin/chromoting_instance.cc:100: static
scoped_refptr<base::MessageLoopProxy> g_logging_message_loop;
On 2012/05/29 18:58:30, sergeyu wrote:
> This adds a static finalizer. Use CR_DEFINE_STATIC_LOCAL() here.

Done.

http://codereview.chromium.org/10454040/diff/1/remoting/client/plugin/chromot...
remoting/client/plugin/chromoting_instance.cc:100: static
scoped_refptr<base::MessageLoopProxy> g_logging_message_loop;
On 2012/05/29 18:58:30, sergeyu wrote:
> SingleThreadTaskRunner please.

Done.

http://codereview.chromium.org/10454040/diff/1/remoting/client/plugin/chromot...
remoting/client/plugin/chromoting_instance.cc:101: static
base::WeakPtr<ChromotingInstance> g_logging_instance;
On 2012/05/29 18:58:30, sergeyu wrote:
> This adds a static finalizer. Use CR_DEFINE_STATIC_LOCAL() to avoid it.

Done.

http://codereview.chromium.org/10454040/diff/1/remoting/client/plugin/chromot...
remoting/client/plugin/chromoting_instance.cc:661:
(!g_logging_message_loop->BelongsToCurrentThread() ||
On 2012/05/29 18:58:30, sergeyu wrote:
> Races are still possible in this code. The logging thread may reset
> |g_logging_message_loop| after we verify g_logging_message_loop.get() and this
> line. One way to solve it is to copy refptr to the stack and then use the copy
> here. Or maybe add a lock, then |g_logging_message_loop| wouldn't have to be
> global.

There is already a lock held at this point (g_logging_lock).  It would make
sense to grab the task runner reference in the lock and log outside it, though.

http://codereview.chromium.org/10454040/diff/1/remoting/host/host_user_interf...
File remoting/host/host_user_interface.cc (right):

http://codereview.chromium.org/10454040/diff/1/remoting/host/host_user_interf...
remoting/host/host_user_interface.cc:80: // TODO(wez): This isn't safe, since
the callback gets called on UI thread.
On 2012/05/29 18:58:30, sergeyu wrote:
> I think we can just remove this line. The disconnection callback handler
should
> be able to handle the case when the host has been shut down already.

Done.  Not sure whether the intent was that Start() could be called to re-start
the interface.

http://codereview.chromium.org/10454040/diff/1/remoting/host/host_user_interf...
remoting/host/host_user_interface.cc:89: DisconnectSession();
On 2012/05/29 18:58:30, sergeyu wrote:
> Do we need DisconnectSession() method at all?

It's used by It2MeHostUserInterface when the Disconnect button is pressed, or
the hot-key activated.

http://codereview.chromium.org/10454040/diff/1/remoting/host/it2me_host_user_...
File remoting/host/it2me_host_user_interface.cc (right):

http://codereview.chromium.org/10454040/diff/1/remoting/host/it2me_host_user_...
remoting/host/it2me_host_user_interface.cc:106:
ui_message_loop()->PostDelayedTask(
On 2012/05/29 18:58:30, sergeyu wrote:
> Now that we have ThreadTaskRunnerHandle, we can use base::Timer on UI thread -
> maybe better to do that here.

The NPAPI plugin doesn't have ThreadTaskRunnerHandle for its UI thread, though,
so I don't think that'll work yet.

http://codereview.chromium.org/10454040/diff/1/remoting/host/it2me_host_user_...
File remoting/host/it2me_host_user_interface.h (right):

http://codereview.chromium.org/10454040/diff/1/remoting/host/it2me_host_user_...
remoting/host/it2me_host_user_interface.h:73:
base::WeakPtrFactory<It2MeHostUserInterface> weak_factory_;
On 2012/05/29 18:58:30, sergeyu wrote:
> maybe call it timer_weak_factory_ to indicate that it should not be used as a
> regular weak factory.

Done.

[Sergey Ulanov, 2012-05-30 00:30:36]

LGTM

http://codereview.chromium.org/10454040/diff/7002/remoting/client/plugin/chro...
File remoting/client/plugin/chromoting_instance.cc (right):

http://codereview.chromium.org/10454040/diff/7002/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.cc:102:
CR_DEFINE_STATIC_LOCAL(base::WeakPtr<ChromotingInstance>, g_logging_instance,
nit: Might be more readable if formatted as follows:
CR_DEFINE_STATIC_LOCAL(
    base::WeakPtr<ChromotingInstance>, g_logging_instance, ());

http://codereview.chromium.org/10454040/diff/7002/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.cc:165: //thread_proxy_->Detach();
Did you mean to make this part for this CL?
I'm not sure this is always safe. The thread proxy may outlive this object, but
the delegate that is passed to it is destroyed shortly after this destructor
returns.

[Wez, 2012-05-30 00:54:02]

http://codereview.chromium.org/10454040/diff/7002/remoting/client/plugin/chro...
File remoting/client/plugin/chromoting_instance.cc (right):

http://codereview.chromium.org/10454040/diff/7002/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.cc:102:
CR_DEFINE_STATIC_LOCAL(base::WeakPtr<ChromotingInstance>, g_logging_instance,
On 2012/05/30 00:30:37, sergeyu wrote:
> nit: Might be more readable if formatted as follows:
> CR_DEFINE_STATIC_LOCAL(
>     base::WeakPtr<ChromotingInstance>, g_logging_instance, ());

Done.

http://codereview.chromium.org/10454040/diff/7002/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.cc:165: //thread_proxy_->Detach();
On 2012/05/30 00:30:37, sergeyu wrote:
> Did you mean to make this part for this CL?
> I'm not sure this is always safe. The thread proxy may outlive this object,
but
> the delegate that is passed to it is destroyed shortly after this destructor
> returns.

We now identify the ChromotingInstance to direct logging to via a WeakPtr, so
once the WeakFactory is deleted, any queued events become no-ops - there's no
need to explicitly invalidate/detach.

http://codereview.chromium.org/10454040/diff/7002/remoting/client/plugin/chro...
remoting/client/plugin/chromoting_instance.cc:660: // case we
Gah, uploaded mid-edit!

[commit-bot: I haz the power, 2012-05-30 00:54:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/10454040/8013

[commit-bot: I haz the power, 2012-05-30 03:16:13]

Try job failure for 10454040-8013 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-30 17:16:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/10454040/8013

[commit-bot: I haz the power, 2012-05-30 20:52:14]

Change committed as 139623