Skip to content

Commit

Permalink
Stop holding on to a PASE DeviceProxy pointer in MTRBaseDevice.
Browse files Browse the repository at this point in the history
Fixes #22846
  • Loading branch information
bzbarsky-apple committed Sep 23, 2022
1 parent 7189344 commit 2608c95
Show file tree
Hide file tree
Showing 5 changed files with 86 additions and 75 deletions.
53 changes: 7 additions & 46 deletions src/darwin/Framework/CHIP/MTRBaseDevice.mm
Original file line number Diff line number Diff line change
Expand Up @@ -74,15 +74,6 @@

class MTRDataValueDictionaryCallbackBridge;

@interface MTRBaseDevice ()

@property (nonatomic, readonly, assign, nullable) chip::DeviceProxy * cppPASEDevice;
@property (nonatomic, readwrite) NSMutableDictionary * reportHandlerBridges;

- (chip::NodeId)deviceID;
- (BOOL)isPASEDevice;
@end

@interface MTRReadClientContainer : NSObject
@property (nonatomic, readwrite) app::ReadClient * readClientPtr;
@property (nonatomic, readwrite) app::AttributePathParams * pathParams;
Expand Down Expand Up @@ -223,19 +214,8 @@ @implementation MTRBaseDevice
- (instancetype)initWithPASEDevice:(chip::DeviceProxy *)device controller:(MTRDeviceController *)controller
{
if (self = [super init]) {
chip::Optional<SessionHandle> session = device->GetSecureSession();
if (!session.HasValue()) {
MTR_LOG_ERROR("Failing to initialize MTRBaseDevice: no secure session");
return nil;
}

if (!session.Value()->AsSecureSession()->IsPASESession()) {
MTR_LOG_ERROR("Failing to initialize MTRBaseDevice: not a PASE session");
return nil;
}

_cppPASEDevice = device;
_nodeID = kUndefinedNodeId;
_isPASEDevice = YES;
_nodeID = device->GetDeviceId();
_deviceController = controller;
}
return self;
Expand All @@ -244,7 +224,7 @@ - (instancetype)initWithPASEDevice:(chip::DeviceProxy *)device controller:(MTRDe
- (instancetype)initWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceController *)controller
{
if (self = [super init]) {
_cppPASEDevice = nil;
_isPASEDevice = NO;
_nodeID = [nodeID unsignedLongLongValue];
_deviceController = controller;
}
Expand All @@ -258,32 +238,13 @@ + (instancetype)deviceWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceControl
return [controller baseDeviceForNodeID:nodeID];
}

- (chip::DeviceProxy * _Nullable)paseDevice
{
return _cppPASEDevice;
}

- (chip::NodeId)deviceID
{
if (_cppPASEDevice != nullptr) {
return _cppPASEDevice->GetDeviceId();
}

return self.nodeID;
}

- (BOOL)isPASEDevice
{
return _cppPASEDevice != nullptr;
}

- (void)invalidateCASESession
{
if (self.isPASEDevice) {
return;
}

[self.deviceController invalidateCASESessionForNode:self.deviceID];
[self.deviceController invalidateCASESessionForNode:self.nodeID];
}

namespace {
Expand Down Expand Up @@ -1195,7 +1156,7 @@ - (void)subscribeAttributePathWithEndpointID:(NSNumber * _Nullable)endpointID
};

MTRReadClientContainer * container = [[MTRReadClientContainer alloc] init];
container.deviceID = [self deviceID];
container.deviceID = self.nodeID;
container.pathParams = Platform::New<app::AttributePathParams>();
if (endpointID) {
container.pathParams->mEndpointId = static_cast<chip::EndpointId>([endpointID unsignedShortValue]);
Expand Down Expand Up @@ -1261,7 +1222,7 @@ - (void)deregisterReportHandlersWithQueue:(dispatch_queue_t)queue completion:(di
{
// This method must only be used for MTRDeviceOverXPC. However, for unit testing purpose, the method purges all read clients.
MTR_LOG_DEBUG("Unexpected call to deregister report handlers");
PurgeReadClientContainers([self deviceID], queue, completion);
PurgeReadClientContainers(self.nodeID, queue, completion);
}

namespace {
Expand Down Expand Up @@ -1407,7 +1368,7 @@ - (void)openCommissioningWindowWithSetupPasscode:(NSNumber *)setupPasscode
- (void)failSubscribers:(dispatch_queue_t)queue completion:(void (^)(void))completion
{
MTR_LOG_DEBUG("Causing failure in subscribers on purpose");
CauseReadClientFailure([self deviceID], queue, completion);
CauseReadClientFailure(self.nodeID, queue, completion);
}
#endif

Expand Down
24 changes: 12 additions & 12 deletions src/darwin/Framework/CHIP/MTRBaseDevice_Internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,27 +29,27 @@ NS_ASSUME_NONNULL_BEGIN

- (instancetype)initWithPASEDevice:(chip::DeviceProxy *)device controller:(MTRDeviceController *)controller;

/**
* Only returns non-nil if the device is using a PASE session. Otherwise, the
* deviceController + nodeId should be used to get a CASE session.
*/
- (chip::DeviceProxy * _Nullable)paseDevice;

/**
* Invalidate the CASE session, so an attempt to getConnectedDevice for this
* device id will have to create a new CASE session. Ideally this API will go
* away.
*/
- (void)invalidateCASESession;

/**
* Whether this device represents a PASE session or not.
*/
@property (nonatomic, assign, readonly) BOOL isPASEDevice;

/**
* Controller that that this MTRDevice was gotten from.
*/
@property (nonatomic, strong, readonly) MTRDeviceController * deviceController;

/**
* Node id for this MTRDevice. Only set to a usable value if this device
* represents a CASE session.
* Node id for this MTRDevice. If this device represents a CASE session, this
* is set to the node ID of the target node. If this device represents a PASE
* session, this is set to the device id of the PASE device.
*/
@property (nonatomic, assign, readonly) chip::NodeId nodeID;

Expand All @@ -60,10 +60,10 @@ NS_ASSUME_NONNULL_BEGIN
+ (instancetype)new NS_UNAVAILABLE;

/**
* Initialize the device object with the given node id and controller. This
* will always succeed, even if there is no such node id on the controller's
* fabric, but attempts to actually use the MTRBaseDevice will fail
* (asynchronously) in that case.
* Initialize the device object as a CASE device with the given node id and
* controller. This will always succeed, even if there is no such node id on
* the controller's fabric, but attempts to actually use the MTRBaseDevice will
* fail (asynchronously) in that case.
*/
- (instancetype)initWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceController *)controller;

Expand Down
31 changes: 17 additions & 14 deletions src/darwin/Framework/CHIP/MTRCallbackBridgeBase_internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,10 @@ template <class T> class MTRCallbackBridge {
}

/**
* Run the given MTRActionBlock on the Matter thread (possibly
* synchronously, if the MTRBaseDevice represents a PASE connection), after
* getting a secure session corresponding to the given MTRBaseDevice. On
* success, convert the success value to whatever type it needs to be to
* call the callback type we're templated over.
* Run the given MTRActionBlock on the Matter thread after getting a secure
* session corresponding to the given MTRBaseDevice. On success, convert
* the success value to whatever type it needs to be to call the callback
* type we're templated over.
*/
MTRCallbackBridge(dispatch_queue_t queue, MTRBaseDevice * device, ResponseHandler handler, MTRActionBlock action, T OnSuccessFn,
bool keepAlive)
Expand All @@ -111,23 +110,27 @@ template <class T> class MTRCallbackBridge {
, mSuccess(OnSuccessFn, this)
, mFailure(OnFailureFn, this)
{
auto * paseDevice = [device paseDevice];
if (paseDevice != nullptr) {
ActionWithDevice(paseDevice);
if (device.isPASEDevice) {
ActionWithPASEDevice(device);
} else {
ActionWithNodeID(device.nodeID, device.deviceController);
}
};

void ActionWithDevice(chip::DeviceProxy * device)
void ActionWithPASEDevice(MTRBaseDevice * device)
{
LogRequestStart();

// Keep doing dispatch_sync here for now, because we don't want device
// to become stale.
dispatch_sync(chip::DeviceLayer::PlatformMgrImpl().GetWorkQueue(), ^{
MaybeDoAction(device->GetExchangeManager(), device->GetSecureSession(), nil);
});
BOOL ok = [device.deviceController
getSessionForCommissioneeDevice:device.nodeID
completion:^(chip::Messaging::ExchangeManager * exchangeManager,
const chip::Optional<chip::SessionHandle> & session, NSError * error) {
MaybeDoAction(exchangeManager, session, error);
}];

if (ok == NO) {
OnFailureFn(this, CHIP_ERROR_INCORRECT_STATE);
}
}

void ActionWithNodeID(chip::NodeId nodeID, MTRDeviceController * controller)
Expand Down
39 changes: 36 additions & 3 deletions src/darwin/Framework/CHIP/MTRDeviceController.mm
Original file line number Diff line number Diff line change
Expand Up @@ -489,18 +489,19 @@ - (MTRBaseDevice *)deviceBeingCommissionedWithNodeID:(NSNumber *)nodeID error:(N
{
VerifyOrReturnValue([self checkIsRunning:error], nil);

__block chip::CommissioneeDeviceProxy * deviceProxy;

__block MTRBaseDevice * device;
__block BOOL success = NO;
dispatch_sync(_chipWorkQueue, ^{
VerifyOrReturn([self checkIsRunning:error]);

chip::CommissioneeDeviceProxy * deviceProxy;
auto errorCode = self->_cppCommissioner->GetDeviceBeingCommissioned([nodeID unsignedLongLongValue], &deviceProxy);
success = ![MTRDeviceController checkForError:errorCode logMsg:kErrorStopPairing error:error];
device = [[MTRBaseDevice alloc] initWithPASEDevice:deviceProxy controller:self];
});
VerifyOrReturnValue(success, nil);

return [[MTRBaseDevice alloc] initWithPASEDevice:deviceProxy controller:self];
return device;
}

- (MTRBaseDevice *)baseDeviceForNodeID:(NSNumber *)nodeID
Expand Down Expand Up @@ -704,6 +705,38 @@ - (BOOL)getSessionForNode:(chip::NodeId)nodeID completion:(MTRInternalDeviceConn
return YES;
}

- (BOOL)getSessionForCommissioneeDevice:(chip::NodeId)deviceID completion:(MTRInternalDeviceConnectionCallback)completion
{
if (![self checkIsRunning]) {
return NO;
}

dispatch_async(_chipWorkQueue, ^{
NSError * error;
if (![self checkIsRunning:&error]) {
completion(nullptr, chip::NullOptional, error);
return;
}

chip::CommissioneeDeviceProxy * deviceProxy;
CHIP_ERROR err = self->_cppCommissioner->GetDeviceBeingCommissioned(deviceID, &deviceProxy);
if (err != CHIP_NO_ERROR) {
completion(nullptr, chip::NullOptional, [MTRError errorForCHIPErrorCode:err]);
return;
}

chip::Optional<chip::SessionHandle> session = deviceProxy->GetSecureSession();
if (!session.HasValue() || !session.Value()->AsSecureSession()->IsPASESession()) {
completion(nullptr, chip::NullOptional, [MTRError errorForCHIPErrorCode:CHIP_ERROR_INCORRECT_STATE]);
return;
}

completion(deviceProxy->GetExchangeManager(), session, nil);
});

return YES;
}

- (void)asyncDispatchToMatterQueue:(void (^)(chip::Controller::DeviceCommissioner *))block
errorHandler:(void (^)(NSError *))errorHandler
{
Expand Down
14 changes: 14 additions & 0 deletions src/darwin/Framework/CHIP/MTRDeviceController_Internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,20 @@ NS_ASSUME_NONNULL_BEGIN
*/
- (BOOL)getSessionForNode:(chip::NodeId)nodeID completion:(MTRInternalDeviceConnectionCallback)completion;

/**
* Get a session for the commissionee device with the given device id. This may
* be called on any queue (including the Matter event queue) and will always
* call the provided connection callback on the Matter queue, asynchronously.
* Consumers must be prepared to run on the Matter queue (an in particular must
* not use any APIs that will try to do sync dispatch to the Matter queue).
*
* If the controller is not running when this function is called, will return NO
* and never invoke the completion. If the controller is not running when the
* async dispatch on the Matter queue would happen, an error will be dispatched
* to the completion handler.
*/
- (BOOL)getSessionForCommissioneeDevice:(chip::NodeId)deviceID completion:(MTRInternalDeviceConnectionCallback)completion;

/**
* Invalidate the CASE session for the given node ID. This is a temporary thing
* just to support MTRBaseDevice's invalidateCASESession. Must not be called on
Expand Down

0 comments on commit 2608c95

Please sign in to comment.