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

webrtc/rtc_base/asyncinvoker.h

 /*
  *  Copyright 2014 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #ifndef WEBRTC_RTC_BASE_ASYNCINVOKER_H_
 #define WEBRTC_RTC_BASE_ASYNCINVOKER_H_
 
 #include <memory>
 #include <utility>
 
 #include "webrtc/rtc_base/asyncinvoker-inl.h"
 #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"
 
 namespace rtc {
 
 // Invokes function objects (aka functors) asynchronously on a Thread, and
 // owns the lifetime of calls (ie, when this object is destroyed, calls in
 // flight are cancelled). AsyncInvoker can optionally execute a user-specified
 // function when the asynchronous call is complete, or operates in
 // fire-and-forget mode otherwise.
@@ skipping 81 matching lines @@
   void DoInvoke(const Location& posted_from,
                 Thread* thread,
                 std::unique_ptr<AsyncClosure> closure,
                 uint32_t id);
   void DoInvokeDelayed(const Location& posted_from,
                        Thread* thread,
                        std::unique_ptr<AsyncClosure> closure,
                        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);
 };
 
 // Similar to AsyncInvoker, but guards against the Thread being destroyed while
 // there are outstanding dangling pointers to it. It will connect to the current
 // thread in the constructor, and will get notified when that thread is
 // destroyed. After GuardedAsyncInvoker is constructed, it can be used from
 // other threads to post functors to the thread it was constructed on. If that
 // thread dies, any further calls to AsyncInvoke() will be safely ignored.
 class GuardedAsyncInvoker : public sigslot::has_slots<> {
  public:
   GuardedAsyncInvoker();
   ~GuardedAsyncInvoker() override;
 
   // Synchronously execute all outstanding calls we own, and wait for calls to
   // complete before returning. Optionally filter by message id. The destructor
   // will not wait for outstanding calls, so if that behavior is desired, call
   // Flush() first. Returns false if the thread has died.
   bool Flush(uint32_t id = MQID_ANY);
 
   // Call |functor| asynchronously with no callback upon completion. Returns
   // immediately. Returns false if the thread has died.
   template <class ReturnT, class FunctorT>
   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);
     return true;
   }
 
   // Call |functor| asynchronously with |delay_ms|, with no callback upon
   // completion. Returns immediately. Returns false if the thread has died.
   template <class ReturnT, class FunctorT>
   bool AsyncInvokeDelayed(const Location& posted_from,
                           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_,
                                                    functor, delay_ms, id);
     return true;
   }
 
   // Call |functor| asynchronously, calling |callback| when done. Returns false
   // if the thread has died.
   template <class ReturnT, class FunctorT, class HostT>
   bool AsyncInvoke(const Location& posted_from,
                    const Location& callback_posted_from,
                    const FunctorT& functor,
                    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>(
         posted_from, callback_posted_from, thread_, functor, callback,
         callback_host, id);
     return true;
   }
 
   // Call |functor| asynchronously calling |callback| when done. Overloaded for
   // void return. Returns false if the thread has died.
   template <class ReturnT, class FunctorT, class HostT>
   bool AsyncInvoke(const Location& posted_from,
                    const Location& callback_posted_from,
                    const FunctorT& functor,
                    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>(
         posted_from, callback_posted_from, thread_, functor, callback,
         callback_host, id);
     return true;
   }
 
  private:
   // Callback when |thread_| is destroyed.
   void ThreadDestroyed();
 
   CriticalSection crit_;
   Thread* thread_ GUARDED_BY(crit_);
   AsyncInvoker invoker_ GUARDED_BY(crit_);
 };
 
 }  // namespace rtc
 
 #endif  // WEBRTC_RTC_BASE_ASYNCINVOKER_H_