Create a desktop environment instance late so that it will see the curtain_required flag.

Create a desktop environment instance late so that it will see the curtain_required flag.

BUG=137696
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188279

[alexeypa (please no reviews), 2013-03-08 18:39:00]

PTAL.

[Wez, 2013-03-12 01:38:36]

ClientSession is ref-counted and can out-live the ChromotingHost, and thereby
the DesktopEnvironmentFactory, so this I don't think this change is safe.

It should be possible to replace ref-counting of ClientSession with use of
WeakPtr, and then we can rely on desktop_environment_factory_ out-living it (and
document that requirement in the ctor).

[alexeypa (please no reviews), 2013-03-12 18:31:34]

On 2013/03/12 01:38:36, Wez wrote:
> ClientSession is ref-counted and can out-live the ChromotingHost, and thereby
> the DesktopEnvironmentFactory, so this I don't think this change is safe.

Yes and no. It is true that storing a raw pointer in a ref-counted object is
generally bad idea because the latter one can outlive the former.

However I don't think this CL makes the code less safer. There are a few
reasons:

1. ChromotingHost and ClientSession are both ref-counted. They both store and
use a raw pointer to desktop environment factory. They both ensure that they are
destroyed before the factory:

  - ChromotingHost makes sure all sessions are deleted before it is deleted.
  - HostProcess makes sure that ChromotingHost is deleted before tearing down
itself.

2. Besides |desktop_environment_factory| there is another raw pointer that
ClientSession stores and uses: |event_handler|. The caller (i.e. ChromotingHost)
makes sure that |event_handler| outlives the session object. The same is true
for |desktop_environment_factory|.

> It should be possible to replace ref-counting of ClientSession with use of
> WeakPtr, and then we can rely on desktop_environment_factory_ out-living it
(and
> document that requirement in the ctor).

Documenting the requirement is a good idea. I'll fix that.

We could actually try to make ClientSession a non-refcounted class. It seems
that we already got rid of most tasks posted to it. I don't think this change
belongs to this CL however. It should be done as a separate CL. The same is true
about fixing |desktop_environment_factory| life time. It should be a separate CL
too.

[Wez, 2013-03-12 20:25:03]

On 2013/03/12 18:31:34, alexeypa wrote:
> On 2013/03/12 01:38:36, Wez wrote:
> > ClientSession is ref-counted and can out-live the ChromotingHost, and
thereby
> > the DesktopEnvironmentFactory, so this I don't think this change is safe.
> 
> Yes and no. It is true that storing a raw pointer in a ref-counted object is
> generally bad idea because the latter one can outlive the former.
> 
> However I don't think this CL makes the code less safer. There are a few
> reasons:
> 
> 1. ChromotingHost and ClientSession are both ref-counted. They both store and
> use a raw pointer to desktop environment factory.

ClientSession does not store a pointer to the DesktopEnvironmentFactory,
specifically because it is not guaranteed to out-live the session object (though
see below for possible mitigation).

> They both ensure that they are
> destroyed before the factory:
> 
>   - ChromotingHost makes sure all sessions are deleted before it is deleted.

ClientSessions are shutdown asynchronously and don't call-back to ChromotingHost
when shutdown completes.  See:
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/chro....

That can be addresses by NULLing the DesktopEnvironmentFactory pointer in
ClientSession::Stop(), though.

>   - HostProcess makes sure that ChromotingHost is deleted before tearing down
> itself.
> 
> 2. Besides |desktop_environment_factory| there is another raw pointer that
> ClientSession stores and uses: |event_handler|. The caller (i.e.
ChromotingHost)
> makes sure that |event_handler| outlives the session object.

The caller does not ensure that - it has no way to do so, since the session
object is ref-counted.

However, |event_handler| is only used to notify ChromotingHost in response to
|connection_| events, and |connect_| is reset in ClientSession::Stop().

You're right, though - |event_handler_| should  be NULLed in Stop() and the
comment on ClientSession's ctor should be corrected to indicate that it must
remain valid until Stop() is called.

> The same is true for |desktop_environment_factory|.

|desktop_environment_factory| is called synchronously in the ClientSession ctor,
and not stored by the ClientSession.

> > It should be possible to replace ref-counting of ClientSession with use of
> > WeakPtr, and then we can rely on desktop_environment_factory_ out-living it
> (and
> > document that requirement in the ctor).
> 
> Documenting the requirement is a good idea. I'll fix that.
> 
> We could actually try to make ClientSession a non-refcounted class. It seems
> that we already got rid of most tasks posted to it. I don't think this change
> belongs to this CL however. It should be done as a separate CL. The same is
true
> about fixing |desktop_environment_factory| life time. It should be a separate
CL
> too

