[COMPOSIT] Extend the AddRef to return the *type* of reference counti… (

#1510)

* [COMPOSIT] Extend the AddRef to return the *type* of reference counting is applicable.

Sometimes an object is reference counted but it is owned (composit) of a larger (COM)object. Than the reference count
is just for tracking correct programming and not specifically for determining the lifetime object (In release the
reference count can even be dropepd for these objects)

The SinkType<> template is such an object. It is always *part* of a bigger (COM)object and thus it is by design that the
lifetime of the SinkType<> object is always equasl to (and *not* extending) the lifetime of the object owning the
SinkType<> member. Typically its parent and typically also a Refernce counted object.

The rationale behind this is to use the SinkType<> template where possible as it uses less resources (no *additional* new
for allocation, and in release builds not even a _referenceCounter member). However, make sure that the lifetime of the
SinkType<> member is always smaller or equal to the lifetime of the owning object!

In R4 the Non-HappyDay scenarios where considered in the Thunder Framework to avoid object leakage without nothering
the plugin developper. This means that if an out-of-process plugin would crash, and during its lifetime took some
references on objects living in the parent process, the ThunderFrameowrk would, under the hood, automatically Release
these taken refernces on behalf of the crashed child process.

Thois would make sure that even if the Child process would crash, resources held in the parent process would still be
properly released, if, for example, the plugin would be deactivated.

However for COmposit plugins this could lead to a race condition. This cleaning up is typically done on idle time of the
framework but if the composit (SinkType<> template) was to be destructed already as that lifetime of the owning object
was already over the "under-the-hood" framework cleanu systm for crashing plugins would call into a dead object.

This PR is now indicating that an Object is actually an COM reference counted object, owned by another object and thus
and thus does not require the non-happy day Releasing on behalf of crashing processes.

I theory a crashing process is now nolonger Release Composit Reference counted objects and might potentially lead to
warning in the log but no leakages for real reference counted objects.

* Update Administrator.h

Source/WPEFramework/PluginServer.h

@@ -2288,9 +2288,10 @@ namespace PluginHost {
                 {
                     return (new RemoteInstantiation(parent, comms, connector));
                 }
-                void AddRef() const override
+                uint32_t AddRef() const override
                 {
                     Core::InterlockedIncrement(_refCount);
+                    return (Core::ERROR_NONE);
                 }
                 uint32_t Release() const override
                 {

Source/com/Administrator.cpp

@@ -344,21 +344,21 @@ namespace RPC {
         if (remotes != _channelReferenceMap.end()) {
             std::list<RecoverySet>::iterator loop(remotes->second.begin());
             while (loop != remotes->second.end()) {
-                uint32_t result = Core::ERROR_NONE;
 
-                // We will release on behalf of the other side :-)
-                do {
-                    Core::IUnknown* iface = loop->Unknown();
+                Core::IUnknown* iface = loop->Unknown();
+                ASSERT(iface != nullptr);
 
-                    ASSERT(iface != nullptr);
+                if ((iface != nullptr) && (loop->IsComposit() == false)) {
 
-                    if (iface != nullptr) {
+                    uint32_t result;
+
+                    // We will release on behalf of the other side :-)
+                    do {
                         result = iface->Release();
-                    }
-                } while ((loop->Decrement()) && (result == Core::ERROR_NONE));
+                    } while ((loop->Decrement(1)) && (result == Core::ERROR_NONE));
+                }
 
                 ASSERT (loop->Flushed() == true);
-
                 loop++;
             }
             _channelReferenceMap.erase(remotes);

Source/com/Administrator.h

@@ -63,28 +63,37 @@ namespace RPC {
             RecoverySet(const uint32_t id, Core::IUnknown* object)
                 : _interfaceId(id)
                 , _interface(object)
-                , _referenceCount(1) {
+                , _referenceCount(1 | (object->AddRef() == Core::ERROR_COMPOSIT_OBJECT ? 0x80000000 : 0)) {
+
+                // Check if this is a "Composit" object to which the IUnknown points. Composit means 
+                // that the object is owned by another object that controls its lifetime and that 
+                // object will handle the lifetime of this object. It will be released anyway, 
+                // no-recovery needed.
+                object->Release();
             }
             ~RecoverySet() = default;
 
         public:
+            bool IsComposit() const {
+                return ((_referenceCount & 0x80000000) != 0);
+            }
             inline uint32_t Id() const {
                 return (_interfaceId);
             }
             inline Core::IUnknown* Unknown() const {
                 return (_interface);
             }
             inline void Increment() {
-                _referenceCount++;
+                _referenceCount = ((_referenceCount & 0x7FFFFFFF) + 1) | (_referenceCount & 0x80000000);
             }
-            inline bool Decrement(const uint32_t dropCount = 1) {
-                ASSERT(_referenceCount >= dropCount);
-                _referenceCount -= dropCount;
-                return(_referenceCount > 0);
+            inline bool Decrement(const uint32_t dropCount) {
+                ASSERT((_referenceCount & 0x7FFFFFFF) >= dropCount);
+                _referenceCount = ((_referenceCount & 0x7FFFFFFF) - dropCount) | (_referenceCount & 0x80000000);
+                return((_referenceCount & 0x7FFFFFFF) > 0);
             }
 #ifdef __DEBUG__
             bool Flushed() const {
-                return (_referenceCount == 0);
+                return (((_referenceCount & 0x7FFFFFFF) == 0) || (IsComposit()));
             }
 #endif
 

Source/com/IUnknown.h

@@ -194,10 +194,11 @@ namespace ProxyStub {
 
             return(result);
         }
-        void AddRef() const {
+        uint32_t AddRef() const {
             _adminLock.Lock();
             _refCount++;
             _adminLock.Unlock();
+            return (Core::ERROR_NONE);
         }
         uint32_t Release() const {
             uint32_t result = Core::ERROR_NONE;
@@ -474,9 +475,9 @@ namespace ProxyStub {
         // -------------------------------------------------------------------------------------------------------------------------------
         // Applications calls to the Proxy
         // -------------------------------------------------------------------------------------------------------------------------------
-        void AddRef() const override
+        uint32_t AddRef() const override
         {
-            _unknown.AddRef();
+            return (_unknown.AddRef());
         }
         uint32_t Release() const override
         {

Source/com/IteratorType.h

@@ -85,7 +85,7 @@ namespace RPC {
         }
 
     public:
-        virtual void AddRef() const = 0;
+        virtual uint32_t AddRef() const = 0;
         virtual uint32_t Release() const = 0;
 
         virtual bool IsValid() const override

Source/core/IPCConnector.h

@@ -289,8 +289,8 @@ namespace Core {
             ~RawSerializedType() override = default;
 
         public:
-            void AddRef() const override {
-                _parent.AddRef();
+            uint32_t AddRef() const override {
+                return (_parent.AddRef());
             }
             uint32_t Release() const override {
                 return(_parent.Release());

Source/core/Portability.h

@@ -774,7 +774,7 @@ namespace Core {
 
     struct EXTERNAL IReferenceCounted {
         virtual ~IReferenceCounted() = default;
-        virtual void AddRef() const = 0;
+        virtual uint32_t AddRef() const = 0;
         virtual uint32_t Release() const = 0;
     };
 
@@ -894,7 +894,8 @@ namespace Core {
         ERROR_CODE(ERROR_UNKNOWN_METHOD, 53) \
         ERROR_CODE(ERROR_INVALID_PARAMETER, 54) \
         ERROR_CODE(ERROR_INTERNAL_JSONRPC, 55) \
-        ERROR_CODE(ERROR_PARSING_ENVELOPPE, 56)
+        ERROR_CODE(ERROR_PARSING_ENVELOPPE, 56) \
+        ERROR_CODE(ERROR_COMPOSIT_OBJECT, 57) \
 
     #define ERROR_CODE(CODE, VALUE) CODE = VALUE,
 

Source/core/Proxy.h

@@ -119,12 +119,14 @@ namespace WPEFramework {
             }
 
         public:
-            void AddRef() const override
+            uint32_t AddRef() const override
             {
                 if (_refCount == 1) {
                     const_cast<ProxyObject<CONTEXT>*>(this)->__Acquire();
                 }
                 _refCount++;
+
+                return (Core::ERROR_NONE);
             }
             uint32_t Release() const override
             {
@@ -512,12 +514,12 @@ POP_WARNING()
 
                 return (result);
             }
-            inline void AddRef() const
+            inline uint32_t AddRef() const
             {
                 // Only allowed on valid objects.
                 ASSERT(_refCount != nullptr);
 
-                _refCount->AddRef();
+                return (_refCount->AddRef());
             }
             inline bool operator==(const ProxyType<CONTEXT>& a_RHS) const
             {

Source/core/Services.h

@@ -71,9 +71,10 @@ namespace Core {
             ASSERT((callback == nullptr) ^ (_callback == nullptr));
             _callback = callback;
         }
-        void AddRef() const
+        uint32_t AddRef() const
         {
             Core::InterlockedIncrement(_instanceCount);
+            return (Core::ERROR_COMPOSIT_OBJECT);
         }
         uint32_t Release() const
         {
@@ -137,19 +138,28 @@ namespace Core {
             REPORT_OUTOFBOUNDS_WARNING(WarningReporting::SinkStillHasReference, _referenceCount);
 
             if (_referenceCount != 0) {
-                // This is probably due to the fact that the "other" side killed the connection, we need to
-                // Remove our selves at the COM Administrator map.. no need to signal Releases on behalf of the dropped connection anymore..
-                TRACE_L1("Oops this is scary, destructing a (%s) sink that still is being refered by something", typeid(ACTUALSINK).name());
+                // Since this is a Composit of a larger object, it could be that the reference count has
+                // not reached 0. This can happen if a process that has a reference to this SinkType (e.g. 
+                // a registered notification) crashed before it could unregister this notification. Due to
+                // the failure of this unregistering and the composit not being recovered through the COMRPC
+                // framework, the _referenceCount might be larger than 0.
+                // No need to worry if it is caused by a crashing process as this sink gets destroyed by the 
+                // owning object anyway (hence why you see this printf :-) ) but under normal conditions, 
+                // this TRACE should *not* be visible!!! If you also see it in happy-day scenarios there 
+                // is an unbalanced AddRef/Release in your code and you should take action !!!
+                TRACE_L1("Oops this might be scary, destructing a (%s) sink that still is being refered by something", typeid(ACTUALSINK).name());
             }
         }
 
     public:
-        virtual void AddRef() const
+        virtual uint32_t AddRef() const
         {
             Core::InterlockedIncrement(_referenceCount);
+            return (Core::ERROR_COMPOSIT_OBJECT);
         }
         virtual uint32_t Release() const
         {
+            ASSERT (_referenceCount > 0);
             Core::InterlockedDecrement(_referenceCount);
             return (Core::ERROR_NONE);
         }

Source/extensions/processcontainers/ProcessContainer.h

@@ -25,11 +25,11 @@ namespace WPEFramework {
 namespace ProcessContainers {
     using IStringIterator = Core::IteratorType<std::vector<string>, const string>;
 
-    class INetworkInterfaceIterator : public Core::IIterator, public Core::IReferenceCounted {
+    class INetworkInterfaceIterator : public Core::IIterator,  public Core::IReferenceCounted  {
     public:
         ~INetworkInterfaceIterator() override = default;
 
-        // Return interface name (eg. veth0)
+        // Return interface name (eg. eth0)
         virtual string Name() const = 0;
 
         // Return numver of ip addresses assigned to the interface

Source/extensions/processcontainers/common/BaseContainerIterator.h

@@ -35,7 +35,7 @@ namespace ProcessContainers {
 
         ~BaseContainerIterator() override = default;
 
-        virtual bool Next() override
+        bool Next() override
         {
             if (_current == UINT32_MAX)
                 _current = 0;

Source/extensions/processcontainers/common/BaseRefCount.h

@@ -32,9 +32,10 @@ namespace ProcessContainers {
         {
         }
 
-        void AddRef() const override
+        uint32_t AddRef() const override
         {
             Core::InterlockedIncrement(_refCount);
+            return(Core::ERROR_NONE);
         }
 
         uint32_t Release() const override

Source/extensions/processcontainers/implementations/AWCImplementation/AWCImplementation.cpp

@@ -74,7 +74,6 @@ namespace ProcessContainers {
         , _pid(0)
         , _runId(-1)
         , _appState(awc::AWC_STATE_UNKNOWN)
-        , _referenceCount(1)
         , _client(client)
         , _notifier(notifier)
         , _mutex()
@@ -204,20 +203,17 @@ namespace ProcessContainers {
         return result;
     }
 
-    void AWCContainer::AddRef() const
+    uint32_t AWCContainer::AddRef() const
     {
         TRACE_L3("%s _name=%s", _TRACE_FUNCTION_, _name.c_str());
-        WPEFramework::Core::InterlockedIncrement(_referenceCount);
+        return(BaseRefCount<IContainer>::AddRef());
     }
 
     uint32_t AWCContainer::Release() const
     {
-        TRACE_L3("%s _name=%s", _TRACE_FUNCTION_, _name.c_str());
-        uint32_t retval = WPEFramework::Core::ERROR_NONE;
-        if (WPEFramework::Core::InterlockedDecrement(_referenceCount) == 0) {
-            delete this;
-            retval = WPEFramework::Core::ERROR_DESTRUCTION_SUCCEEDED;
-        }
+        uint32_t retVal = BaseRefCount<IContainer>::Release();
+
+        TRACE_L3("%s _name=%s result=%d", _TRACE_FUNCTION_, _name.c_str(), retVal);
         return retval;
     }
 

Source/extensions/processcontainers/implementations/AWCImplementation/AWCImplementation.h

@@ -73,7 +73,7 @@ namespace ProcessContainers {
         bool Start(const string& command, ProcessContainers::IStringIterator& parameters) override;
         bool Stop(const uint32_t timeout /*ms*/) override;
 
-        void AddRef() const override;
+        uint32_t AddRef() const override;
         uint32_t Release() const override;
         void notifyStateChange(int req_id, awc::awc_app_state_t app_state, int status, unsigned int pid) override;
 
@@ -82,7 +82,6 @@ namespace ProcessContainers {
         uint32_t _pid;
         int _runId;
         awc::awc_app_state_t _appState;
-        mutable uint32_t _referenceCount;
         awc::AWCClient * _client;
         AWCStateChangeNotifier * _notifier;
         mutable std::mutex _mutex;

Source/extensions/processcontainers/implementations/LXCImplementation/LXCImplementation.cpp

@@ -173,7 +173,6 @@ namespace ProcessContainers {
         , _lxcPath(lxcPath)
         , _containerLogDir(containerLogDir)
         , _adminLock()
-        , _referenceCount(1)
         , _lxcContainer(lxcContainer)
     {
         Config config;
@@ -402,23 +401,18 @@ namespace ProcessContainers {
         return result;
     }
 
-    void LXCContainer::AddRef() const
+    uint32_t LXCContainer::AddRef() const
     {
-        WPEFramework::Core::InterlockedIncrement(_referenceCount);
         lxc_container_get(_lxcContainer);
+        return(BaseRefCount<IContainer>::AddRef());
     }
 
     uint32_t LXCContainer::Release() const
     {
-        uint32_t retval = WPEFramework::Core::ERROR_NONE;
-
         uint32_t lxcresult = lxc_container_put(_lxcContainer);
-        if (WPEFramework::Core::InterlockedDecrement(_referenceCount) == 0) {
-            ASSERT(lxcresult == 1); // if 1 is returned, lxc also released the container
+        uint32_t retVal = BaseRefCount<IContainer>::Release();
 
-            delete this;
-            retval = WPEFramework::Core::ERROR_DESTRUCTION_SUCCEEDED;
-        }
+        ASSERT((retval != WPEFramework::Core::ERROR_DESTRUCTION_SUCCEEDED) || (lxcresult == 1)); // if 1 is returned, lxc also released the container
         return retval;
     }
 

Source/extensions/processcontainers/implementations/LXCImplementation/LXCImplementation.h

@@ -112,7 +112,7 @@ namespace ProcessContainers {
         bool Start(const string& command, ProcessContainers::IStringIterator& parameters) override;
         bool Stop(const uint32_t timeout /*ms*/) override;
 
-        void AddRef() const override;
+        uint32_t AddRef() const override;
         uint32_t Release() const override;
 
     protected:
@@ -124,7 +124,6 @@ namespace ProcessContainers {
         string _lxcPath;
         string _containerLogDir;
         mutable Core::CriticalSection _adminLock;
-        mutable uint32_t _referenceCount;
         LxcContainerType* _lxcContainer;
 #ifdef __DEBUG__
         bool _attach;