Fix crashes in ChromotingHost

Fix crashes in ChromotingHost

Capturer should be owned by ScreenRecoder. This patch also addes test to test
reconnection of ChromotingHost.

BUG=69969
TEST=ChromotingHostTest.*

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

[Sergey Ulanov, 2011-01-20 19:51:26]

Mostly nits.

http://codereview.chromium.org/6266010/diff/1/remoting/host/chromoting_host.cc
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/6266010/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:192: recorder_->Stop(NULL);
Why don't we pass shutdown_task_ after the client is disconnected?

http://codereview.chromium.org/6266010/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:213: make_scoped_refptr(connection)));
We don't really need this argument in OnClientConnected, the connection is
stored in connection_. Add DCHECK_EQ(connection, connection_), and remove
connection argument from OnClientConnected()?

http://codereview.chromium.org/6266010/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:223: make_scoped_refptr(connection)));
same here

http://codereview.chromium.org/6266010/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:233: make_scoped_refptr(connection)));
and here

http://codereview.chromium.org/6266010/diff/1/remoting/host/chromoting_host.h
File remoting/host/chromoting_host.h (right):

http://codereview.chromium.org/6266010/diff/1/remoting/host/chromoting_host.h...
remoting/host/chromoting_host.h:68: // instances.  Default capturer is used if
it is not specified.
update comment: Default capturer and input stub are used...

[Alpha Left Google, 2011-01-20 20:04:31]

http://codereview.chromium.org/6266010/diff/1/remoting/host/chromoting_host.cc
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/6266010/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:192: recorder_->Stop(NULL);
On 2011/01/20 19:51:26, sergeyu wrote:
> Why don't we pass shutdown_task_ after the client is disconnected?

shutdown_task_ is used only if Shutdown() is called by the client or
ChromotingHost fails to connect to talk network which is catastrophic failure.
In case of user disconnect we don't want to shutdown everything.

The use of shutdown task is to wake up the main message loop to stop all
chromoting threads.

http://codereview.chromium.org/6266010/diff/1/remoting/host/chromoting_host.c...
remoting/host/chromoting_host.cc:213: make_scoped_refptr(connection)));
On 2011/01/20 19:51:26, sergeyu wrote:
> We don't really need this argument in OnClientConnected, the connection is
> stored in connection_. Add DCHECK_EQ(connection, connection_), and remove
> connection argument from OnClientConnected()?

|connection| is more correct because it's referring to the current connection,
|connection_| was a bad usage since it assumes both variables are the same, this
will not hold when there's more than one connection.

http://codereview.chromium.org/6266010/diff/1/remoting/host/chromoting_host.h
File remoting/host/chromoting_host.h (right):

http://codereview.chromium.org/6266010/diff/1/remoting/host/chromoting_host.h...
remoting/host/chromoting_host.h:68: // instances.  Default capturer is used if
it is not specified.
On 2011/01/20 19:51:26, sergeyu wrote:
> update comment: Default capturer and input stub are used...

Done.

[Sergey Ulanov, 2011-01-20 23:06:52]

LGTM

[Timur Iskhodzhanov, 2011-01-26 02:59:06]

This change has made all three TSan bots red.
Can you please take a look at the bots?
Also, there are leaks on all Valgrind bots running remoting_unittests.

Alpha, Sergey, do you run your tests under Valgrind & TSan before commit or do
you watch the memory waterfall status after?

[Sergey Ulanov, 2011-01-26 03:10:08]

I did run valgrind locally, it didn't catch the leak (not sure why). Already
have a fix for it (http://codereview.chromium.org/6266020/).
I will take a look at the TSan errors.

On Tue, Jan 25, 2011 at 6:59 PM, <timurrrr@chromium.org> wrote:

> This change has made all three TSan bots red.
> Can you please take a look at the bots?
> Also, there are leaks on all Valgrind bots running remoting_unittests.
>
> Alpha, Sergey, do you run your tests under Valgrind & TSan before commit or
> do
> you watch the memory waterfall status after?
>
>
> http://codereview.chromium.org/6266010/
>

[hclam, 2011-01-26 03:10:43]

Sorry! But I do run valgrind and tsan when doing those changes..

2011年1月25日18:59 <timurrrr@chromium.org>:

> This change has made all three TSan bots red.
> Can you please take a look at the bots?
> Also, there are leaks on all Valgrind bots running remoting_unittests.
>
> Alpha, Sergey, do you run your tests under Valgrind & TSan before commit or
> do
> you watch the memory waterfall status after?
>
>
> http://codereview.chromium.org/6266010/
>

[Sergey Ulanov, 2011-01-26 03:19:59]

On Tue, Jan 25, 2011 at 7:09 PM, Sergey Ulanov <sergeyu@chromium.org> wrote:

> I did run valgrind locally, it didn't catch the leak (not sure why).
> Already have a fix for it (http://codereview.chromium.org/6266020/).

Oh, sorry, that fix was for a different issue.


> I will take a look at the TSan errors.
>
>
> On Tue, Jan 25, 2011 at 6:59 PM, <timurrrr@chromium.org> wrote:
>
>> This change has made all three TSan bots red.
>> Can you please take a look at the bots?
>> Also, there are leaks on all Valgrind bots running remoting_unittests.
>>
>> Alpha, Sergey, do you run your tests under Valgrind & TSan before commit
>> or do
>> you watch the memory waterfall status after?
>>
>>
>> http://codereview.chromium.org/6266010/
>>
>
>

[Nico, 2011-01-26 18:55:51]

Did this get resolved?

On Tue, Jan 25, 2011 at 6:59 PM,  <timurrrr@chromium.org> wrote:
> This change has made all three TSan bots red.
> Can you please take a look at the bots?
> Also, there are leaks on all Valgrind bots running remoting_unittests.
>
> Alpha, Sergey, do you run your tests under Valgrind & TSan before commit or
> do
> you watch the memory waterfall status after?
>
> http://codereview.chromium.org/6266010/
>

[hclam, 2011-01-26 19:09:15]

Hi Nico,

I'm working on it, will submit a fix today.

Alpha

2011年1月26日10:55 Nico Weber <thakis@chromium.org>:

> Did this get resolved?
>
> On Tue, Jan 25, 2011 at 6:59 PM,  <timurrrr@chromium.org> wrote:
> > This change has made all three TSan bots red.
> > Can you please take a look at the bots?
> > Also, there are leaks on all Valgrind bots running remoting_unittests.
> >
> > Alpha, Sergey, do you run your tests under Valgrind & TSan before commit
> or
> > do
> > you watch the memory waterfall status after?
> >
> > http://codereview.chromium.org/6266010/
> >
>

[Nico, 2011-01-26 20:13:46]

hclam will revert this until a fix is found.

On Wed, Jan 26, 2011 at 11:09 AM, Alpha (Hin-Chung) Lam
<hclam@google.com> wrote:
> Hi Nico,
> I'm working on it, will submit a fix today.
> Alpha
>
> 2011年1月26日10:55 Nico Weber <thakis@chromium.org>:
>>
>> Did this get resolved?
>>
>> On Tue, Jan 25, 2011 at 6:59 PM,  <timurrrr@chromium.org> wrote:
>> > This change has made all three TSan bots red.
>> > Can you please take a look at the bots?
>> > Also, there are leaks on all Valgrind bots running remoting_unittests.
>> >
>> > Alpha, Sergey, do you run your tests under Valgrind & TSan before commit
>> > or
>> > do
>> > you watch the memory waterfall status after?
>> >
>> > http://codereview.chromium.org/6266010/
>> >
>
>