Delete Session and SessionManager object synchronously.

Delete Session and SessionManager object synchronously.

JingleSession and JingleSessionManager were previously deleted
asychronously, but it is not necessary anymore arter we've switched to
PepperSession and PepperSessionManager. Instead changed 
LibjingleTransportFactory to delete libjingle objects asynchronously.
Also fixed some bugs related that would cause crash when channel auth 
fails.

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

[Sergey Ulanov, 2012-02-22 22:04:14]

Also extended the tests to verify that we shutdown properly when channel
creation (e.g. authentication) fails (these would catch bugs fixed in this CL).

[Wez, 2012-02-22 22:51:38]

http://codereview.chromium.org/9433027/diff/3001/jingle/glue/pseudotcp_adapter.h
File jingle/glue/pseudotcp_adapter.h (right):

http://codereview.chromium.org/9433027/diff/3001/jingle/glue/pseudotcp_adapte...
jingle/glue/pseudotcp_adapter.h:32: // Sets callback that should be called when
the adapter is being
nit: Suggest "Sets a callback to be notified when ..."

http://codereview.chromium.org/9433027/diff/4007/jingle/glue/pseudotcp_adapter.h
File jingle/glue/pseudotcp_adapter.h (right):

http://codereview.chromium.org/9433027/diff/4007/jingle/glue/pseudotcp_adapte...
jingle/glue/pseudotcp_adapter.h:35: void SetOnDestroyedCallback(const
base::Closure& callback);
We only need this because the Transport implementation that created the adapter
needs to out-live it, right?  This arrangement seems unfortunate.

nit: The comment mentions destroying the TransportChannel, which is a
remoting/protocol/ concept, so either clarify that or don't refer to it?

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/fake_authe...
File remoting/protocol/fake_authenticator.cc (right):

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/fake_authe...
remoting/protocol/fake_authenticator.cc:38: error_ = net::ERR_FAILED;
nit: Now that |error_| is a member, you could rename it to |result_| and get rid
of |accept_|.

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/fake_authe...
remoting/protocol/fake_authenticator.cc:48: base::Unretained(this)));
Why not continue to use |weak_factory_| here?

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/fake_authe...
remoting/protocol/fake_authenticator.cc:50: OnAuthBytesWritten(result);
nit: At this point |read_| is never true, so we can't possibly invoke the done
callback, so we're not assuming anything about its behaviour here, but that's
not obvious on a first glance.

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/fake_authe...
File remoting/protocol/fake_authenticator.h (right):

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/fake_authe...
remoting/protocol/fake_authenticator.h:42: bool written_;
nit: did_read_bytes_, did_write_bytes_?

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/libjingle_...
File remoting/protocol/libjingle_transport_factory.cc (right):

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/libjingle_...
remoting/protocol/libjingle_transport_factory.cc:299: if (channel_.get()) {
Comment to clarify that we can't delete |channel_| directly because we're in the
context of a sigslot signal.

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/libjingle_...
remoting/protocol/libjingle_transport_factory.cc:301: FROM_HERE,
channel_.release());
Isn't there a risk that we'll end up processing some other libjingle signal on
|channel_| before it gets deleted?

