Add an API to stop a DNS-SD browse operation. (project-chip#22823)

* Add an API to stop a DNS-SD browse operation.

Most backends don't implement this yet. Darwin does, and no longer
stops Browse operations itself.

Fixes project-chip#19194

May provide a way toward fixing
project-chip#13275

* Address review comments.

* Address more review comments.

examples/chip-tool/commands/pairing/PairingCommand.cpp

@@ -237,7 +237,8 @@ void PairingCommand::OnDiscoveredDevice(const chip::Dnssd::DiscoveredNodeData &
     nodeData.resolutionData.ipAddress[0].ToString(buf);
     ChipLogProgress(chipTool, "Discovered Device: %s:%u", buf, port);
 
-    // Stop Mdns discovery. Is it the right method ?
+    // Stop Mdns discovery.
+    CurrentCommissioner().StopCommissionableDiscovery();
     CurrentCommissioner().RegisterDeviceDiscoveryDelegate(nullptr);
 
     Inet::InterfaceId interfaceId =

src/app/tests/suites/commands/discovery/DiscoveryCommands.cpp

@@ -125,6 +125,7 @@ CHIP_ERROR DiscoveryCommands::SetupDiscoveryCommands()
 
 CHIP_ERROR DiscoveryCommands::TearDownDiscoveryCommands()
 {
+    mDNSResolver.StopDiscovery();
     mDNSResolver.SetOperationalDelegate(nullptr);
     mDNSResolver.SetCommissioningDelegate(nullptr);
     return CHIP_NO_ERROR;

src/controller/CHIPDeviceController.cpp

@@ -1431,6 +1431,11 @@ CHIP_ERROR DeviceCommissioner::DiscoverCommissionableNodes(Dnssd::DiscoveryFilte
     return mDNSResolver.DiscoverCommissionableNodes(filter);
 }
 
+CHIP_ERROR DeviceCommissioner::StopCommissionableDiscovery()
+{
+    return mDNSResolver.StopDiscovery();
+}
+
 const Dnssd::DiscoveredNodeData * DeviceCommissioner::GetDiscoveredDevice(int idx)
 {
     return GetDiscoveredNode(idx);

src/controller/CHIPDeviceController.h

@@ -597,6 +597,12 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
      */
     CHIP_ERROR DiscoverCommissionableNodes(Dnssd::DiscoveryFilter filter);
 
+    /**
+     * Stop commissionable discovery triggered by a previous
+     * DiscoverCommissionableNodes call.
+     */
+    CHIP_ERROR StopCommissionableDiscovery();
+
     /**
      * @brief
      *   Returns information about discovered devices.

src/controller/SetUpCodePairer.cpp

@@ -187,6 +187,8 @@ CHIP_ERROR SetUpCodePairer::StopConnectOverIP()
 
     mWaitingForDiscovery[kIPTransport] = false;
     currentFilter.type                 = Dnssd::DiscoveryFilterType::kNone;
+
+    mCommissioner->StopCommissionableDiscovery();
     return CHIP_NO_ERROR;
 }
 

src/controller/tests/TestCommissionableNodeController.cpp

@@ -41,6 +41,7 @@ class MockResolver : public Resolver
     {
         return CHIP_ERROR_NOT_IMPLEMENTED;
     }
+    CHIP_ERROR StopDiscovery() override { return CHIP_ERROR_NOT_IMPLEMENTED; }
 
     CHIP_ERROR InitStatus                  = CHIP_NO_ERROR;
     CHIP_ERROR ResolveNodeIdStatus         = CHIP_NO_ERROR;

src/lib/dnssd/ActiveResolveAttempts.cpp

@@ -79,6 +79,19 @@ void ActiveResolveAttempts::CompleteIpResolution(SerializedQNameIterator targetH
     }
 }
 
+CHIP_ERROR ActiveResolveAttempts::CompleteAllBrowses()
+{
+    for (auto & item : mRetryQueue)
+    {
+        if (item.attempt.IsBrowse())
+        {
+            item.attempt.Clear();
+        }
+    }
+
+    return CHIP_NO_ERROR;
+}
+
 void ActiveResolveAttempts::MarkPending(const chip::PeerId & peerId)
 {
     MarkPending(ScheduledAttempt(peerId, /* firstSend */ true));

src/lib/dnssd/ActiveResolveAttempts.h

@@ -204,6 +204,10 @@ class ActiveResolveAttempts
     void Complete(const chip::Dnssd::DiscoveredNodeData & data);
     void CompleteIpResolution(SerializedQNameIterator targetHostName);
 
+    /// Mark all browse-type scheduled attemptes as a success, removing them
+    /// from the internal list.
+    CHIP_ERROR CompleteAllBrowses();
+
     /// Mark that a resolution is pending, adding it to the internal list
     ///
     /// Once this complete, this peer id will be returned immediately

src/lib/dnssd/Discovery_ImplPlatform.cpp

@@ -153,6 +153,14 @@ static void HandleNodeBrowse(void * context, DnssdService * services, size_t ser
 
     if (error != CHIP_NO_ERROR)
     {
+        // We don't have access to the ResolverProxy here to clear out its
+        // mDiscoveryContext.  The underlying implementation of
+        // ChipDnssdStopBrowse needs to handle a possibly-stale reference
+        // safely, so this won't lead to crashes, but it can lead to
+        // mis-behavior if a stale mDiscoveryContext happens to match a newer
+        // browse operation.
+        //
+        // TODO: Have a way to clear that state here.
         proxy->Release();
         return;
     }
@@ -174,6 +182,14 @@ static void HandleNodeBrowse(void * context, DnssdService * services, size_t ser
 
     if (finalBrowse)
     {
+        // We don't have access to the ResolverProxy here to clear out its
+        // mDiscoveryContext.  The underlying implementation of
+        // ChipDnssdStopBrowse needs to handle a possibly-stale reference
+        // safely, so this won't lead to crashes, but it can lead to
+        // mis-behavior if a stale mDiscoveryContext happens to match a newer
+        // browse operation.
+        //
+        // TODO: Have a way to clear that state here.
         proxy->Release();
     }
 }
@@ -616,6 +632,12 @@ CHIP_ERROR DiscoveryImplPlatform::DiscoverCommissioners(DiscoveryFilter filter)
     return mResolverProxy.DiscoverCommissioners(filter);
 }
 
+CHIP_ERROR DiscoveryImplPlatform::StopDiscovery()
+{
+    ReturnErrorOnFailure(InitImpl());
+    return mResolverProxy.StopDiscovery();
+}
+
 DiscoveryImplPlatform & DiscoveryImplPlatform::GetInstance()
 {
     return sManager;
@@ -656,6 +678,8 @@ ResolverProxy::~ResolverProxy()
 
 CHIP_ERROR ResolverProxy::DiscoverCommissionableNodes(DiscoveryFilter filter)
 {
+    StopDiscovery();
+
     VerifyOrReturnError(mDelegate != nullptr, CHIP_ERROR_INCORRECT_STATE);
     mDelegate->Retain();
 
@@ -674,12 +698,17 @@ CHIP_ERROR ResolverProxy::DiscoverCommissionableNodes(DiscoveryFilter filter)
     char serviceName[kMaxCommissionableServiceNameSize];
     ReturnErrorOnFailure(MakeServiceTypeName(serviceName, sizeof(serviceName), filter, DiscoveryType::kCommissionableNode));
 
-    return ChipDnssdBrowse(serviceName, DnssdServiceProtocol::kDnssdProtocolUdp, Inet::IPAddressType::kAny,
-                           Inet::InterfaceId::Null(), HandleNodeBrowse, mDelegate);
+    intptr_t browseIdentifier;
+    ReturnErrorOnFailure(ChipDnssdBrowse(serviceName, DnssdServiceProtocol::kDnssdProtocolUdp, Inet::IPAddressType::kAny,
+                                         Inet::InterfaceId::Null(), HandleNodeBrowse, mDelegate, &browseIdentifier));
+    mDiscoveryContext.Emplace(browseIdentifier);
+    return CHIP_NO_ERROR;
 }
 
 CHIP_ERROR ResolverProxy::DiscoverCommissioners(DiscoveryFilter filter)
 {
+    StopDiscovery();
+
     VerifyOrReturnError(mDelegate != nullptr, CHIP_ERROR_INCORRECT_STATE);
     mDelegate->Retain();
 
@@ -698,8 +727,24 @@ CHIP_ERROR ResolverProxy::DiscoverCommissioners(DiscoveryFilter filter)
     char serviceName[kMaxCommissionerServiceNameSize];
     ReturnErrorOnFailure(MakeServiceTypeName(serviceName, sizeof(serviceName), filter, DiscoveryType::kCommissionerNode));
 
-    return ChipDnssdBrowse(serviceName, DnssdServiceProtocol::kDnssdProtocolUdp, Inet::IPAddressType::kAny,
-                           Inet::InterfaceId::Null(), HandleNodeBrowse, mDelegate);
+    intptr_t browseIdentifier;
+    ReturnErrorOnFailure(ChipDnssdBrowse(serviceName, DnssdServiceProtocol::kDnssdProtocolUdp, Inet::IPAddressType::kAny,
+                                         Inet::InterfaceId::Null(), HandleNodeBrowse, mDelegate, &browseIdentifier));
+    mDiscoveryContext.Emplace(browseIdentifier);
+    return CHIP_NO_ERROR;
+}
+
+CHIP_ERROR ResolverProxy::StopDiscovery()
+{
+    if (!mDiscoveryContext.HasValue())
+    {
+        // No discovery going on.
+        return CHIP_NO_ERROR;
+    }
+
+    CHIP_ERROR err = ChipDnssdStopBrowse(mDiscoveryContext.Value());
+    mDiscoveryContext.ClearValue();
+    return err;
 }
 
 } // namespace Dnssd

src/lib/dnssd/Discovery_ImplPlatform.h

@@ -57,6 +57,7 @@ class DiscoveryImplPlatform : public ServiceAdvertiser, public Resolver
     CHIP_ERROR ResolveNodeId(const PeerId & peerId, Inet::IPAddressType type) override;
     CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override;
     CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter = DiscoveryFilter()) override;
+    CHIP_ERROR StopDiscovery() override;
 
     static DiscoveryImplPlatform & GetInstance();
 

src/lib/dnssd/Resolver.h

@@ -390,6 +390,14 @@ class Resolver
      */
     virtual CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter = DiscoveryFilter()) = 0;
 
