Fixing race between ~AsyncInvoker and ~AsyncClosure, using ref-counting.

webrtc/rtc_base/asyncinvoker.h

Index: webrtc/rtc_base/asyncinvoker.h
diff --git a/webrtc/rtc_base/asyncinvoker.h b/webrtc/rtc_base/asyncinvoker.h
index 17d702a37bdce677636475f7f09cd52c5ffe9f1d..5ba270a6a361fc647f59c81b9a2a2587b6b99fde 100644
--- a/webrtc/rtc_base/asyncinvoker.h
+++ b/webrtc/rtc_base/asyncinvoker.h
@@ -18,6 +18,8 @@
 #include "webrtc/rtc_base/bind.h"
 #include "webrtc/rtc_base/constructormagic.h"
 #include "webrtc/rtc_base/event.h"
+#include "webrtc/rtc_base/refcountedobject.h"
+#include "webrtc/rtc_base/scoped_ref_ptr.h"
 #include "webrtc/rtc_base/sigslot.h"
 #include "webrtc/rtc_base/thread.h"
 
@@ -119,8 +121,18 @@ class AsyncInvoker : public MessageHandler {
                        uint32_t delay_ms,
                        uint32_t id);
   volatile int pending_invocations_ = 0;
-  Event invocation_complete_;
-  bool destroying_ = false;
+
+  // Reference counted so that if the AsyncInvoker destructor finishes before
+  // an AsyncClosure's destructor that's about to call
+  // "invocation_complete_->Set()", it's not dereferenced after being
+  // destroyed.
+  scoped_refptr<RefCountedObject<Event>> invocation_complete_;

[kwiberg-webrtc, 2017/08/04 09:28:09]

Umm... isn't this exactly how scoped_refptr isn't intended to be used? The ref
counting is supposed to be an inalienable part of the class, so that it's
impossible to create non-refcounted instances. But that isn't the case with
Event.

Adding reference counting to a class that lacks native support for it is
precisely the intended job of std::shared_ptr, on the other hand.

I'm thinking we should consider deviating from Chromium's ban on shared_ptr for
cases where non-intrusive reference counting is what we want; I'll write
something up. In the mean time, I guess using this construction in this CL is
OK. Having to create a RefCountedEventContainer class would be painful.

[Taylor Brandstetter, 2017/08/04 19:14:22]

Sorry, I still don't understand what the problem is. I'd use shared_ptr if I
could, but I don't see what practical difference it makes whether Event inherits
from RefCountInterface or not. Can you explain more/give an example?

[kwiberg-webrtc, 2017/08/06 04:17:12]

No, it's just me who's confused. The requirements on a refcounted type is that
it must only owned by folks who use its reference count correctly (which almost
inevitably implies it must be on the heap). Event isn't such a type, but
RefCountedObject<Event> is, and the fact that RefCountedObject<Event> is a
subclass of Event doesn't matter.

However,

  1. The reference counter methods are virtual for no good reason. (This could
be fixed by making a template like RefCountedObject that has non-virtual
reference counter methods.)

  2. The contained class is required to be non-final. (Not a problem in this
case, because Event is non-final. Could be fixed by making a refcounted template
that has the given class as a member instead of as a subclass.)

  3. The contained class is required to be concrete. (Not a problem in this
case, because Event is concrete. Could be fixed by having a unique_ptr<T>
instead of a plain T as member.)

...at which point we've essentially reinvented a primitive form of
std::shared_ptr.

So keep doing what you're doing for now (it *is* safe), and we'll see if we can
get a consensus on allowing std::shared_ptr in cases like this where it would
obviously be a good fit.

+
+  // This flag is used to ensure that if an application AsyncInvokes tasks that
+  // recursively AsyncInvoke other tasks ad infinitum, the cycle eventually
+  // terminates.

[kwiberg-webrtc, 2017/08/04 09:28:09]

That's why you need it. Could you also explain what the value means? Is it a
count of the number of currently running destructors, or what?

+  int destroying_ = 0;

[kwiberg-webrtc, 2017/08/04 09:28:09]

Ooohhh... when reading the .cc file, I found that you use atomic operations on
this member. That isn't apparent from its type! Consider using std::atomic<int>
instead. (Side note: We should probably remove our home-grown atomics and use
the std:: atomics everywhere.)

Hmm. No, it should be std::atomic<bool>, right? It isn't a counter, but with our
homegrown atomics, int is the only available type.

[Taylor Brandstetter, 2017/08/04 19:14:22]

Oh, apparently <atomic> is a feature chromium allows now! I'll switch to that.

+
   friend class AsyncClosure;
 
   RTC_DISALLOW_COPY_AND_ASSIGN(AsyncInvoker);
@@ -149,7 +161,7 @@ class GuardedAsyncInvoker : public sigslot::has_slots<> {
   bool AsyncInvoke(const Location& posted_from,
                    const FunctorT& functor,
                    uint32_t id = 0) {
-    rtc::CritScope cs(&crit_);
+    CritScope cs(&crit_);
     if (thread_ == nullptr)
       return false;
     invoker_.AsyncInvoke<ReturnT, FunctorT>(posted_from, thread_, functor, id);
@@ -163,7 +175,7 @@ class GuardedAsyncInvoker : public sigslot::has_slots<> {
                           const FunctorT& functor,
                           uint32_t delay_ms,
                           uint32_t id = 0) {
-    rtc::CritScope cs(&crit_);
+    CritScope cs(&crit_);
     if (thread_ == nullptr)
       return false;
     invoker_.AsyncInvokeDelayed<ReturnT, FunctorT>(posted_from, thread_,
@@ -180,7 +192,7 @@ class GuardedAsyncInvoker : public sigslot::has_slots<> {
                    void (HostT::*callback)(ReturnT),
                    HostT* callback_host,
                    uint32_t id = 0) {
-    rtc::CritScope cs(&crit_);
+    CritScope cs(&crit_);
     if (thread_ == nullptr)
       return false;
     invoker_.AsyncInvoke<ReturnT, FunctorT, HostT>(
@@ -198,7 +210,7 @@ class GuardedAsyncInvoker : public sigslot::has_slots<> {
                    void (HostT::*callback)(),
                    HostT* callback_host,
                    uint32_t id = 0) {
-    rtc::CritScope cs(&crit_);
+    CritScope cs(&crit_);
     if (thread_ == nullptr)
       return false;
     invoker_.AsyncInvoke<ReturnT, FunctorT, HostT>(