Would it be cleaner to disconnect the signal-handlers from |channel_| at this
point?  Or is that not possible from within the context of a sigslot call-stack,
either?

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/libjingle_...
remoting/protocol/libjingle_transport_factory.cc:303: if (port_allocator_.get())
{
We might end up here due to a |channel_| signal triggered by a |port_allocator_|
signal?

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/libjingle_...
remoting/protocol/libjingle_transport_factory.cc:322:
base::MessageLoopProxy::current()->DeleteSoon(
Is the worry that LibjingleTransportFactory might be deleted from within a
sigslot handler?

[Sergey Ulanov, 2012-02-22 23:36:32]

http://codereview.chromium.org/9433027/diff/4007/jingle/glue/pseudotcp_adapter.h
File jingle/glue/pseudotcp_adapter.h (right):

http://codereview.chromium.org/9433027/diff/4007/jingle/glue/pseudotcp_adapte...
jingle/glue/pseudotcp_adapter.h:35: void SetOnDestroyedCallback(const
base::Closure& callback);
On 2012/02/22 22:51:38, Wez wrote:
> We only need this because the Transport implementation that created the
adapter
> needs to out-live it, right?  This arrangement seems unfortunate.

No. The problem is that PseudoTcpAdapter::Core may outlive PseudoTcpAdapter, and
so it would delete the underlying channel adapter (Core::socket_) after the
PseudoTcpAdapter is deleted. In some cases this was causing
LibjingleStreamTransport::OnChannelDestroyed() to be called after it is
destroyed.

> 
> nit: The comment mentions destroying the TransportChannel, which is a
> remoting/protocol/ concept, so either clarify that or don't refer to it?

Done

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/fake_authe...
File remoting/protocol/fake_authenticator.cc (right):

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/fake_authe...
remoting/protocol/fake_authenticator.cc:38: error_ = net::ERR_FAILED;
On 2012/02/22 22:51:38, Wez wrote:
> nit: Now that |error_| is a member, you could rename it to |result_| and get
rid
> of |accept_|.

Done.

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/fake_authe...
remoting/protocol/fake_authenticator.cc:48: base::Unretained(this)));
On 2012/02/22 22:51:38, Wez wrote:
> Why not continue to use |weak_factory_| here?

It is not necessary because socket_ will be deleted before this object unless
these calls are complete. Using weak pointers would require making this class
SupportsWeakPtr<>

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/fake_authe...
remoting/protocol/fake_authenticator.cc:50: OnAuthBytesWritten(result);
On 2012/02/22 22:51:38, Wez wrote:
> nit: At this point |read_| is never true, so we can't possibly invoke the done
> callback, so we're not assuming anything about its behaviour here, but that's
> not obvious on a first glance.

Added comment.

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/fake_authe...
File remoting/protocol/fake_authenticator.h (right):

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/fake_authe...
remoting/protocol/fake_authenticator.h:42: bool written_;
On 2012/02/22 22:51:38, Wez wrote:
> nit: did_read_bytes_, did_write_bytes_?

Done.

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/libjingle_...
File remoting/protocol/libjingle_transport_factory.cc (right):

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/libjingle_...
remoting/protocol/libjingle_transport_factory.cc:299: if (channel_.get()) {
On 2012/02/22 22:51:38, Wez wrote:
> Comment to clarify that we can't delete |channel_| directly because we're in
the
> context of a sigslot signal.

Done.

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/libjingle_...
remoting/protocol/libjingle_transport_factory.cc:301: FROM_HERE,
channel_.release());
On 2012/02/22 22:51:38, Wez wrote:
> Isn't there a risk that we'll end up processing some other libjingle signal on
> |channel_| before it gets deleted?
> 
> Would it be cleaner to disconnect the signal-handlers from |channel_| at this
> point?  Or is that not possible from within the context of a sigslot
call-stack,
> either?

sigslot::has_slots<> disconnects signals automatically and signal handlers
inherit it.

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/libjingle_...
remoting/protocol/libjingle_transport_factory.cc:303: if (port_allocator_.get())
{
On 2012/02/22 22:51:38, Wez wrote:
> We might end up here due to a |channel_| signal triggered by a
|port_allocator_|
> signal?
Yes, or channel_ may try to access port_allocator_ before it is destroyed.

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/libjingle_...
remoting/protocol/libjingle_transport_factory.cc:322:
base::MessageLoopProxy::current()->DeleteSoon(
On 2012/02/22 22:51:38, Wez wrote:
> Is the worry that LibjingleTransportFactory might be deleted from within a
> sigslot handler?

Yes. Added comments

[Wez, 2012-02-23 19:33:42]

http://codereview.chromium.org/9433027/diff/4007/jingle/glue/pseudotcp_adapter.h
File jingle/glue/pseudotcp_adapter.h (right):

http://codereview.chromium.org/9433027/diff/4007/jingle/glue/pseudotcp_adapte...
jingle/glue/pseudotcp_adapter.h:35: void SetOnDestroyedCallback(const
base::Closure& callback);
On 2012/02/22 23:36:32, sergeyu wrote:
> On 2012/02/22 22:51:38, Wez wrote:
> > We only need this because the Transport implementation that created the
> adapter
> > needs to out-live it, right?  This arrangement seems unfortunate.
> 
> No. The problem is that PseudoTcpAdapter::Core may outlive PseudoTcpAdapter,
and
> so it would delete the underlying channel adapter (Core::socket_) after the
> PseudoTcpAdapter is deleted. In some cases this was causing
> LibjingleStreamTransport::OnChannelDestroyed() to be called after it is
> destroyed.

Ouch!  I wonder if we could tear down the underlying channel adapter when the
PseudoTcpAdapter is torn down, rather than the Core?

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/fake_authe...
File remoting/protocol/fake_authenticator.cc (right):

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/fake_authe...
remoting/protocol/fake_authenticator.cc:48: base::Unretained(this)));
On 2012/02/22 23:36:32, sergeyu wrote:
> On 2012/02/22 22:51:38, Wez wrote:
> > Why not continue to use |weak_factory_| here?
> 
> It is not necessary because socket_ will be deleted before this object unless
> these calls are complete. Using weak pointers would require making this class
> SupportsWeakPtr<>