See above - the only problem with the current code is the ambiguity over
|event_handler|'s lifetime, which I think works as intended in practice, but we
should still fix properly.

We can remove ref-counting from ClientSession in a follow-up CL, in which case
this CL must NULL |desktop_environment_factory| in ClientSession::Stop(), and
document the requirement on its lifetime.

[alexeypa (please no reviews), 2013-03-12 20:51:11]

On 2013/03/12 20:25:03, Wez wrote:
> ClientSession does not store a pointer to the DesktopEnvironmentFactory,

I thought it was obvious, but I was talking about the code after this CL is
applied. 

> ClientSessions are shutdown asynchronously and don't call-back to
ChromotingHost
> when shutdown completes.

True, but it does not matter in this case. ClientSession::Stop() ensures (though
in not very obvious way) that |desktop_environment_factory_| is not used after
it is called. Also |desktop_environment_factory_| pointer is used in much
narrower scope than |event_handler|. 

It is enough for practical purposes. I agree, however, that it is not a strong
guarantee and it can backfire when this code changes (and therefore it should be
addressed by a follow up CL.)

> That can be addresses by NULLing the DesktopEnvironmentFactory pointer in
> ClientSession::Stop(), though.

Agree.

> The caller does not ensure that - it has no way to do so, since the session
> object is ref-counted.

Yes.

> However, |event_handler| is only used to notify ChromotingHost in response to
> |connection_| events, and |connect_| is reset in ClientSession::Stop().

Yes, same as |desktop_environment_factory|.

> |desktop_environment_factory| is called synchronously in the ClientSession
ctor,
> and not stored by the ClientSession.

Again, I was talking about the code after this CL is applied. 

> We can remove ref-counting from ClientSession in a follow-up CL, in which case
> this CL must NULL |desktop_environment_factory| in ClientSession::Stop(), and
> document the requirement on its lifetime.

Done.

[alexeypa (please no reviews), 2013-03-13 20:03:45]

ping.

[Wez, 2013-03-14 04:15:54]

https://chromiumcodereview.appspot.com/12544019/diff/29001/remoting/host/clie...
File remoting/host/client_session.cc (right):

https://chromiumcodereview.appspot.com/12544019/diff/29001/remoting/host/clie...
remoting/host/client_session.cc:161: connection_factory_.GetWeakPtr()));
ClientSession owns the DesktopEnvironment, so can't we
Bind(&ClientSession::Disconnect, Unretained(this))?

If we do that then I think we can get rid of |connection_factory_| as well, and
tear-down the DesktopEnvironment in OnConnectionClosed, instead.

That does depend upon DesktopEnvironmentFactory::Create() guaranteeing that the
supplied callback can only be called while the DesktopEnvironment actually
exists.

https://chromiumcodereview.appspot.com/12544019/diff/29001/remoting/host/clie...
File remoting/host/client_session.h (right):

https://chromiumcodereview.appspot.com/12544019/diff/29001/remoting/host/clie...
remoting/host/client_session.h:164: // Used to create an instance of desktop
environment for this session.
nit: "an instance of ..." -> "a DesktopEnvironment instance"

https://chromiumcodereview.appspot.com/12544019/diff/29001/remoting/host/clie...
File remoting/host/client_session_unittest.cc (right):

https://chromiumcodereview.appspot.com/12544019/diff/29001/remoting/host/clie...
remoting/host/client_session_unittest.cc:262: .InSequence(s)
Aren't you effectively saying that you expect the first ProcessVideoPacket to
occur only after the EventExecutor::StartPtr?  Is that actually guaranteed?

https://chromiumcodereview.appspot.com/12544019/diff/29001/remoting/host/clie...
remoting/host/client_session_unittest.cc:337: .InSequence(s)
See above.

https://chromiumcodereview.appspot.com/12544019/diff/29001/remoting/host/clie...
remoting/host/client_session_unittest.cc:388: .InSequence(s)
See above.

https://chromiumcodereview.appspot.com/12544019/diff/29001/remoting/host/clie...
remoting/host/client_session_unittest.cc:441: .InSequence(s)
See above.

[Wez, 2013-03-14 04:16:57]

Oh, and thanks for the un-ref-counting precursor CL. :)


On 13 March 2013 13:03, <alexeypa@chromium.org> wrote:

> ping.
>
>
https://chromiumcodereview.**appspot.com/12544019/<https://chromiumcodereview...
>

[alexeypa (please no reviews), 2013-03-14 19:09:06]

https://chromiumcodereview.appspot.com/12544019/diff/29001/remoting/host/clie...
File remoting/host/client_session.cc (right):

https://chromiumcodereview.appspot.com/12544019/diff/29001/remoting/host/clie...
remoting/host/client_session.cc:161: connection_factory_.GetWeakPtr()));
On 2013/03/14 04:15:54, Wez wrote:
> ClientSession owns the DesktopEnvironment, so can't we
> Bind(&ClientSession::Disconnect, Unretained(this))?

