upnp: unconditionally remove failed mappings

In the previous implementation, a failed mapping was not removed if it's
beeing used. The responsability was left to the owner to release it when
done with it. This requires that all the owners implement the mapping
callback and properly release the mapping. This might be somehow cumbersome
and prune to error.
Now, a failed mapping is automatically removed, and no further action is
required from the owner. The mapping's owner will still receive a notification
callback (if registered) when the state of the mapping changes to failed (or
to any other state).

Change-Id: I014ba3906b836efce3173fb22fbe298a4023db7e
Gitlab: #335

src/ice_transport.cpp

@@ -800,6 +800,7 @@ IceTransport::Impl::requestUpnpMappings()
             }
         } else {
             JAMI_ERR("[ice:%p]: UPNP mapping request failed!", this);
+            upnp_->releaseMapping(requestedMap);
         }
     }
 }

src/upnp/upnp_context.cpp

@@ -115,10 +115,8 @@ UPnPContext::StopUpnp()
     for (auto const& map : toRemoveList) {
         map->cancelTimeoutTimer();
         updateMappingState(map, MappingState::FAILED);
-        if (map->isAvailable()) {
         unregisterMapping(map);
     }
-    }
 
     for (auto const& [_, protocol] : protocolList_) {
         protocol->clearIgds();
@@ -529,6 +527,18 @@ UPnPContext::updateMappingList(bool async)
                  status.inProgressCount_,
                  status.failedCount_);
 
+        if (status.failedCount_ > 0) {
+            std::lock_guard<std::mutex> lock(mappingMutex_);
+            auto const& mappingList = getMappingList(type);
+            for (auto const& [_, map] : mappingList) {
+                if (map->getState() == MappingState::FAILED) {
+                    JAMI_DBG("Mapping status [%s] - Available [%s]",
+                             map->toString(true).c_str(),
+                             map->isAvailable() ? "YES" : "NO");
+                }
+            }
+        }
+
         int toRequestCount = (int) minOpenPortLimit_[idx]
                              - (int) (status.readyCount_ + status.inProgressCount_
                                       + status.pendingCount_);
@@ -626,13 +636,10 @@ UPnPContext::pruneUnMatchedMappings(const std::shared_ptr<IGD>& igd,
 
         for (auto const& map : toRemoveList) {
             updateMappingState(map, MappingState::FAILED);
-            // Remove if not used.
-            if (map->isAvailable()) {
             unregisterMapping(map);
         }
     }
 }
-}
 
 void
 UPnPContext::pruneUnTrackedMappings(const std::shared_ptr<IGD>& igd,
@@ -695,11 +702,9 @@ UPnPContext::pruneMappingsWithInvalidIgds(const std::shared_ptr<IGD>& igd)
                  igd->getProtocolName());
         map->cancelTimeoutTimer();
         updateMappingState(map, MappingState::FAILED);
-        if (map->isAvailable()) {
         unregisterMapping(map);
     }
 }
-}
 
 void
 UPnPContext::processPendingRequests(const std::shared_ptr<IGD>& igd)
@@ -922,11 +927,8 @@ UPnPContext::onMappingAdded(const std::shared_ptr<IGD>& igd, const Mapping& mapR
                  map->toString().c_str(),
                  igd->getLocalIp().toString().c_str());
         updateMappingState(map, MappingState::FAILED);
-        // Remove if not used.
-        if (map->isAvailable()) {
         unregisterMapping(map);
     }
-    }
 
     // We have a response, so cancel the time-out.
     map->cancelTimeoutTimer();
@@ -1195,12 +1197,8 @@ UPnPContext::onRequestTimeOut(const std::shared_ptr<IGD>& igd, const Mapping::sh
 
     // Considere time-out as failure.
     updateMappingState(map, MappingState::FAILED);
-
-    // Remove it if not used.
-    if (map->isAvailable()) {
     unregisterMapping(map);
 }
-}
 
 void
 UPnPContext::updateMappingState(const Mapping::sharedPtr_t& map, MappingState newState, bool notify)