The old code was using a |weak_factory_| member, so I don't think you'd need
SupportsWeakPtr<>, would you?

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/libjingle_...
File remoting/protocol/libjingle_transport_factory.cc (right):

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/libjingle_...
remoting/protocol/libjingle_transport_factory.cc:81:
jingle_glue::TransportChannelSocketAdapter* channel_adapter_;
Is this needed?

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/libjingle_...
remoting/protocol/libjingle_transport_factory.cc:301: FROM_HERE,
channel_.release());
On 2012/02/22 23:36:32, sergeyu wrote:
> On 2012/02/22 22:51:38, Wez wrote:
> > Isn't there a risk that we'll end up processing some other libjingle signal
on
> > |channel_| before it gets deleted?
> > 
> > Would it be cleaner to disconnect the signal-handlers from |channel_| at
this
> > point?  Or is that not possible from within the context of a sigslot
> call-stack,
> > either?
> 
> sigslot::has_slots<> disconnects signals automatically and signal handlers
> inherit it.

Right, but it does that when it's deleted, i.e. when LibjingleStreamTransport is
deleted in this case, which isn't something this function (explicitly) does.

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/pepper_ses...
File remoting/protocol/pepper_session_unittest.cc (right):

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session_unittest.cc:243:
EXPECT_TRUE(!client_socket_.get() || host_socket_.get());
Do you mean !(client_socket_.get() || host_socket_.get()) here?

Given that you've written the expect success case as two EXPECT_TRUE()s you
could just have these be EXPECT_FALSE()?

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session_unittest.cc:363: // Verify that data can sent
over stream channel.
typo: ... can be sent ...

http://codereview.chromium.org/9433027/diff/5002/remoting/protocol/fake_authe...
File remoting/protocol/fake_authenticator.cc (right):

http://codereview.chromium.org/9433027/diff/5002/remoting/protocol/fake_authe...
remoting/protocol/fake_authenticator.cc:43: // This will cannot call the
callback because |did_read_bytes_|
typo: will cannot

[Sergey Ulanov, 2012-02-23 22:10:06]

http://codereview.chromium.org/9433027/diff/4007/jingle/glue/pseudotcp_adapter.h
File jingle/glue/pseudotcp_adapter.h (right):

http://codereview.chromium.org/9433027/diff/4007/jingle/glue/pseudotcp_adapte...
jingle/glue/pseudotcp_adapter.h:35: void SetOnDestroyedCallback(const
base::Closure& callback);
On 2012/02/23 19:33:42, Wez wrote:
> On 2012/02/22 23:36:32, sergeyu wrote:
> > On 2012/02/22 22:51:38, Wez wrote:
> > > We only need this because the Transport implementation that created the
> > adapter
> > > needs to out-live it, right?  This arrangement seems unfortunate.
> > 
> > No. The problem is that PseudoTcpAdapter::Core may outlive PseudoTcpAdapter,
> and
> > so it would delete the underlying channel adapter (Core::socket_) after the
> > PseudoTcpAdapter is deleted. In some cases this was causing
> > LibjingleStreamTransport::OnChannelDestroyed() to be called after it is
> > destroyed.
> 
> Ouch!  I wonder if we could tear down the underlying channel adapter when the
> PseudoTcpAdapter is torn down, rather than the Core?

