Allow to call mDNS browse callback more than once (#19554)

* [Tizen] Wait for mDNS on-browse events until timeout

Tizen Native API dos not deliver all-for-now event (such event is
delivered by e.g. Avahi), so it is impossible to tell when the mDNS
browsing shall be considered finished. This commit adds timeout event
which is delivered after 250ms from the last on-browse event. This
allows Tizen platform to discover more than one mDNS service on local
network.

* Allow to call mDNS browse callback more than once

This commit extends mDNS browsing callback to be called once per single
mDNS service or all at once. In case when the callback might be called
again, the internal proxy is not released.

The callback could be called with the error code equal to:

- CHIP_NO_ERROR - no errors; browsing is not done yet
- CHIP_END_OF_INPUT - no errors; end of browsing
- other - browsing error code

* Fix DNS-SD unit test so it can resolve more than one address

* Adopt OpenThread browsing to end-of-browse notification

This will fix double-free error when more than one service was
discovered by the OpenThread mDNS browsing.

Author: arkq

src/include/platform/ThreadStackManager.h

@@ -69,7 +69,8 @@ class GenericThreadStackManagerImpl_FreeRTOS;
 // Declaration of callback types corresponding to DnssdResolveCallback and DnssdBrowseCallback to avoid circular including.
 using DnsResolveCallback = void (*)(void * context, chip::Dnssd::DnssdService * result, const Span<Inet::IPAddress> & addresses,
                                     CHIP_ERROR error);
-using DnsBrowseCallback  = void (*)(void * context, chip::Dnssd::DnssdService * services, size_t servicesSize, CHIP_ERROR error);
+using DnsBrowseCallback  = void (*)(void * context, chip::Dnssd::DnssdService * services, size_t servicesSize, bool finalBrowse,
+                                   CHIP_ERROR error);
 #endif // CHIP_DEVICE_CONFIG_ENABLE_THREAD_DNS_CLIENT
 
 #if CHIP_DEVICE_CONFIG_ENABLE_THREAD_SRP_CLIENT

src/lib/dnssd/Discovery_ImplPlatform.cpp

@@ -147,10 +147,16 @@ static void HandleNodeIdResolve(void * context, DnssdService * result, const Spa
     proxy->Release();
 }
 
-static void HandleNodeBrowse(void * context, DnssdService * services, size_t servicesSize, CHIP_ERROR error)
+static void HandleNodeBrowse(void * context, DnssdService * services, size_t servicesSize, bool finalBrowse, CHIP_ERROR error)
 {
     ResolverDelegateProxy * proxy = static_cast<ResolverDelegateProxy *>(context);
 
+    if (error != CHIP_NO_ERROR)
+    {
+        proxy->Release();
+        return;
+    }
+
     for (size_t i = 0; i < servicesSize; ++i)
     {
         proxy->Retain();
@@ -165,7 +171,11 @@ static void HandleNodeBrowse(void * context, DnssdService * services, size_t ser
             HandleNodeResolve(context, &services[i], Span<Inet::IPAddress>(address, 1), error);
         }
     }
-    proxy->Release();
+
+    if (finalBrowse)
+    {
+        proxy->Release();
+    }
 }
 
 CHIP_ERROR AddPtrRecord(DiscoveryFilter filter, const char ** entries, size_t & entriesCount, char * buffer, size_t bufferLen)

src/lib/dnssd/platform/Dnssd.h

@@ -87,7 +87,7 @@ struct DnssdService
  * any pointer inside this structure.
  *
  * @param[in] context     The context passed to ChipDnssdBrowse or ChipDnssdResolve.
- * @param[in] result      The mdns resolve result, can be nullptr if error
+ * @param[in] result      The mDNS resolve result, can be nullptr if error
  *                        happens.  The mAddress of this object will be ignored.
  * @param[in] addresses   IP addresses that we resolved.
  * @param[in] error       The error code.
@@ -102,13 +102,19 @@ using DnssdResolveCallback = void (*)(void * context, DnssdService * result, con
  * The callback function SHALL NOT take the ownership of the service pointer or
  * any pointer inside this structure.
  *
+ * The callback function SHALL release its internal resources only when the
+ * finalBrowse is true or when the error is not CHIP_NO_ERROR. Calling this
+ * callback function again in either case is a programming error.
+ *
  * @param[in] context       The context passed to ChipDnssdBrowse or ChipDnssdResolve.
  * @param[in] services      The service list, can be nullptr.
  * @param[in] servicesSize  The size of the service list.
+ * @param[in] finalBrowse   When true, this is the last callback for this browse.
  * @param[in] error         The error code.
  *
  */
-using DnssdBrowseCallback = void (*)(void * context, DnssdService * services, size_t servicesSize, CHIP_ERROR error);
+using DnssdBrowseCallback = void (*)(void * context, DnssdService * services, size_t servicesSize, bool finalBrowse,
+                                     CHIP_ERROR error);
 
 /**
  * The callback function for mDNS publish.
@@ -129,7 +135,7 @@ using DnssdPublishCallback = void (*)(void * context, const char * type, const c
 using DnssdAsyncReturnCallback = void (*)(void * context, CHIP_ERROR error);
 
 /**
- * This function initializes the mdns module
+ * This function initializes the mDNS module
  *
  * @param[in] initCallback    The callback for notifying the initialization result.
  * @param[in] errorCallback   The callback for notifying internal errors.
@@ -142,7 +148,7 @@ using DnssdAsyncReturnCallback = void (*)(void * context, CHIP_ERROR error);
 CHIP_ERROR ChipDnssdInit(DnssdAsyncReturnCallback initCallback, DnssdAsyncReturnCallback errorCallback, void * context);
 
 /**
- * This function shuts down the mdns module
+ * This function shuts down the mDNS module
  */
 void ChipDnssdShutdown();
 
@@ -189,7 +195,7 @@ CHIP_ERROR ChipDnssdPublishService(const DnssdService * service, DnssdPublishCal
 CHIP_ERROR ChipDnssdFinalizeServiceUpdate();
 
 /**
- * This function browses the services published by mdns
+ * This function browses the services published by mDNS
  *
  * @param[in] type        The service type.
  * @param[in] protocol    The service protocol.
@@ -207,7 +213,7 @@ CHIP_ERROR ChipDnssdBrowse(const char * type, DnssdServiceProtocol protocol, chi
                            chip::Inet::InterfaceId interface, DnssdBrowseCallback callback, void * context);
 
 /**
- * This function resolves the services published by mdns
+ * This function resolves the services published by mDNS
  *
  * @param[in] browseResult  The service entry returned by @ref ChipDnssdBrowse
  * @param[in] interface     The interface to send queries.

src/platform/Darwin/DnssdContexts.cpp

@@ -285,13 +285,13 @@ BrowseContext::BrowseContext(void * cbContext, DnssdBrowseCallback cb, DnssdServ
 void BrowseContext::DispatchFailure(DNSServiceErrorType err)
 {
     ChipLogError(Discovery, "Mdns: Browse failure (%s)", Error::ToString(err));
-    callback(context, nullptr, 0, CHIP_ERROR_INTERNAL);
+    callback(context, nullptr, 0, true, CHIP_ERROR_INTERNAL);
     MdnsContexts::GetInstance().Remove(this);
 }
 
 void BrowseContext::DispatchSuccess()
 {
-    callback(context, services.data(), services.size(), CHIP_NO_ERROR);
+    callback(context, services.data(), services.size(), true, CHIP_NO_ERROR);
     MdnsContexts::GetInstance().Remove(this);
 }
 

src/platform/ESP32/DnssdImpl.cpp

@@ -314,7 +314,7 @@ CHIP_ERROR OnBrowseDone(BrowseContext * ctx)
         }
     }
 exit:
-    ctx->mBrowseCb(ctx->mCbContext, ctx->mServices, ctx->mServiceSize, error);
+    ctx->mBrowseCb(ctx->mCbContext, ctx->mServices, ctx->mServiceSize, true, error);
     return RemoveMdnsQuery(reinterpret_cast<GenericContext *>(ctx));
 }
 

src/platform/Linux/DnssdImpl.cpp

@@ -642,7 +642,7 @@ void MdnsAvahi::HandleBrowse(AvahiServiceBrowser * browser, AvahiIfIndex interfa
         break;
     case AVAHI_BROWSER_ALL_FOR_NOW:
         ChipLogProgress(DeviceLayer, "Avahi browse: all for now");
-        context->mCallback(context->mContext, context->mServices.data(), context->mServices.size(), CHIP_NO_ERROR);
+        context->mCallback(context->mContext, context->mServices.data(), context->mServices.size(), true, CHIP_NO_ERROR);
         avahi_service_browser_free(browser);
         chip::Platform::Delete(context);
         break;

src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp

@@ -2522,15 +2522,15 @@ template <class ImplClass>
 void GenericThreadStackManagerImpl_OpenThread<ImplClass>::DispatchBrowseEmpty(intptr_t context)
 {
     auto * dnsResult = reinterpret_cast<DnsResult *>(context);
-    ThreadStackMgrImpl().mDnsBrowseCallback(dnsResult->context, nullptr, 0, dnsResult->error);
+    ThreadStackMgrImpl().mDnsBrowseCallback(dnsResult->context, nullptr, 0, true, dnsResult->error);
     Platform::Delete<DnsResult>(dnsResult);
 }
 
 template <class ImplClass>
 void GenericThreadStackManagerImpl_OpenThread<ImplClass>::DispatchBrowse(intptr_t context)
 {
     auto * dnsResult = reinterpret_cast<DnsResult *>(context);
-    ThreadStackMgrImpl().mDnsBrowseCallback(dnsResult->context, &dnsResult->mMdnsService, 1, dnsResult->error);
+    ThreadStackMgrImpl().mDnsBrowseCallback(dnsResult->context, &dnsResult->mMdnsService, 1, false, dnsResult->error);
     Platform::Delete<DnsResult>(dnsResult);
 }
 
@@ -2547,8 +2547,7 @@ void GenericThreadStackManagerImpl_OpenThread<ImplClass>::OnDnsBrowseResult(otEr
     // each entry consists of txt_entry_size (1B) + txt_entry_key + "=" + txt_entry_data
     uint8_t txtBuffer[kMaxDnsServiceTxtEntriesNumber + kTotalDnsServiceTxtBufferSize];
     otDnsServiceInfo serviceInfo;
-    uint16_t index          = 0;
-    bool wasAnythingBrowsed = false;
+    uint16_t index = 0;
 
     if (ThreadStackMgrImpl().mDnsBrowseCallback == nullptr)
     {
@@ -2574,18 +2573,16 @@ void GenericThreadStackManagerImpl_OpenThread<ImplClass>::OnDnsBrowseResult(otEr
 
         VerifyOrExit(error == CHIP_NO_ERROR, );
 
-        DnsResult * dnsResult = Platform::New<DnsResult>(aContext, MapOpenThreadError(aError));
+        DnsResult * dnsResult = Platform::New<DnsResult>(aContext, CHIP_NO_ERROR);
         error = FromOtDnsResponseToMdnsData(serviceInfo, type, dnsResult->mMdnsService, dnsResult->mServiceTxtEntry);
         if (CHIP_NO_ERROR == error)
         {
-            // Invoke callback for every service one by one instead of for the whole list due to large memory size needed to
-            // allocate on
-            // stack.
+            // Invoke callback for every service one by one instead of for the whole
+            // list due to large memory size needed to allocate on stack.
             static_assert(ArraySize(dnsResult->mMdnsService.mName) >= ArraySize(serviceName),
                           "The target buffer must be big enough");
             Platform::CopyString(dnsResult->mMdnsService.mName, serviceName);
             DeviceLayer::PlatformMgr().ScheduleWork(DispatchBrowse, reinterpret_cast<intptr_t>(dnsResult));
-            wasAnythingBrowsed = true;
         }
         else
         {
@@ -2595,13 +2592,9 @@ void GenericThreadStackManagerImpl_OpenThread<ImplClass>::OnDnsBrowseResult(otEr
     }
 
 exit:
-
-    // In case no service was found invoke callback to notify about failure. In other case it was already called before.
-    if (!wasAnythingBrowsed)
-    {
-        DnsResult * dnsResult = Platform::New<DnsResult>(aContext, MapOpenThreadError(aError));
-        DeviceLayer::PlatformMgr().ScheduleWork(DispatchBrowseEmpty, reinterpret_cast<intptr_t>(dnsResult));
-    }
+    // Invoke callback to notify about end-of-browse or failure
+    DnsResult * dnsResult = Platform::New<DnsResult>(aContext, error);
+    DeviceLayer::PlatformMgr().ScheduleWork(DispatchBrowseEmpty, reinterpret_cast<intptr_t>(dnsResult));
 }
 
 template <class ImplClass>

src/platform/Tizen/DnssdImpl.cpp

@@ -124,7 +124,7 @@ gboolean OnBrowseTimeout(void * userData)
     auto * bCtx = reinterpret_cast<BrowseContext *>(userData);
 
     bCtx->MainLoopQuit();
-    bCtx->mCallback(bCtx->mCbContext, bCtx->mServices.data(), bCtx->mServices.size(), CHIP_NO_ERROR);
+    bCtx->mCallback(bCtx->mCbContext, bCtx->mServices.data(), bCtx->mServices.size(), true, CHIP_NO_ERROR);
 
     // After this point the context might be no longer valid
     bCtx->mInstance->RemoveContext(bCtx);
@@ -215,7 +215,7 @@ void OnBrowse(dnssd_service_state_e state, dnssd_service_h service, void * data)
 
     if (ret != DNSSD_ERROR_NONE)
     {
-        bCtx->mCallback(bCtx->mCbContext, nullptr, 0, GetChipError(ret));
+        bCtx->mCallback(bCtx->mCbContext, nullptr, 0, true, GetChipError(ret));
         // After this point the context might be no longer valid
         bCtx->mInstance->RemoveContext(bCtx);
     }
@@ -614,7 +614,7 @@ CHIP_ERROR DnssdTizen::Browse(const char * type, DnssdServiceProtocol protocol,
 exit:
     if (err != CHIP_NO_ERROR)
     { // Notify caller about error
-        callback(context, nullptr, 0, err);
+        callback(context, nullptr, 0, true, err);
         RemoveContext(browseCtx);
     }
     return err;

src/platform/android/DnssdImpl.cpp

@@ -377,7 +377,7 @@ void HandleBrowse(jobjectArray instanceName, jstring serviceType, jlong callback
     const auto dispatch = [callbackHandle, contextHandle](CHIP_ERROR error, DnssdService * service = nullptr, size_t size = 0) {
         DeviceLayer::StackLock lock;
         DnssdBrowseCallback callback = reinterpret_cast<DnssdBrowseCallback>(callbackHandle);
-        callback(reinterpret_cast<void *>(contextHandle), service, size, error);
+        callback(reinterpret_cast<void *>(contextHandle), service, size, true, error);
     };
 
     JNIEnv * env = JniReferences::GetInstance().GetEnvForCurrentThread();

src/platform/tests/TestDnssd.cpp

@@ -14,6 +14,10 @@ using chip::Dnssd::DnssdService;
 using chip::Dnssd::DnssdServiceProtocol;
 using chip::Dnssd::TextEntry;
 
+static unsigned int gBrowsedServicesCount  = 0;
+static unsigned int gResolvedServicesCount = 0;
+static bool gEndOfInput                    = false;
+
 static void HandleResolve(void * context, DnssdService * result, const chip::Span<chip::Inet::IPAddress> & addresses,
                           CHIP_ERROR error)
 {
@@ -22,31 +26,45 @@ static void HandleResolve(void * context, DnssdService * result, const chip::Spa
 
     NL_TEST_ASSERT(suite, result != nullptr);
     NL_TEST_ASSERT(suite, error == CHIP_NO_ERROR);
+
     if (!addresses.empty())
     {
         addresses.data()[0].ToString(addrBuf, sizeof(addrBuf));
-        printf("Service at [%s]:%u\n", addrBuf, result->mPort);
+        printf("Service[%u] at [%s]:%u\n", gResolvedServicesCount, addrBuf, result->mPort);
     }
+
     NL_TEST_ASSERT(suite, result->mTextEntrySize == 1);
     NL_TEST_ASSERT(suite, strcmp(result->mTextEntries[0].mKey, "key") == 0);
     NL_TEST_ASSERT(suite, strcmp(reinterpret_cast<const char *>(result->mTextEntries[0].mData), "val") == 0);
 
-    chip::DeviceLayer::PlatformMgr().StopEventLoopTask();
-    chip::DeviceLayer::PlatformMgr().Shutdown();
-    exit(0);
+    if (gBrowsedServicesCount == ++gResolvedServicesCount)
+    {
+        chip::DeviceLayer::PlatformMgr().StopEventLoopTask();
+        chip::DeviceLayer::PlatformMgr().Shutdown();
+        exit(0);
+    }
 }
 
-static void HandleBrowse(void * context, DnssdService * services, size_t servicesSize, CHIP_ERROR error)
+static void HandleBrowse(void * context, DnssdService * services, size_t servicesSize, bool finalBrowse, CHIP_ERROR error)
 {
     nlTestSuite * suite = static_cast<nlTestSuite *>(context);
 
+    // Make sure that we will not be called again after end-of-input is set
+    NL_TEST_ASSERT(suite, gEndOfInput == false);
     NL_TEST_ASSERT(suite, error == CHIP_NO_ERROR);
-    if (services)
+
+    gBrowsedServicesCount += servicesSize;
+    gEndOfInput = finalBrowse;
+
+    if (servicesSize > 0)
     {
-        printf("Mdns service size %u\n", static_cast<unsigned int>(servicesSize));
-        printf("Service name %s\n", services->mName);
-        printf("Service type %s\n", services->mType);
-        NL_TEST_ASSERT(suite, ChipDnssdResolve(services, chip::Inet::InterfaceId::Null(), HandleResolve, suite) == CHIP_NO_ERROR);
+        printf("Browse mDNS service size %u\n", static_cast<unsigned int>(servicesSize));
+        for (unsigned int i = 0; i < servicesSize; i++)
+        {
+            printf("Service[%u] name %s\n", i, services[i].mName);
+            printf("Service[%u] type %s\n", i, services[i].mType);
+            NL_TEST_ASSERT(suite, ChipDnssdResolve(&services[i], services[i].mInterface, HandleResolve, suite) == CHIP_NO_ERROR);
+        }
     }
 }
 

src/platform/webos/DnssdImpl.cpp

@@ -640,7 +640,7 @@ void MdnsAvahi::HandleBrowse(AvahiServiceBrowser * browser, AvahiIfIndex interfa
         break;
     case AVAHI_BROWSER_ALL_FOR_NOW:
         ChipLogProgress(DeviceLayer, "Avahi browse: all for now");
-        context->mCallback(context->mContext, context->mServices.data(), context->mServices.size(), CHIP_NO_ERROR);
+        context->mCallback(context->mContext, context->mServices.data(), context->mServices.size(), true, CHIP_NO_ERROR);
         avahi_service_browser_free(browser);
         chip::Platform::Delete(context);
         break;