Fixing potential AsyncInvoker deadlock that occurs for "reentrant" invocations.

webrtc/base/asyncinvoker.cc

Index: webrtc/base/asyncinvoker.cc
diff --git a/webrtc/base/asyncinvoker.cc b/webrtc/base/asyncinvoker.cc
index bfd13172f70329598dc9393d41eb660f8877e8f7..e3f33b5123ebc2dbabe849b044b258d579e0f715 100644
--- a/webrtc/base/asyncinvoker.cc
+++ b/webrtc/base/asyncinvoker.cc
@@ -29,8 +29,8 @@ AsyncInvoker::~AsyncInvoker() {
     // another thread, WITHIN an AsyncInvoked functor, it may do another
     // Thread::Post even after we called MessageQueueManager::Clear(this). So
     // we need to keep calling Clear to discard these posts.
-    MessageQueueManager::Clear(this);
     invocation_complete_.Wait(Event::kForever);
+    MessageQueueManager::Clear(this);

[nisse-webrtc, 2017/05/18 08:04:55]

This is subtle (maybe too subtle...).

So at the end of this loop, pending_invocations is zero. Is destruction  safe
then? I think it is; in the "reentrant" case count starts at 1 (or larger), goes
up to 2 when the new invocation is posted, and gets down to zero only when both
the parent closure and the new closure are destroyed. (And pending_invocations_
going from 0 to 1 indicates improper "top-level" invocations racing against the
destructor, which why can't take any responsibility for here).

As for the deadlock, I'm having some difficulty understanding 
what difference this change makes. Let us see if we can sort this out... 
MessageQueueManager::Clear is expected to destroy some references, with the
AsyncClosure destructor being called, and for each decrement, the event will be
signalled.

Say we race against another thread executing an AsyncClosure.
When it's done, it will signal the event, terminating our Wait(). By this time,
it has added the new invocation to our queue, so MessageQueueManager::Clear will
remove it, make pending_invocations_ zero (and signal the event again, which we
don't care about).

So how is this race different from the old code before this change? Even if the
new invocation gets posted between Clear and Wait, that means that the parent
invocation on the other thread was alive by the time we called Clear, and when
it finishes, our Wait wakes up. Clear in the next iteration through the loop
should destroy that AsyncClosure and signal the event again. So I'm having some
difficulty understanding the deadlock.

[Taylor Brandstetter, 2017/05/18 20:54:52]

I must have had a brain lapse and forgot that if "Wait" is called after an event
is already set, it returns immediately. So you're right, this makes no
difference.

   }
 }
 
@@ -69,7 +69,6 @@ void AsyncInvoker::DoInvoke(const Location& posted_from,
     LOG(LS_WARNING) << "Tried to invoke while destroying the invoker.";
     return;
   }
-  AtomicOps::Increment(&pending_invocations_);
   thread->Post(posted_from, this, id,
                new ScopedMessageData<AsyncClosure>(std::move(closure)));
 }
@@ -83,7 +82,6 @@ void AsyncInvoker::DoInvokeDelayed(const Location& posted_from,
     LOG(LS_WARNING) << "Tried to invoke while destroying the invoker.";
     return;
   }
-  AtomicOps::Increment(&pending_invocations_);
   thread->PostDelayed(posted_from, delay_ms, this, id,
                       new ScopedMessageData<AsyncClosure>(std::move(closure)));
 }
@@ -111,6 +109,10 @@ void GuardedAsyncInvoker::ThreadDestroyed() {
   thread_ = nullptr;
 }
 
+AsyncClosure::AsyncClosure(AsyncInvoker* invoker) : invoker_(invoker) {
+  AtomicOps::Increment(&invoker_->pending_invocations_);

[nisse-webrtc, 2017/05/18 08:04:55]

I take it moving the increment here from DoInvoke* is a stylistic rather than
functional change, it is still done *before* anything gets added to the
MessageQueue? Or am I missing some subtlety?

(Diversion: I no longer think incrementing from 0 to 1 is our problem, but I'll
keep this note anyhow in case some veriant of it is useful):
 
If we (1) change pending_invocations_ to be a refcount which is initialized to 1
and decremented also in the destructor, and (2) ensure that we never increment
it from zero, would that help? Could be done with something like

  while (true) {
    int refs = atomic::AcquireLoad(&refcount);
    if (!refs)
      return; 
    if (Atomic::CompareAndSwap(&refcount, refs, refs+1))
      break;
  }

We would at least get the helpful invariant that whenever refcount gets to zero,
it's stuck at zero. (But on the other hand, if it *is* zero, we shouldn't have
accessed it in the first place, since it's a likely use-after-free).

[Taylor Brandstetter, 2017/05/18 20:54:52]

It's still done before anything gets added to the MessageQueue. But it's not
just stylistic; it prevents the reference count from getting messed up in the
"tried to invoke while destroying" case. In that case, the destructor was
getting run, decrementing pending_invocations_, even though it wasn't
incremented. Resulting in a refcount of -1.

[nisse-webrtc, 2017/05/19 06:43:21]

I see, I was under the impression that the closure object was created by
DoInvoke* (and hence this change would move the increment slightly later). But
in fact, it moves the increment earlier, before the if (destroying_) check.

+}
+
 AsyncClosure::~AsyncClosure() {
   AtomicOps::Decrement(&invoker_->pending_invocations_);

[nisse-webrtc, 2017/05/18 11:12:39]

Thinking a bit more, I'm afraids this approach is fundamentally unsafe. Assume
the AsyncInvoker destructor gets to run between this Decrement call and the next
line. Then pending_invocations_ is zero, the AsyncInvoker object is destroyed,
and the access to the invocation_complete_ event below is a use-after-free. When
using a reference count, decrementing the count to zero has to be the final
access to the object.

Also note that using Event::Set involves both locking operations and a condition
variable, so attempting to avoid locks and instead doing an Event.Set per
invocation seems pointless.

I see two approaches that might work:

* Use an explicit lock, which could protect both the count, |destroying_ | flag
and any other needed state.

* Introduce a refcounted object, referenced (via plain scoped_refptr) from both
AsyncInvoker and AsyncClosure, and put all synchronization state there. Then I
think AsyncClosure could be implemented without lock operations, only atomics.
With heavier synchronization only at AsyncInvoker destruction time.

[Taylor Brandstetter, 2017/05/18 20:54:52]

That's exactly the issue I was trying to fix in this CL:

https://codereview.webrtc.org/2876273002/

... which just uses a busy loop, since I wanted to avoid adding a new lock for
every async task. I also made an alternative that uses a ref-counted object just
like you suggest:

https://codereview.webrtc.org/2885143005/

But *this* CL isn't about fixing that race condition. It's just about adding a
test for the "reentrant" invocations.
I didn't notice that... I'll try to come up with a solution that doesn't use the
event. But I think we can go ahead with this CL, at least so that the unit test
gets added.

[nisse-webrtc, 2017/05/19 06:43:21]

Makes sense. lgtm, then.

[nisse-webrtc, 2017/05/19 06:48:16]

But please update cl title accordingly...

   invoker_->invocation_complete_.Set();