Fix XmppLoginHandler to handle destruction from delegate methods.

Fix XmppLoginHandler to handle destruction from delegate methods.

XmppLoginHandler was crashing when it's destroyed from SendMessage().
E.g. it can happen when Write() fails after TLS handshake. Also added
unittests to test this scenario.

BUG=498989
Committed: https://crrev.com/56988978ff675e340927da78f65c123f8b6a0008
Cr-Commit-Position: refs/heads/master@{#333877}

[Sergey Ulanov, 2015-06-10 21:25:24]

sergeyu@chromium.org changed reviewers:
+ jamiewalch@chromium.org

[Jamie, 2015-06-10 22:13:40]

AIUI, a crash is not guaranteed when accessing data members of a deleted object,
so the unit tests may not catch regressions (or may be flaky). It seems like the
pattern of allowing a delegate to delete its owner is risky, unless you're using
something like refcounting to make sure that the deletion is deferred until it's
safe.

In this specific case, there are lots of delegate method invocations where it's
not trivial by inspection that no code runs after the delegate returns (delegate
calls inside multiple switch cases, for example). They're all safe if you look
carefully enough, but I don't have a lot of confidence that future code changes
won't regress them.

Do we have a pattern like Objective C's autorelease in Chromium, which would let
us do this more safely?

[Wez, 2015-06-10 22:17:52]

On 2015/06/10 22:13:40, Jamie wrote:
> AIUI, a crash is not guaranteed when accessing data members of a deleted
object,
> so the unit tests may not catch regressions (or may be flaky). It seems like
the
> pattern of allowing a delegate to delete its owner is risky, unless you're
using
> something like refcounting to make sure that the deletion is deferred until
it's
> safe.
> 
> In this specific case, there are lots of delegate method invocations where
it's
> not trivial by inspection that no code runs after the delegate returns
(delegate
> calls inside multiple switch cases, for example). They're all safe if you look
> carefully enough, but I don't have a lot of confidence that future code
changes
> won't regress them.
> 
> Do we have a pattern like Objective C's autorelease in Chromium, which would
let
> us do this more safely?

TaskRunner has the DeleteSoon() method, which allows you to defer deletion back
to the thread's message loop.

[Wez, 2015-06-10 22:20:30]

On 2015/06/10 22:17:52, Wez wrote:
> On 2015/06/10 22:13:40, Jamie wrote:
> > AIUI, a crash is not guaranteed when accessing data members of a deleted
> object,
> > so the unit tests may not catch regressions (or may be flaky). It seems like
> the
> > pattern of allowing a delegate to delete its owner is risky, unless you're
> using
> > something like refcounting to make sure that the deletion is deferred until
> it's
> > safe.
> > 
> > In this specific case, there are lots of delegate method invocations where
> it's
> > not trivial by inspection that no code runs after the delegate returns
> (delegate
> > calls inside multiple switch cases, for example). They're all safe if you
look
> > carefully enough, but I don't have a lot of confidence that future code
> changes
> > won't regress them.
> > 
> > Do we have a pattern like Objective C's autorelease in Chromium, which would
> let
> > us do this more safely?
> 
> TaskRunner has the DeleteSoon() method, which allows you to defer deletion
back
> to the thread's message loop.

In this case the simplest approach is probably to use PostTask() to defer the
calls to the delegate to run w/ a clean stack.

[Sergey Ulanov, 2015-06-11 00:54:57]

On 2015/06/10 22:13:40, Jamie wrote:
> AIUI, a crash is not guaranteed when accessing data members of a deleted
object,
> so the unit tests may not catch regressions (or may be flaky).

ASan will normally catch such errors even if they don't cause a crash.

> It seems like the
> pattern of allowing a delegate to delete its owner is risky, unless you're
using
> something like refcounting to make sure that the deletion is deferred until
it's
> safe.

It's common to allow deletion from callbacks. E.g. sockets can be deleted from
read/write callbacks. And that's the approach we follow in most parts in
remoting. Ref-counting doesn't really make it easier to handle objects
lifetimes. If XmppLoginHandler was ref-counted then it may outlive
XmppSignalStrategy, which means that the delegate_ may become invalid so we
would have some extra code to handle that case. Also XmppLoginHandler was
already implemented such that it can be deleted from Delegate, it was only
broken for Delegate::SendMessage().

> 
> In this specific case, there are lots of delegate method invocations where
it's
> not trivial by inspection that no code runs after the delegate returns
(delegate
> calls inside multiple switch cases, for example). They're all safe if you look
> carefully enough, but I don't have a lot of confidence that future code
changes
> won't regress them.
> 
> Do we have a pattern like Objective C's autorelease in Chromium, which would
let
> us do this more safely?

There is nothing like autorelease because most objects are not ref-counted.
I think in most cases callbacks are allowed to delete the caller. In some cases
DeleteSoon() is used, but it's not a silver bullet - it still doesn't guarantee
that stuff is destroyed in correct order and it doesn't work well when you have
more complex dependencies between objects.
One nice thing about the approach I'm using here is that this fix is isolated to
XmppLoginHandler. Not allowing Delegate implementation to delete
XmppLoginHandler moves that complexity out of XmppLoginHandler into Delegate
implementations. It doesn't matter much in this particular case, but if there
were more than one Delegate implementations each of them would need to take care
of deleting XmppLoginHandler properly.

[Jamie, 2015-06-11 00:59:31]

ASAN goes a long way to mitigating my concerns about this, especially since it
seems like trying to make this bulletproof introduces a lot of extra complexity.
LGTM.

[Sergey Ulanov, 2015-06-11 01:17:10]

On 2015/06/10 22:20:30, Wez wrote:
> On 2015/06/10 22:17:52, Wez wrote:
> > On 2015/06/10 22:13:40, Jamie wrote:
> > > AIUI, a crash is not guaranteed when accessing data members of a deleted
> > object,
> > > so the unit tests may not catch regressions (or may be flaky). It seems
like
> > the
> > > pattern of allowing a delegate to delete its owner is risky, unless you're
> > using
> > > something like refcounting to make sure that the deletion is deferred
until
> > it's
> > > safe.
> > > 
> > > In this specific case, there are lots of delegate method invocations where
> > it's
> > > not trivial by inspection that no code runs after the delegate returns
> > (delegate
> > > calls inside multiple switch cases, for example). They're all safe if you
> look
> > > carefully enough, but I don't have a lot of confidence that future code
> > changes
> > > won't regress them.
> > > 
> > > Do we have a pattern like Objective C's autorelease in Chromium, which
would
> > let
> > > us do this more safely?
> > 
> > TaskRunner has the DeleteSoon() method, which allows you to defer deletion
> back
> > to the thread's message loop.
> 
> In this case the simplest approach is probably to use PostTask() to defer the
> calls to the delegate to run w/ a clean stack.

Yes. That would be a different way to satisfy the requirement that Delegate
methods can delete the object. But it would be much more complicated than
replacing method calls with PostTask(). One issue is that PostTask() would need
to be posted with weak pointers. Much bigger probelm is that the Delegate
interface would need to be redesigned because the current version won't work
properly if it's called asynchronously. E.g. right now when OnHandshakeDone() is
called it's assumed that StreamParser is passed to the delegate synchronously.
If that happens asynchronously then there would be a period when neither
XmppSignalStrategy nor XmppLoginHandler have a reference to stream parser, but
any incoming data would still need to be handled somehow. Also currently
XmppLoginHandler doesn't expect any calls after OnLoginHandlerError(), but that
would not be the case if OnLoginHandlerError() was asynchronous.

[Sergey Ulanov, 2015-06-11 01:35:15]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2015-06-11 01:35:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1179543004/1

[commit-bot: I haz the power, 2015-06-11 02:23:41]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-06-11 02:24:42]

Patchset 1 (id:??) landed as
https://crrev.com/56988978ff675e340927da78f65c123f8b6a0008
Cr-Commit-Position: refs/heads/master@{#333877}