+    /**
+     * Stop discovery (of commissionable or commissioner nodes).
+     *
+     * Some back ends may not support stopping discovery, so consumers should
+     * not assume they will stop getting callbacks after calling this.
+     */
+    virtual CHIP_ERROR StopDiscovery() = 0;
+
     /**
      * Provides the system-wide implementation of the service resolver
      */

src/lib/dnssd/ResolverProxy.h

@@ -152,11 +152,16 @@ class ResolverProxy : public Resolver
     CHIP_ERROR ResolveNodeId(const PeerId & peerId, Inet::IPAddressType type) override;
     CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override;
     CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter = DiscoveryFilter()) override;
+    CHIP_ERROR StopDiscovery() override;
 
 private:
     ResolverDelegateProxy * mDelegate                            = nullptr;
     OperationalResolveDelegate * mPreInitOperationalDelegate     = nullptr;
     CommissioningResolveDelegate * mPreInitCommissioningDelegate = nullptr;
+
+    // While discovery (commissionable or commissioner) is ongoing,
+    // mDiscoveryContext may have a value to allow StopDiscovery to work.
+    Optional<intptr_t> mDiscoveryContext;
 };
 
 } // namespace Dnssd

src/lib/dnssd/Resolver_ImplMinimalMdns.cpp

