Address review comments.

Author: bzbarsky-apple

src/darwin/Framework/CHIP/MTRCallbackBridgeBase.h

@@ -215,6 +215,8 @@ class MTRCallbackBridge : public MTRCallbackBridgeBase {
         }
 
         if (!callbackBridge->mQueue) {
+            ChipLogDetail(Controller, "%s %f seconds: can't dispatch response; no queue", callbackBridge->mCookie.UTF8String,
+                -[callbackBridge->mRequestTime timeIntervalSinceNow]);
             if (!callbackBridge->mKeepAlive) {
                 delete callbackBridge;
             }

src/darwin/Framework/CHIP/MTRDeviceController.h

@@ -248,6 +248,12 @@ MTR_AVAILABLE(ios(16.1), macos(13.0), watchos(9.1), tvos(16.1))
  */
 - (void)removeServerEndpoint:(MTRServerEndpoint *)endpoint queue:(dispatch_queue_t)queue completion:(dispatch_block_t)completion MTR_NEWLY_AVAILABLE;
 
+/**
+ * Remove the given server endpoint without being notified when the removal
+ * completes.
+ */
+- (void)removeServerEndpoint:(MTRServerEndpoint *)endpoint MTR_NEWLY_AVAILABLE;
+
 /**
  * Compute a PASE verifier for the desired setup passcode.
  *

src/darwin/Framework/CHIP/MTRDeviceController.mm

@@ -981,19 +981,33 @@ - (BOOL)addServerEndpoint:(MTRServerEndpoint *)endpoint
     return YES;
 }
 
-- (void)removeServerEndpoint:(MTRServerEndpoint *)endpoint queue:(dispatch_queue_t)queue completion:(dispatch_block_t)completion MTR_NEWLY_AVAILABLE
+- (void)removeServerEndpoint:(MTRServerEndpoint *)endpoint queue:(dispatch_queue_t)queue completion:(dispatch_block_t)completion
+{
+    [self removeServerEndpointInternal:endpoint queue:queue completion:completion];
+}
+
+- (void)removeServerEndpoint:(MTRServerEndpoint *)endpoint
+{
+    [self removeServerEndpointInternal:endpoint queue:nil completion:nil];
+}
+
+- (void)removeServerEndpointInternal:(MTRServerEndpoint *)endpoint queue:(dispatch_queue_t _Nullable)queue completion:(dispatch_block_t _Nullable)completion
 {
     VerifyOrReturn([self checkIsRunning]);
 
     // We need to unhook the endpoint from the Matter side before we can start
     // tearing it down.
     [self asyncDispatchToMatterQueue:^() {
         [self removeServerEndpointOnMatterQueue:endpoint];
-        dispatch_async(queue, completion);
+        if (queue != nil && completion != nil) {
+            dispatch_async(queue, completion);
+        }
     }
         errorHandler:^(NSError * error) {
             // Error means we got shut down, so the endpoint is removed now.
-            dispatch_async(queue, completion);
+            if (queue != nil && completion != nil) {
+                dispatch_async(queue, completion);
+            }
         }];
 }
 

src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm

@@ -422,7 +422,7 @@ - (BOOL)startControllerFactory:(MTRDeviceControllerFactoryParams *)startupParams
             return;
         }
 
-        [MTRServerAccessControl init];
+        InitializeServerAccessControl();
 
         if (startupParams.hasStorage) {
             _persistentStorageDelegate = new (std::nothrow) MTRPersistentStorageDelegateBridge(startupParams.storage);

src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAccessControl.h

@@ -19,21 +19,10 @@
 
 NS_ASSUME_NONNULL_BEGIN
 
-/**
- * An access control implementation that looks at the MTRAccessGrants we have.
- */
-@interface MTRServerAccessControl : NSObject
-
-- (instancetype)init NS_UNAVAILABLE;
-+ (instancetype)new NS_UNAVAILABLE;
-+ (instancetype)alloc NS_UNAVAILABLE;
-
 /**
  * Initialize the access control module. Must be called on the Matter task
  * queue.
  */
-+ (void)init;
-
-@end
+void InitializeServerAccessControl();
 
 NS_ASSUME_NONNULL_END

src/darwin/Framework/CHIP/ServerEndpoint/MTRServerAccessControl.mm

@@ -150,8 +150,9 @@ int MatterGetAccessPrivilegeForReadAttribute(ClusterId cluster, AttributeId attr
 {
     NSNumber * _Nullable neededPrivilege = [[MTRDeviceControllerFactory sharedInstance] neededReadPrivilegeForClusterID:@(cluster) attributeID:@(attribute)];
     if (neededPrivilege == nil) {
-        // Default is View.
-        return kMatterAccessPrivilegeView;
+        // No privileges declared for this attribute on this cluster.  Treat as
+        // "needs admin privileges", so we fail closed.
+        return kMatterAccessPrivilegeAdminister;
     }
 
     switch (neededPrivilege.unsignedLongLongValue) {
@@ -182,12 +183,10 @@ int MatterGetAccessPrivilegeForWriteAttribute(ClusterId cluster, AttributeId att
     return kMatterAccessPrivilegeOperate;
 }
 
-@implementation MTRServerAccessControl
-
-+ (void)init
+void InitializeServerAccessControl()
 {
+    assertChipStackLockedByCurrentThread();
+
     // Ensure the access control bits are created.  No-op after the first call.
     gControllerAccessControl.get();
 }
-
-@end

src/darwin/Framework/CHIP/ServerEndpoint/MTRServerCluster.mm

@@ -71,32 +71,13 @@ @implementation MTRServerCluster {
     std::unique_ptr<MTRServerAttributeAccessInterface> _attributeAccessInterface;
     // We can't use something like std::unique_ptr<EmberAfAttributeMetadata[]>
     // because EmberAfAttributeMetadata does not have a default constructor, so
-    // we can't alloc and then initializer later.  And we need a contiguous
-    // buffer for all the attribute metadata, so we need to do this by hand.
-    EmberAfAttributeMetadata * _matterAttributeMetadata;
-    size_t _matterAttributeMetadataCount;
+    // we can't alloc and then initializer later.
+    std::vector<EmberAfAttributeMetadata> _matterAttributeMetadata;
 
     std::unique_ptr<CommandId[]> _matterAcceptedCommandList;
     std::unique_ptr<CommandId[]> _matterGeneratedCommandList;
 }
 
-- (void)dealloc
-{
-    [self deallocateAttributeMetadata];
-}
-
-- (void)deallocateAttributeMetadata
-{
-    if (_matterAttributeMetadata != nullptr) {
-        for (size_t i = 0; i < _matterAttributeMetadataCount; ++i) {
-            _matterAttributeMetadata[i].~EmberAfAttributeMetadata();
-        }
-        free(_matterAttributeMetadata);
-        _matterAttributeMetadata = nullptr;
-    }
-    _matterAttributeMetadataCount = 0;
-}
-
 - (nullable instancetype)initWithClusterID:(NSNumber *)clusterID revision:(NSNumber *)revision
 {
     auto clusterIDValue = clusterID.unsignedLongLongValue;
@@ -225,18 +206,13 @@ - (BOOL)addAttribute:(MTRServerAttribute *)attribute
     return YES;
 }
 
-#define MTR_DECLARE_LIST_ATTRIBUTE(attrID) \
-    DECLARE_DYNAMIC_ATTRIBUTE(attrID, ARRAY, 0, 0)
-
 static constexpr EmberAfAttributeMetadata sDescriptorAttributesMetadata[] = {
-    MTR_DECLARE_LIST_ATTRIBUTE(MTRAttributeIDTypeClusterDescriptorAttributeDeviceTypeListID),
-    MTR_DECLARE_LIST_ATTRIBUTE(MTRAttributeIDTypeClusterDescriptorAttributeServerListID),
-    MTR_DECLARE_LIST_ATTRIBUTE(MTRAttributeIDTypeClusterDescriptorAttributeClientListID),
-    MTR_DECLARE_LIST_ATTRIBUTE(MTRAttributeIDTypeClusterDescriptorAttributePartsListID),
+    DECLARE_DYNAMIC_ATTRIBUTE(MTRAttributeIDTypeClusterDescriptorAttributeDeviceTypeListID, ARRAY, 0, 0),
+    DECLARE_DYNAMIC_ATTRIBUTE(MTRAttributeIDTypeClusterDescriptorAttributeServerListID, ARRAY, 0, 0),
+    DECLARE_DYNAMIC_ATTRIBUTE(MTRAttributeIDTypeClusterDescriptorAttributeClientListID, ARRAY, 0, 0),
+    DECLARE_DYNAMIC_ATTRIBUTE(MTRAttributeIDTypeClusterDescriptorAttributePartsListID, ARRAY, 0, 0),
 };
 
-#undef MTR_DECLARE_LIST_ATTRIBUTE
-
 - (BOOL)associateWithController:(nullable MTRDeviceController *)controller
 {
     MTRDeviceController * existingController = _deviceController;
@@ -292,49 +268,40 @@ - (BOOL)associateWithController:(nullable MTRDeviceController *)controller
         return NO;
     }
 
-    _matterAttributeMetadata = static_cast<EmberAfAttributeMetadata *>(calloc(attributeCount, sizeof(EmberAfAttributeMetadata)));
-    if (_matterAttributeMetadata == nullptr) {
-        return NO;
-    }
-
-    _matterAttributeMetadataCount = attributeCount;
-
     size_t attrIndex = 0;
     for (; attrIndex < _attributes.count; ++attrIndex) {
         auto * attr = _attributes[attrIndex];
-        // Placement-new into the right slot.
-        new (&_matterAttributeMetadata[attrIndex])
-            EmberAfAttributeMetadata(DECLARE_DYNAMIC_ATTRIBUTE(static_cast<AttributeId>(attr.attributeID.unsignedLongLongValue),
-                // The type does not actually matter, since we plan to
-                // handle this entirely via AttributeAccessInterface.
-                // Claim Array because that one will keep random IM
-                // code from trying to do things with the attribute
-                // store.
-                ARRAY,
-                // Size in bytes does not matter, since we plan to
-                // handle this entirely via AttributeAccessInterface.
-                0,
-                // ATTRIBUTE_MASK_NULLABLE is not relevant because we
-                // are handling this all via AttributeAccessInterface.
-                0));
+        _matterAttributeMetadata.emplace_back(EmberAfAttributeMetadata(DECLARE_DYNAMIC_ATTRIBUTE(static_cast<AttributeId>(attr.attributeID.unsignedLongLongValue),
+            // The type does not actually matter, since we plan to
+            // handle this entirely via AttributeAccessInterface.
+            // Claim Array because that one will keep random IM
+            // code from trying to do things with the attribute
+            // store.
+            ARRAY,
+            // Size in bytes does not matter, since we plan to
+            // handle this entirely via AttributeAccessInterface.
+            0,
+            // ATTRIBUTE_MASK_NULLABLE is not relevant because we
+            // are handling this all via AttributeAccessInterface.
+            0)));
     }
 
     if (needsFeatureMap) {
-        new (&_matterAttributeMetadata[attrIndex]) EmberAfAttributeMetadata(DECLARE_DYNAMIC_ATTRIBUTE(MTRAttributeIDTypeGlobalAttributeFeatureMapID,
-            BITMAP32, 4, 0));
+        _matterAttributeMetadata.emplace_back(EmberAfAttributeMetadata(DECLARE_DYNAMIC_ATTRIBUTE(MTRAttributeIDTypeGlobalAttributeFeatureMapID,
+            BITMAP32, 4, 0)));
         ++attrIndex;
     }
 
     if (needsDescriptorAttributes) {
         for (auto & data : sDescriptorAttributesMetadata) {
-            new (&_matterAttributeMetadata[attrIndex]) EmberAfAttributeMetadata(data);
+            _matterAttributeMetadata.emplace_back(data);
             ++attrIndex;
         }
     }
 
     // Add our ClusterRevision bit.
-    new (&_matterAttributeMetadata[attrIndex]) EmberAfAttributeMetadata(DECLARE_DYNAMIC_ATTRIBUTE(MTRAttributeIDTypeGlobalAttributeClusterRevisionID,
-        INT16U, 2, 0));
+    _matterAttributeMetadata.emplace_back(EmberAfAttributeMetadata(DECLARE_DYNAMIC_ATTRIBUTE(MTRAttributeIDTypeGlobalAttributeClusterRevisionID,
+        INT16U, 2, 0)));
     ++attrIndex;
 
     _attributeAccessInterface = std::make_unique<MTRServerAttributeAccessInterface>(_parentEndpoint,
@@ -362,7 +329,7 @@ - (void)invalidate
     // queue after associateWithController succeeds, but we are no longer being
     // looked at from that queue, so it's safe to reset it here.
     _matterAccessGrants = [NSSet set];
-    [self deallocateAttributeMetadata];
+    _matterAttributeMetadata.clear();
     _attributeAccessInterface.reset();
     _matterAcceptedCommandList.reset();
     _matterGeneratedCommandList.reset();
@@ -413,7 +380,9 @@ - (void)setParentEndpoint:(EndpointId)endpoint
 
 - (Span<const EmberAfAttributeMetadata>)matterAttributeMetadata
 {
-    return Span<const EmberAfAttributeMetadata>(_matterAttributeMetadata, _matterAttributeMetadataCount);
+    // This is always called after our _matterAttributeMetadata has been set up
+    // by associateWithController.
+    return Span<const EmberAfAttributeMetadata>(_matterAttributeMetadata.data(), _matterAttributeMetadata.size());
 }
 
 - (CommandId *)matterAcceptedCommands

src/darwin/Framework/CHIP/ServerEndpoint/MTRServerEndpoint.mm

@@ -175,10 +175,10 @@ - (BOOL)addServerCluster:(MTRServerCluster *)serverCluster
     DECLARE_DYNAMIC_ATTRIBUTE(attrID, ARRAY, 0, 0)
 
 static constexpr EmberAfAttributeMetadata sDescriptorAttributesMetadata[] = {
-    MTR_DECLARE_LIST_ATTRIBUTE(MTRAttributeIDTypeClusterDescriptorAttributeDeviceTypeListID),
-    MTR_DECLARE_LIST_ATTRIBUTE(MTRAttributeIDTypeClusterDescriptorAttributeServerListID),
-    MTR_DECLARE_LIST_ATTRIBUTE(MTRAttributeIDTypeClusterDescriptorAttributeClientListID),
-    MTR_DECLARE_LIST_ATTRIBUTE(MTRAttributeIDTypeClusterDescriptorAttributePartsListID),
+    DECLARE_DYNAMIC_ATTRIBUTE(MTRAttributeIDTypeClusterDescriptorAttributeDeviceTypeListID, ARRAY, 0, 0),
+    DECLARE_DYNAMIC_ATTRIBUTE(MTRAttributeIDTypeClusterDescriptorAttributeServerListID, ARRAY, 0, 0),
+    DECLARE_DYNAMIC_ATTRIBUTE(MTRAttributeIDTypeClusterDescriptorAttributeClientListID, ARRAY, 0, 0),
+    DECLARE_DYNAMIC_ATTRIBUTE(MTRAttributeIDTypeClusterDescriptorAttributePartsListID, ARRAY, 0, 0),
     DECLARE_DYNAMIC_ATTRIBUTE(MTRAttributeIDTypeGlobalAttributeFeatureMapID, BITMAP32, 4, 0),
     DECLARE_DYNAMIC_ATTRIBUTE(MTRAttributeIDTypeGlobalAttributeClusterRevisionID, INT16U, 2, 0),
 };
@@ -257,7 +257,7 @@ - (BOOL)finishAssociationWithController:(nullable MTRDeviceController *)controll
 
         auto attrMetadata = cluster.matterAttributeMetadata;
         metadata.attributes = attrMetadata.data();
-        // This cast is safe because cluster's check for this constraint on
+        // This cast is safe because clusters check for this constraint on
         // number of attributes.
         metadata.attributeCount = static_cast<uint16_t>(attrMetadata.size());