Agree, that's a better approach. Done.

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/fake_authe...
File remoting/protocol/fake_authenticator.cc (right):

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/fake_authe...
remoting/protocol/fake_authenticator.cc:48: base::Unretained(this)));
On 2012/02/23 19:33:42, Wez wrote:
> On 2012/02/22 23:36:32, sergeyu wrote:
> > On 2012/02/22 22:51:38, Wez wrote:
> > > Why not continue to use |weak_factory_| here?
> > 
> > It is not necessary because socket_ will be deleted before this object
unless
> > these calls are complete. Using weak pointers would require making this
class
> > SupportsWeakPtr<>
> 
> The old code was using a |weak_factory_| member, so I don't think you'd need
> SupportsWeakPtr<>, would you?

You are right, changed it to use weak_factory_

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/libjingle_...
File remoting/protocol/libjingle_transport_factory.cc (right):

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/libjingle_...
remoting/protocol/libjingle_transport_factory.cc:81:
jingle_glue::TransportChannelSocketAdapter* channel_adapter_;
On 2012/02/23 19:33:42, Wez wrote:
> Is this needed?

No. Removed.

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/libjingle_...
remoting/protocol/libjingle_transport_factory.cc:301: FROM_HERE,
channel_.release());
On 2012/02/23 19:33:42, Wez wrote:
> On 2012/02/22 23:36:32, sergeyu wrote:
> > On 2012/02/22 22:51:38, Wez wrote:
> > > Isn't there a risk that we'll end up processing some other libjingle
signal
> on
> > > |channel_| before it gets deleted?
> > > 
> > > Would it be cleaner to disconnect the signal-handlers from |channel_| at
> this
> > > point?  Or is that not possible from within the context of a sigslot
> > call-stack,
> > > either?
> > 
> > sigslot::has_slots<> disconnects signals automatically and signal handlers
> > inherit it.
> 
> Right, but it does that when it's deleted, i.e. when LibjingleStreamTransport
is
> deleted in this case, which isn't something this function (explicitly) does.

LibjingleStreamTransport doesn't have any signal handlers installed for the
|channel_|. The channel adapter handles signals from the |channel_|, but it
should be deleted above when we reset |socket_|.

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/pepper_ses...
File remoting/protocol/pepper_session_unittest.cc (right):

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session_unittest.cc:243:
EXPECT_TRUE(!client_socket_.get() || host_socket_.get());
On 2012/02/23 19:33:42, Wez wrote:
> Do you mean !(client_socket_.get() || host_socket_.get()) here?
> 
> Given that you've written the expect success case as two EXPECT_TRUE()s you
> could just have these be EXPECT_FALSE()?

I meant (!client_socket_.get()) || (!host_socket_.get()). Fixed.

http://codereview.chromium.org/9433027/diff/4007/remoting/protocol/pepper_ses...
remoting/protocol/pepper_session_unittest.cc:363: // Verify that data can sent
over stream channel.
On 2012/02/23 19:33:42, Wez wrote:
> typo: ... can be sent ...

The comment was incorrect.

http://codereview.chromium.org/9433027/diff/5002/remoting/protocol/fake_authe...
File remoting/protocol/fake_authenticator.cc (right):

http://codereview.chromium.org/9433027/diff/5002/remoting/protocol/fake_authe...
remoting/protocol/fake_authenticator.cc:43: // This will cannot call the
callback because |did_read_bytes_|
On 2012/02/23 19:33:42, Wez wrote:
> typo: will cannot

Done.

[Wez, 2012-02-24 00:37:48]

lgtm

[commit-bot: I haz the power, 2012-02-24 00:39:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/9433027/10003

[commit-bot: I haz the power, 2012-02-24 04:27:46]

Change committed as 123436