@@ -281,6 +281,7 @@ class MinMdnsResolver : public Resolver, public MdnsPacketDelegate
     CHIP_ERROR ResolveNodeId(const PeerId & peerId, Inet::IPAddressType type) override;
     CHIP_ERROR DiscoverCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override;
     CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter = DiscoveryFilter()) override;
+    CHIP_ERROR StopDiscovery() override;
 
 private:
     OperationalResolveDelegate * mOperationalDelegate     = nullptr;
@@ -633,6 +634,11 @@ CHIP_ERROR MinMdnsResolver::DiscoverCommissioners(DiscoveryFilter filter)
     return BrowseNodes(DiscoveryType::kCommissionerNode, filter);
 }
 
+CHIP_ERROR MinMdnsResolver::StopDiscovery()
+{
+    return mActiveResolves.CompleteAllBrowses();
+}
+
 CHIP_ERROR MinMdnsResolver::BrowseNodes(DiscoveryType type, DiscoveryFilter filter)
 {
     mActiveResolves.MarkPending(filter, type);
@@ -712,5 +718,10 @@ CHIP_ERROR ResolverProxy::DiscoverCommissioners(DiscoveryFilter filter)
     return chip::Dnssd::Resolver::Instance().DiscoverCommissioners(filter);
 }
 