No, we can't. The callback is passed to DesktopSessionProxy which potentially
tears down later than ClientSession.

https://chromiumcodereview.appspot.com/12544019/diff/29001/remoting/host/clie...
File remoting/host/client_session.h (right):

https://chromiumcodereview.appspot.com/12544019/diff/29001/remoting/host/clie...
remoting/host/client_session.h:164: // Used to create an instance of desktop
environment for this session.
On 2013/03/14 04:15:54, Wez wrote:
> nit: "an instance of ..." -> "a DesktopEnvironment instance"

Done.

https://chromiumcodereview.appspot.com/12544019/diff/29001/remoting/host/clie...
File remoting/host/client_session_unittest.cc (right):

https://chromiumcodereview.appspot.com/12544019/diff/29001/remoting/host/clie...
remoting/host/client_session_unittest.cc:262: .InSequence(s)
On 2013/03/14 04:15:54, Wez wrote:
> Aren't you effectively saying that you expect the first ProcessVideoPacket to
> occur only after the EventExecutor::StartPtr?  Is that actually guaranteed?

It is not guaranteed if we assume that video scheduler, capturer and encoder can
run on the same thread.

[Wez, 2013-03-15 00:15:25]

lgtm

https://chromiumcodereview.appspot.com/12544019/diff/29001/remoting/host/clie...
File remoting/host/client_session.cc (right):

https://chromiumcodereview.appspot.com/12544019/diff/29001/remoting/host/clie...
remoting/host/client_session.cc:161: connection_factory_.GetWeakPtr()));
On 2013/03/14 19:09:06, alexeypa wrote:
> On 2013/03/14 04:15:54, Wez wrote:
> > ClientSession owns the DesktopEnvironment, so can't we
> > Bind(&ClientSession::Disconnect, Unretained(this))?
> 
> No, we can't. The callback is passed to DesktopSessionProxy which potentially
> tears down later than ClientSession. 

That feels like IpcDesktopEnvironment/DesktopSessionProxy's problem to deal
with, not the caller's.  An issue for another CL, though.

https://chromiumcodereview.appspot.com/12544019/diff/29001/remoting/host/clie...
File remoting/host/client_session_unittest.cc (right):

https://chromiumcodereview.appspot.com/12544019/diff/29001/remoting/host/clie...
remoting/host/client_session_unittest.cc:262: .InSequence(s)
On 2013/03/14 19:09:06, alexeypa wrote:
> On 2013/03/14 04:15:54, Wez wrote:
> > Aren't you effectively saying that you expect the first ProcessVideoPacket
to
> > occur only after the EventExecutor::StartPtr?  Is that actually guaranteed?
> 
> It is not guaranteed if we assume that video scheduler, capturer and encoder
can
> run on the same thread.

nit: If you instead save the OnSessionAuthenticated expectation then you can
expect that StartPtr, OnSessionChannelsConnected and ProcessVideoPacketPtr must
all come after it. You should still be able to use InSequence for the sequential
portion of the expectations, provided the first of them is
After(ProcessVideoPacketPtr).

[alexeypa (please no reviews), 2013-03-15 00:28:02]

https://chromiumcodereview.appspot.com/12544019/diff/29001/remoting/host/clie...
File remoting/host/client_session_unittest.cc (right):

https://chromiumcodereview.appspot.com/12544019/diff/29001/remoting/host/clie...
remoting/host/client_session_unittest.cc:262: .InSequence(s)
On 2013/03/15 00:15:26, Wez wrote:
> On 2013/03/14 19:09:06, alexeypa wrote:
> > On 2013/03/14 04:15:54, Wez wrote:
> > > Aren't you effectively saying that you expect the first ProcessVideoPacket
> to
> > > occur only after the EventExecutor::StartPtr?  Is that actually
guaranteed?
> > 
> > It is not guaranteed if we assume that video scheduler, capturer and encoder
> can
> > run on the same thread.
> 
> nit: If you instead save the OnSessionAuthenticated expectation then you can
> expect that StartPtr, OnSessionChannelsConnected and ProcessVideoPacketPtr
must
> all come after it. You should still be able to use InSequence for the
sequential
> portion of the expectations, provided the first of them is
> After(ProcessVideoPacketPtr).

Done.

[commit-bot: I haz the power, 2013-03-15 00:29:57]

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

[commit-bot: I haz the power, 2013-03-15 04:16:47]

Retried try job too often on win_rel for step(s) chrome_frame_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-03-15 04:51:19]

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

[commit-bot: I haz the power, 2013-03-15 07:18:18]

Change committed as 188279