+CHIP_ERROR ResolverProxy::StopDiscovery()
+{
+    return CHIP_ERROR_NOT_IMPLEMENTED;
+}
+
 } // namespace Dnssd
 } // namespace chip

src/lib/dnssd/Resolver_ImplNone.cpp

@@ -42,6 +42,7 @@ class NoneResolver : public Resolver
         return CHIP_ERROR_NOT_IMPLEMENTED;
     }
     CHIP_ERROR DiscoverCommissioners(DiscoveryFilter filter = DiscoveryFilter()) override { return CHIP_ERROR_NOT_IMPLEMENTED; }
+    CHIP_ERROR StopDiscovery() override { return CHIP_ERROR_NOT_IMPLEMENTED; }
 };
 
 NoneResolver gResolver;
@@ -73,5 +74,10 @@ CHIP_ERROR ResolverProxy::DiscoverCommissioners(DiscoveryFilter filter)
     return CHIP_ERROR_NOT_IMPLEMENTED;
 }
 
+CHIP_ERROR ResolverProxy::StopDiscovery()
+{
+    return CHIP_ERROR_NOT_IMPLEMENTED;
+}
+
 } // namespace Dnssd
 } // namespace chip

src/lib/dnssd/platform/Dnssd.h

@@ -203,14 +203,31 @@ CHIP_ERROR ChipDnssdFinalizeServiceUpdate();
  * @param[in] interface   The interface to send queries.
  * @param[in] callback    The callback for found services.
  * @param[in] context     The user context.
+ * @param[out] browseIdentifier an identifier for this browse operation. This
+ *                              can be used to call ChipDnssdStopBrowse.  Only
+ *                              set on success.  This value remains valid until
+ *                              the browse callback is called with an error or
+ *                              is called with finalBrowse set to true.
  *
  * @retval CHIP_NO_ERROR                The browse succeeds.
  * @retval CHIP_ERROR_INVALID_ARGUMENT  The type or callback is nullptr.
  * @retval Error code                   The browse fails.
  *
  */
 CHIP_ERROR ChipDnssdBrowse(const char * type, DnssdServiceProtocol protocol, chip::Inet::IPAddressType addressType,
-                           chip::Inet::InterfaceId interface, DnssdBrowseCallback callback, void * context);
+                           chip::Inet::InterfaceId interface, DnssdBrowseCallback callback, void * context,
+                           intptr_t * browseIdentifier);
+
+/**
+ * Stop an ongoing browse, if supported by this backend.  If successful, this
+ * will trigger a final callback, with either an error status or finalBrowse set
+ * to true, to the DnssdBrowseCallback that was passed to the ChipDnssdBrowse
+ * call that handed back this browseIdentifier.
+ *
+ * @param browseIdentifier an identifier for a browse operation that came from
+ *                         ChipDnssdBrowse.
+ */
+CHIP_ERROR ChipDnssdStopBrowse(intptr_t browseIdentifier);
 
 /**
  * This function resolves the services published by mDNS

src/platform/Ameba/DnssdImpl.cpp

@@ -97,7 +97,13 @@ CHIP_ERROR ChipDnssdStopPublish()
 }
 
 CHIP_ERROR ChipDnssdBrowse(const char * type, DnssdServiceProtocol protocol, chip::Inet::IPAddressType addressType,
-                           chip::Inet::InterfaceId interface, DnssdBrowseCallback callback, void * context);
+                           chip::Inet::InterfaceId interface, DnssdBrowseCallback callback, void * context,
+                           intptr_t * browseIdentifier);
+{
+    return CHIP_ERROR_NOT_IMPLEMENTED;
+}
+
+CHIP_ERROR ChipDnssdStopBrowse(intptr_t browseIdentifier)
 {
     return CHIP_ERROR_NOT_IMPLEMENTED;
 }

src/platform/Darwin/DnssdContexts.cpp

@@ -299,6 +299,12 @@ void BrowseContext::DispatchSuccess()
     MdnsContexts::GetInstance().Remove(this);
 }
 
+void BrowseContext::DispatchPartialSuccess()
+{
+    callback(context, services.data(), services.size(), false, CHIP_NO_ERROR);
+    services.clear();
+}
+
 ResolveContext::ResolveContext(void * cbContext, DnssdResolveCallback cb, chip::Inet::IPAddressType cbAddressType)
 {
     type     = ContextType::Resolve;