From e1d26a08fbf80c49f3b782081140c5ffd0ec0514 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 6 Feb 2025 23:00:13 -0500 Subject: [PATCH 1/6] Fix validation of return value for invokeCommands. (#37442) The response array for this case can have any number of entries, not just one. --- .../Framework/CHIP/MTRDeviceDataValidation.h | 5 ++ .../Framework/CHIP/MTRDeviceDataValidation.mm | 73 ++++++++++++------- src/darwin/Framework/CHIP/MTRDevice_XPC.mm | 2 +- .../Framework/CHIPTests/MTRDeviceTests.m | 7 ++ 4 files changed, 58 insertions(+), 29 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDeviceDataValidation.h b/src/darwin/Framework/CHIP/MTRDeviceDataValidation.h index 5d448d8aa4fcba..72ee6f4679341a 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceDataValidation.h +++ b/src/darwin/Framework/CHIP/MTRDeviceDataValidation.h @@ -39,4 +39,9 @@ MTR_EXTERN MTR_TESTABLE BOOL MTREventReportIsWellFormed(NSArray * response); +// Returns whether the provided invoke reponses actually has the right sorts of +// objects in the right places. This differes from +// MTRInvokeResponseIsWellFormed in not enforcing that there is only one response. +MTR_EXTERN MTR_TESTABLE BOOL MTRInvokeResponsesAreWellFormed(NSArray * response); + NS_ASSUME_NONNULL_END diff --git a/src/darwin/Framework/CHIP/MTRDeviceDataValidation.mm b/src/darwin/Framework/CHIP/MTRDeviceDataValidation.mm index 838f33d093e933..ba71a807170b6e 100644 --- a/src/darwin/Framework/CHIP/MTRDeviceDataValidation.mm +++ b/src/darwin/Framework/CHIP/MTRDeviceDataValidation.mm @@ -165,51 +165,68 @@ BOOL MTREventReportIsWellFormed(NSArray * repo BOOL MTRInvokeResponseIsWellFormed(NSArray * response) { - if (!MTR_SAFE_CAST(response, NSArray)) { - MTR_LOG_ERROR("Invoke response is not an array: %@", response); + if (!MTRInvokeResponsesAreWellFormed(response)) { return NO; } - // Input is an array with a single value that must have MTRCommandPathKey. + // Input is an array with a single value. if (response.count != 1) { MTR_LOG_ERROR("Invoke response is not an array with exactly one entry: %@", response); return NO; } - MTRDeviceResponseValueDictionary responseValue = response[0]; + return YES; +} - if (!MTR_SAFE_CAST(responseValue, NSDictionary) || !MTR_SAFE_CAST(responseValue[MTRCommandPathKey], MTRCommandPath)) { - MTR_LOG_ERROR("Invoke response is not an array with the right things in it: %@", response); +BOOL MTRInvokeResponsesAreWellFormed(NSArray * response) +{ + if (!MTR_SAFE_CAST(response, NSArray)) { + MTR_LOG_ERROR("Invoke response is not an array: %@", response); return NO; } - MTRDeviceDataValueDictionary _Nullable data = responseValue[MTRDataKey]; - NSError * _Nullable error = responseValue[MTRErrorKey]; + for (MTRDeviceResponseValueDictionary responseValue in response) { + // Each entry must be a dictionary that has MTRCommandPathKey. - if (data != nil && error != nil) { - MTR_LOG_ERROR("Invoke response claims to have both data and error: %@", responseValue); - return NO; - } + if (!MTR_SAFE_CAST(responseValue, NSDictionary) || !MTR_SAFE_CAST(responseValue[MTRCommandPathKey], MTRCommandPath)) { + MTR_LOG_ERROR("Invoke response has an invalid array entry: %@", responseValue); + return NO; + } - if (error != nil) { - return MTR_SAFE_CAST(error, NSError) != nil; - } + MTRDeviceDataValueDictionary _Nullable data = responseValue[MTRDataKey]; + NSError * _Nullable error = responseValue[MTRErrorKey]; - if (data == nil) { - // This is valid: indicates a success status response. - return YES; - } + if (data != nil && error != nil) { + MTR_LOG_ERROR("Invoke response claims to have both data and error: %@", responseValue); + return NO; + } - if (!MTRDataValueDictionaryIsWellFormed(data)) { - MTR_LOG_ERROR("Invoke response claims to have data that is not a data-value: %@", data); - return NO; - } + if (error != nil) { + if (!MTR_SAFE_CAST(error, NSError)) { + MTR_LOG_ERROR("Invoke response %@ has %@ instead of an NSError", responseValue, error); + return NO; + } - // Now we know data is a dictionary (in fact a data-value). The only thing - // we promise about it is that it has type MTRStructureValueType. - if (![MTRStructureValueType isEqual:data[MTRTypeKey]]) { - MTR_LOG_ERROR("Invoke response data is not of structure type: %@", data); - return NO; + // Valid error response. + continue; + } + + if (data == nil) { + // This is valid: indicates a success status response. + continue; + } + + if (!MTRDataValueDictionaryIsWellFormed(data)) { + MTR_LOG_ERROR("Invoke response claims to have data that is not a data-value: %@", data); + return NO; + } + + // Now we know data is a dictionary (in fact a data-value). The only thing + // we promise about it is that it has type MTRStructureValueType. + if (![MTRStructureValueType isEqual:data[MTRTypeKey]]) { + MTR_LOG_ERROR("Invoke response data is not of structure type: %@", data); + return NO; + } } return YES; diff --git a/src/darwin/Framework/CHIP/MTRDevice_XPC.mm b/src/darwin/Framework/CHIP/MTRDevice_XPC.mm index 31cf08287657d7..fed4238f6b0554 100644 --- a/src/darwin/Framework/CHIP/MTRDevice_XPC.mm +++ b/src/darwin/Framework/CHIP/MTRDevice_XPC.mm @@ -488,7 +488,7 @@ - (void)invokeCommands:(NSArray *> *)c return; } - if (responses != nil && !MTRInvokeResponseIsWellFormed(responses)) { + if (responses != nil && !MTRInvokeResponsesAreWellFormed(responses)) { MTR_LOG_ERROR("%@ got invoke responses for %@ that has invalid data: %@", self, commands, responses); completion(nil, [MTRError errorForCHIPErrorCode:CHIP_ERROR_INVALID_ARGUMENT]); return; diff --git a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m index 71f6aa29fb35d2..7184e434a4eff9 100644 --- a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m +++ b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m @@ -5752,6 +5752,7 @@ - (void)test045_MTRDeviceInvokeGroups completion:^(NSArray *> * _Nullable values, NSError * _Nullable error) { XCTAssertNil(error); XCTAssertNotNil(values); + XCTAssertTrue(MTRInvokeResponsesAreWellFormed(values)); // Successful invoke is represented as a value with the relevant // command path and neither data nor error. @@ -5781,6 +5782,7 @@ - (void)test045_MTRDeviceInvokeGroups completion:^(NSArray *> * _Nullable values, NSError * _Nullable error) { XCTAssertNil(error); XCTAssertNotNil(values); + XCTAssertTrue(MTRInvokeResponsesAreWellFormed(values)); // We should not have anything for groups after the first one XCTAssertEqual(values.count, 2); @@ -5814,6 +5816,7 @@ - (void)test045_MTRDeviceInvokeGroups completion:^(NSArray *> * _Nullable values, NSError * _Nullable error) { XCTAssertNil(error); XCTAssertNotNil(values); + XCTAssertTrue(MTRInvokeResponsesAreWellFormed(values)); // We should not have anything for groups after the first one __auto_type * expectedValues = @[ @@ -5858,6 +5861,7 @@ - (void)test045_MTRDeviceInvokeGroups completion:^(NSArray *> * _Nullable values, NSError * _Nullable error) { XCTAssertNil(error); XCTAssertNotNil(values); + XCTAssertTrue(MTRInvokeResponsesAreWellFormed(values)); XCTAssertEqual(values.count, 3); @@ -5902,6 +5906,7 @@ - (void)test045_MTRDeviceInvokeGroups completion:^(NSArray *> * _Nullable values, NSError * _Nullable error) { XCTAssertNil(error); XCTAssertNotNil(values); + XCTAssertTrue(MTRInvokeResponsesAreWellFormed(values)); XCTAssertEqual(values.count, 3); @@ -5946,6 +5951,7 @@ - (void)test045_MTRDeviceInvokeGroups completion:^(NSArray *> * _Nullable values, NSError * _Nullable error) { XCTAssertNil(error); XCTAssertNotNil(values); + XCTAssertTrue(MTRInvokeResponsesAreWellFormed(values)); XCTAssertEqual(values.count, 2); @@ -5988,6 +5994,7 @@ - (void)test045_MTRDeviceInvokeGroups completion:^(NSArray *> * _Nullable values, NSError * _Nullable error) { XCTAssertNil(error); XCTAssertNotNil(values); + XCTAssertTrue(MTRInvokeResponsesAreWellFormed(values)); XCTAssertEqual(values.count, 2); From 2a77a116fe5234a14e7578933459fa76f240cd6e Mon Sep 17 00:00:00 2001 From: yunhanw-google Date: Thu, 6 Feb 2025 21:35:55 -0800 Subject: [PATCH 2/6] Revert "[Java] Delete finalize method in java (#37417)" (#37446) This reverts commit d777dce30baad3b5298346b6a1661b9a78c8a6ff. --- .../generators/java/ChipClusters_java.jinja | 23 +++++------ .../several_clusters/java/ChipClusters.java | 23 +++++------ .../chip/devicecontroller/ChipClusters.java | 23 +++++------ .../BatchInvokeCallbackJni.java | 39 +++++++++---------- .../ChipDeviceController.java | 24 +++++------- .../ExtendableInvokeCallbackJni.java | 25 ++++++------ .../GetConnectedDeviceCallbackJni.java | 26 ++++++------- .../devicecontroller/InvokeCallbackJni.java | 25 ++++++------ .../devicecontroller/ReportCallbackJni.java | 25 ++++++------ .../WriteAttributesCallbackJni.java | 25 ++++++------ 10 files changed, 110 insertions(+), 148 deletions(-) diff --git a/scripts/py_matter_idl/matter_idl/generators/java/ChipClusters_java.jinja b/scripts/py_matter_idl/matter_idl/generators/java/ChipClusters_java.jinja index dd2627221c08fa..954bd7de57dcbf 100644 --- a/scripts/py_matter_idl/matter_idl/generators/java/ChipClusters_java.jinja +++ b/scripts/py_matter_idl/matter_idl/generators/java/ChipClusters_java.jinja @@ -105,7 +105,6 @@ import chip.devicecontroller.model.NodeState; import chip.devicecontroller.model.Status; import javax.annotation.Nullable; -import java.lang.ref.Cleaner; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -170,23 +169,10 @@ public class ChipClusters { private Optional timeoutMillis = Optional.empty(); - private final Cleaner.Cleanable cleanable; - public BaseChipCluster(long devicePtr, int endpointId, long clusterId) { this.devicePtr = devicePtr; this.endpointId = endpointId; this.clusterId = clusterId; - - this.cleanable = - Cleaner.create() - .register( - this, - () -> { - if (chipClusterPtr != 0) { - deleteCluster(chipClusterPtr); - chipClusterPtr = 0; - } - }); } /** @@ -255,6 +241,15 @@ public class ChipClusters { @Deprecated public void deleteCluster(long chipClusterPtr) {} + @SuppressWarnings("deprecation") + protected void finalize() throws Throwable { + super.finalize(); + + if (chipClusterPtr != 0) { + deleteCluster(chipClusterPtr); + chipClusterPtr = 0; + } + } } abstract static class ReportCallbackImpl implements ReportCallback, SubscriptionEstablishedCallback, ResubscriptionAttemptCallback { diff --git a/scripts/py_matter_idl/matter_idl/tests/outputs/several_clusters/java/ChipClusters.java b/scripts/py_matter_idl/matter_idl/tests/outputs/several_clusters/java/ChipClusters.java index 997631bb47d546..037870ee206c5a 100644 --- a/scripts/py_matter_idl/matter_idl/tests/outputs/several_clusters/java/ChipClusters.java +++ b/scripts/py_matter_idl/matter_idl/tests/outputs/several_clusters/java/ChipClusters.java @@ -28,7 +28,6 @@ import chip.devicecontroller.model.Status; import javax.annotation.Nullable; -import java.lang.ref.Cleaner; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -93,23 +92,10 @@ public static abstract class BaseChipCluster { private Optional timeoutMillis = Optional.empty(); - private final Cleaner.Cleanable cleanable; - public BaseChipCluster(long devicePtr, int endpointId, long clusterId) { this.devicePtr = devicePtr; this.endpointId = endpointId; this.clusterId = clusterId; - - this.cleanable = - Cleaner.create() - .register( - this, - () -> { - if (chipClusterPtr != 0) { - deleteCluster(chipClusterPtr); - chipClusterPtr = 0; - } - }); } /** @@ -178,6 +164,15 @@ protected void invoke( @Deprecated public void deleteCluster(long chipClusterPtr) {} + @SuppressWarnings("deprecation") + protected void finalize() throws Throwable { + super.finalize(); + + if (chipClusterPtr != 0) { + deleteCluster(chipClusterPtr); + chipClusterPtr = 0; + } + } } abstract static class ReportCallbackImpl implements ReportCallback, SubscriptionEstablishedCallback, ResubscriptionAttemptCallback { diff --git a/src/controller/java/generated/java/chip/devicecontroller/ChipClusters.java b/src/controller/java/generated/java/chip/devicecontroller/ChipClusters.java index b41b847b1eab51..da6e6e5c7d63da 100644 --- a/src/controller/java/generated/java/chip/devicecontroller/ChipClusters.java +++ b/src/controller/java/generated/java/chip/devicecontroller/ChipClusters.java @@ -28,7 +28,6 @@ import chip.devicecontroller.model.Status; import javax.annotation.Nullable; -import java.lang.ref.Cleaner; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -93,23 +92,10 @@ public static abstract class BaseChipCluster { private Optional timeoutMillis = Optional.empty(); - private final Cleaner.Cleanable cleanable; - public BaseChipCluster(long devicePtr, int endpointId, long clusterId) { this.devicePtr = devicePtr; this.endpointId = endpointId; this.clusterId = clusterId; - - this.cleanable = - Cleaner.create() - .register( - this, - () -> { - if (chipClusterPtr != 0) { - deleteCluster(chipClusterPtr); - chipClusterPtr = 0; - } - }); } /** @@ -178,6 +164,15 @@ protected void invoke( @Deprecated public void deleteCluster(long chipClusterPtr) {} + @SuppressWarnings("deprecation") + protected void finalize() throws Throwable { + super.finalize(); + + if (chipClusterPtr != 0) { + deleteCluster(chipClusterPtr); + chipClusterPtr = 0; + } + } } abstract static class ReportCallbackImpl implements ReportCallback, SubscriptionEstablishedCallback, ResubscriptionAttemptCallback { diff --git a/src/controller/java/src/chip/devicecontroller/BatchInvokeCallbackJni.java b/src/controller/java/src/chip/devicecontroller/BatchInvokeCallbackJni.java index f873ce6addc442..6982f58429a353 100644 --- a/src/controller/java/src/chip/devicecontroller/BatchInvokeCallbackJni.java +++ b/src/controller/java/src/chip/devicecontroller/BatchInvokeCallbackJni.java @@ -19,31 +19,17 @@ import chip.devicecontroller.model.InvokeResponseData; import chip.devicecontroller.model.NoInvokeResponseData; -import java.lang.ref.Cleaner; import java.util.Optional; import javax.annotation.Nullable; /** JNI wrapper callback class for {@link InvokeCallback}. */ -public final class BatchInvokeCallbackJni { - private final BatchInvokeCallbackJni wrappedBatchInvokeCallback; +public final class ExtendableInvokeCallbackJni { + private final ExtendableInvokeCallback wrappedExtendableInvokeCallback; private long callbackHandle; - private final Cleaner.Cleanable cleanable; - - public BatchInvokeCallbackJni(BatchInvokeCallback wrappedBatchInvokeCallback) { - this.wrappedBatchInvokeCallback = wrappedBatchInvokeCallback; + public ExtendableInvokeCallbackJni(ExtendableInvokeCallback wrappedExtendableInvokeCallback) { + this.wrappedExtendableInvokeCallback = wrappedExtendableInvokeCallback; this.callbackHandle = newCallback(); - - this.cleanable = - Cleaner.create() - .register( - this, - () -> { - if (chipClusterPtr != 0) { - deleteCluster(chipClusterPtr); - chipClusterPtr = 0; - } - }); } long getCallbackHandle() { @@ -55,7 +41,7 @@ long getCallbackHandle() { private native void deleteCallback(long callbackHandle); private void onError(Exception e) { - wrappedBatchInvokeCallback.onError(e); + wrappedExtendableInvokeCallback.onError(e); } private void onResponse( @@ -88,10 +74,21 @@ private void onResponse( } private void onNoResponse(int commandRef) { - wrappedBatchInvokeCallback.onNoResponse(NoInvokeResponseData.newInstance(commandRef)); + wrappedExtendableInvokeCallback.onNoResponse(NoInvokeResponseData.newInstance(commandRef)); } private void onDone() { - wrappedBatchInvokeCallback.onDone(); + wrappedExtendableInvokeCallback.onDone(); + } + + // TODO(#8578): Replace finalizer with PhantomReference. + @SuppressWarnings("deprecation") + protected void finalize() throws Throwable { + super.finalize(); + + if (callbackHandle != 0) { + deleteCallback(callbackHandle); + callbackHandle = 0; + } } } diff --git a/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java b/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java index 893307be2116bf..c3249489749110 100644 --- a/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java +++ b/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java @@ -26,7 +26,6 @@ import chip.devicecontroller.model.ChipEventPath; import chip.devicecontroller.model.DataVersionFilter; import chip.devicecontroller.model.InvokeElement; -import java.lang.ref.Cleaner; import java.time.ZoneOffset; import java.util.ArrayList; import java.util.Calendar; @@ -45,8 +44,6 @@ public class ChipDeviceController { private ScanNetworksListener scanNetworksListener; private NOCChainIssuer nocChainIssuer; - private final Cleaner.Cleanable cleanable; - /** * To load class and jni, we need to new AndroidChipPlatform after jni load but before new * ChipDeviceController @@ -70,17 +67,6 @@ public ChipDeviceController(ControllerParams params) { throw new NullPointerException("params cannot be null"); } deviceControllerPtr = newDeviceController(params); - - this.cleanable = - Cleaner.create() - .register( - this, - () -> { - if (deviceControllerPtr != 0) { - deleteDeviceController(deviceControllerPtr); - deviceControllerPtr = 0; - } - }); } public void setCompletionListener(CompletionListener listener) { @@ -1787,6 +1773,16 @@ private native void updateCommissioningICDRegistrationInfo( System.loadLibrary("CHIPController"); } + @SuppressWarnings("deprecation") + protected void finalize() throws Throwable { + super.finalize(); + + if (deviceControllerPtr != 0) { + deleteDeviceController(deviceControllerPtr); + deviceControllerPtr = 0; + } + } + /** Interface to implement custom operational credentials issuer (NOC chain generation). */ public interface NOCChainIssuer { /** diff --git a/src/controller/java/src/chip/devicecontroller/ExtendableInvokeCallbackJni.java b/src/controller/java/src/chip/devicecontroller/ExtendableInvokeCallbackJni.java index 6c12940fb6a1d5..6982f58429a353 100644 --- a/src/controller/java/src/chip/devicecontroller/ExtendableInvokeCallbackJni.java +++ b/src/controller/java/src/chip/devicecontroller/ExtendableInvokeCallbackJni.java @@ -19,7 +19,6 @@ import chip.devicecontroller.model.InvokeResponseData; import chip.devicecontroller.model.NoInvokeResponseData; -import java.lang.ref.Cleaner; import java.util.Optional; import javax.annotation.Nullable; @@ -28,22 +27,9 @@ public final class ExtendableInvokeCallbackJni { private final ExtendableInvokeCallback wrappedExtendableInvokeCallback; private long callbackHandle; - private final Cleaner.Cleanable cleanable; - public ExtendableInvokeCallbackJni(ExtendableInvokeCallback wrappedExtendableInvokeCallback) { this.wrappedExtendableInvokeCallback = wrappedExtendableInvokeCallback; this.callbackHandle = newCallback(); - - this.cleanable = - Cleaner.create() - .register( - this, - () -> { - if (callbackHandle != 0) { - deleteCallback(callbackHandle); - callbackHandle = 0; - } - }); } long getCallbackHandle() { @@ -94,4 +80,15 @@ private void onNoResponse(int commandRef) { private void onDone() { wrappedExtendableInvokeCallback.onDone(); } + + // TODO(#8578): Replace finalizer with PhantomReference. + @SuppressWarnings("deprecation") + protected void finalize() throws Throwable { + super.finalize(); + + if (callbackHandle != 0) { + deleteCallback(callbackHandle); + callbackHandle = 0; + } + } } diff --git a/src/controller/java/src/chip/devicecontroller/GetConnectedDeviceCallbackJni.java b/src/controller/java/src/chip/devicecontroller/GetConnectedDeviceCallbackJni.java index 84338f82340cd6..b45b04a778b053 100644 --- a/src/controller/java/src/chip/devicecontroller/GetConnectedDeviceCallbackJni.java +++ b/src/controller/java/src/chip/devicecontroller/GetConnectedDeviceCallbackJni.java @@ -17,29 +17,14 @@ */ package chip.devicecontroller; -import java.lang.ref.Cleaner; - /** JNI wrapper callback class for getting a connected device. */ public class GetConnectedDeviceCallbackJni { private final GetConnectedDeviceCallback wrappedCallback; private long callbackHandle; - private final Cleaner.Cleanable cleanable; - public GetConnectedDeviceCallbackJni(GetConnectedDeviceCallback wrappedCallback) { this.wrappedCallback = wrappedCallback; this.callbackHandle = newCallback(wrappedCallback); - - this.cleanable = - Cleaner.create() - .register( - this, - () -> { - if (callbackHandle != 0) { - deleteCallback(callbackHandle); - callbackHandle = 0; - } - }); } long getCallbackHandle() { @@ -50,6 +35,17 @@ long getCallbackHandle() { private native void deleteCallback(long callbackHandle); + // TODO(#8578): Replace finalizer with PhantomReference. + @SuppressWarnings("deprecation") + protected void finalize() throws Throwable { + super.finalize(); + + if (callbackHandle != 0) { + deleteCallback(callbackHandle); + callbackHandle = 0; + } + } + /** Callbacks for getting a device connected with PASE or CASE, depending on the context. */ public interface GetConnectedDeviceCallback { void onDeviceConnected(long devicePointer); diff --git a/src/controller/java/src/chip/devicecontroller/InvokeCallbackJni.java b/src/controller/java/src/chip/devicecontroller/InvokeCallbackJni.java index 69881a778481e7..ceb35eccefd9b0 100644 --- a/src/controller/java/src/chip/devicecontroller/InvokeCallbackJni.java +++ b/src/controller/java/src/chip/devicecontroller/InvokeCallbackJni.java @@ -18,29 +18,15 @@ package chip.devicecontroller; import chip.devicecontroller.model.InvokeElement; -import java.lang.ref.Cleaner; /** JNI wrapper callback class for {@link InvokeCallback}. */ public final class InvokeCallbackJni { private final InvokeCallback wrappedInvokeCallback; private long callbackHandle; - private final Cleaner.Cleanable cleanable; - public InvokeCallbackJni(InvokeCallback wrappedInvokeCallback) { this.wrappedInvokeCallback = wrappedInvokeCallback; this.callbackHandle = newCallback(); - - this.cleanable = - Cleaner.create() - .register( - this, - () -> { - if (callbackHandle != 0) { - deleteCallback(callbackHandle); - callbackHandle = 0; - } - }); } long getCallbackHandle() { @@ -69,4 +55,15 @@ private void onResponse( private void onDone() { wrappedInvokeCallback.onDone(); } + + // TODO(#8578): Replace finalizer with PhantomReference. + @SuppressWarnings("deprecation") + protected void finalize() throws Throwable { + super.finalize(); + + if (callbackHandle != 0) { + deleteCallback(callbackHandle); + callbackHandle = 0; + } + } } diff --git a/src/controller/java/src/chip/devicecontroller/ReportCallbackJni.java b/src/controller/java/src/chip/devicecontroller/ReportCallbackJni.java index 9ff86d1e9f602b..4f2529fa8d58b0 100644 --- a/src/controller/java/src/chip/devicecontroller/ReportCallbackJni.java +++ b/src/controller/java/src/chip/devicecontroller/ReportCallbackJni.java @@ -20,7 +20,6 @@ import chip.devicecontroller.model.ChipAttributePath; import chip.devicecontroller.model.ChipEventPath; import chip.devicecontroller.model.NodeState; -import java.lang.ref.Cleaner; import javax.annotation.Nullable; /** JNI wrapper callback class for {@link ReportCallback}. */ @@ -31,8 +30,6 @@ public class ReportCallbackJni { private long callbackHandle; @Nullable private NodeState nodeState; - private final Cleaner.Cleanable cleanable; - public ReportCallbackJni( @Nullable SubscriptionEstablishedCallback subscriptionEstablishedCallback, ReportCallback reportCallback, @@ -42,17 +39,6 @@ public ReportCallbackJni( this.wrappedResubscriptionAttemptCallback = resubscriptionAttemptCallback; this.callbackHandle = newCallback(subscriptionEstablishedCallback, resubscriptionAttemptCallback); - - this.cleanable = - Cleaner.create() - .register( - this, - () -> { - if (callbackHandle != 0) { - deleteCallback(callbackHandle); - callbackHandle = 0; - } - }); } long getCallbackHandle() { @@ -102,4 +88,15 @@ private void onError( private void onDone() { wrappedReportCallback.onDone(); } + + // TODO(#8578): Replace finalizer with PhantomReference. + @SuppressWarnings("deprecation") + protected void finalize() throws Throwable { + super.finalize(); + + if (callbackHandle != 0) { + deleteCallback(callbackHandle); + callbackHandle = 0; + } + } } diff --git a/src/controller/java/src/chip/devicecontroller/WriteAttributesCallbackJni.java b/src/controller/java/src/chip/devicecontroller/WriteAttributesCallbackJni.java index 84a74c4d9eec0d..7b95b30759222f 100644 --- a/src/controller/java/src/chip/devicecontroller/WriteAttributesCallbackJni.java +++ b/src/controller/java/src/chip/devicecontroller/WriteAttributesCallbackJni.java @@ -19,7 +19,6 @@ import chip.devicecontroller.model.ChipAttributePath; import chip.devicecontroller.model.Status; -import java.lang.ref.Cleaner; import javax.annotation.Nullable; /** JNI wrapper callback class for {@link WriteAttributesCallback}. */ @@ -27,22 +26,9 @@ public final class WriteAttributesCallbackJni { private final WriteAttributesCallback wrappedWriteAttributesCallback; private long callbackHandle; - private final Cleaner.Cleanable cleanable; - public WriteAttributesCallbackJni(WriteAttributesCallback wrappedWriteAttributesCallback) { this.wrappedWriteAttributesCallback = wrappedWriteAttributesCallback; this.callbackHandle = newCallback(); - - this.cleanable = - Cleaner.create() - .register( - this, - () -> { - if (callbackHandle != 0) { - deleteCallback(callbackHandle); - callbackHandle = 0; - } - }); } long getCallbackHandle() { @@ -75,4 +61,15 @@ private void onResponse( private void onDone() { wrappedWriteAttributesCallback.onDone(); } + + // TODO(#8578): Replace finalizer with PhantomReference. + @SuppressWarnings("deprecation") + protected void finalize() throws Throwable { + super.finalize(); + + if (callbackHandle != 0) { + deleteCallback(callbackHandle); + callbackHandle = 0; + } + } } From ee6341ea6a25a0aff45e1fe0b8370639e20c45d6 Mon Sep 17 00:00:00 2001 From: Shreyas Balakrishna Bhandare Date: Thu, 6 Feb 2025 23:31:47 -0800 Subject: [PATCH 3/6] Update chef air quality sample to use ug/m^3 (#37441) * Change pm10 unit from ppm to micrograms per cubic meters * Fix bug - micro is U not M --- examples/chef/common/chef-concentration-measurement.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/chef/common/chef-concentration-measurement.cpp b/examples/chef/common/chef-concentration-measurement.cpp index a23e66561d4cce..f0db3395cf4f9c 100644 --- a/examples/chef/common/chef-concentration-measurement.cpp +++ b/examples/chef/common/chef-concentration-measurement.cpp @@ -306,7 +306,7 @@ void emberAfPm1ConcentrationMeasurementClusterInitCallback(EndpointId endpoint) void emberAfPm10ConcentrationMeasurementClusterInitCallback(EndpointId endpoint) { gPm10ConcentrationMeasurementInstance[EndpointId(endpoint)] = new Instance( - EndpointId(endpoint), Pm10ConcentrationMeasurement::Id, MeasurementMediumEnum::kAir, MeasurementUnitEnum::kPpm); + EndpointId(endpoint), Pm10ConcentrationMeasurement::Id, MeasurementMediumEnum::kAir, MeasurementUnitEnum::kUgm3); gPm10ConcentrationMeasurementInstance[EndpointId(endpoint)]->Init(); gPm10ConcentrationMeasurementInstance[EndpointId(endpoint)]->SetMeasuredValue(MakeNullable(50.0f)); gPm10ConcentrationMeasurementInstance[EndpointId(endpoint)]->SetMinMeasuredValue(MakeNullable(1.0f)); From 1348a8a3201df39f37670a55cea1bcd1aa8377c1 Mon Sep 17 00:00:00 2001 From: Jeff Tung <100387939+jtung-apple@users.noreply.github.com> Date: Thu, 6 Feb 2025 23:35:29 -0800 Subject: [PATCH 4/6] [Darwin] MTRDevice should throttle deviceBecameActive callbacks (#37436) * [Darwin] MTRDevice should throttle deviceBecameActive callbacks * * Use a slightly shorter interval with its own define for deviceBecameActive throttling. * Log when we start throttling deviceBecameActive callbacks. --------- Co-authored-by: Boris Zbarsky --- .../Framework/CHIP/MTRDevice_Concrete.mm | 32 ++++++++++++++++--- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm index 2a9ef219ee0956..4e29b0e5e3d3a3 100644 --- a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm +++ b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm @@ -331,6 +331,9 @@ @interface MTRDevice_Concrete () @property (nonatomic) NSDate * estimatedStartTimeFromGeneralDiagnosticsUpTime; +@property (nonatomic) NSDate * lastDeviceBecameActiveCallbackTime; +@property (nonatomic) BOOL throttlingDeviceBecameActiveCallbacks; + /** * If currentReadClient is non-null, that means that we successfully * called SendAutoResubscribeRequest on the ReadClient and have not yet gotten @@ -470,6 +473,7 @@ - (instancetype)initWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceControlle _persistedClusters = [NSMutableSet set]; _highestObservedEventNumber = nil; _matterCPPObjectsHolder = [[MTRDeviceMatterCPPObjectsHolder alloc] init]; + _throttlingDeviceBecameActiveCallbacks = NO; // If there is a data store, make sure we have an observer to monitor system clock changes, so // NSDate-based write coalescing could be reset and not get into a bad state. @@ -864,6 +868,7 @@ - (void)_setDSTOffsets:(NSArray // subscription intervals are in seconds #define MTR_DEVICE_SUBSCRIPTION_MAX_INTERVAL_MIN (10 * 60) // 10 minutes (for now) #define MTR_DEVICE_SUBSCRIPTION_MAX_INTERVAL_MAX (60 * 60) // 60 minutes +#define MTR_DEVICE_MIN_SECONDS_BETWEEN_DEVICE_BECAME_ACTIVE_CALLBACKS (1 * 60) // 1 minute (for now) - (BOOL)_subscriptionsAllowed { @@ -1634,12 +1639,29 @@ - (void)_handleUnsolicitedMessageFromPublisher [self _changeState:MTRDeviceStateReachable]; - [self _callDelegatesWithBlock:^(id delegate) { - if ([delegate respondsToSelector:@selector(deviceBecameActive:)]) { - [delegate deviceBecameActive:self]; + // Given the framework requests a minimum subscription keep alive time of devices, this callback is not expected to happen more often than that + BOOL shouldCallDelegate = NO; + if (self.lastDeviceBecameActiveCallbackTime) { + NSTimeInterval intervalSinceLastCallback = -[self.lastDeviceBecameActiveCallbackTime timeIntervalSinceNow]; + if (intervalSinceLastCallback > MTR_DEVICE_MIN_SECONDS_BETWEEN_DEVICE_BECAME_ACTIVE_CALLBACKS) { + shouldCallDelegate = YES; } - }]; - [self _notifyDelegateOfPrivateInternalPropertiesChanges]; + } else { + shouldCallDelegate = YES; + } + + if (shouldCallDelegate) { + [self _callDelegatesWithBlock:^(id delegate) { + if ([delegate respondsToSelector:@selector(deviceBecameActive:)]) { + [delegate deviceBecameActive:self]; + } + }]; + self.lastDeviceBecameActiveCallbackTime = [NSDate now]; + self.throttlingDeviceBecameActiveCallbacks = NO; + } else if (!self.throttlingDeviceBecameActiveCallbacks) { + MTR_LOG("%@ throttling deviceBecameActive callbacks because report came in too soon after %@", self, self.lastDeviceBecameActiveCallbackTime); + self.throttlingDeviceBecameActiveCallbacks = YES; + } // in case this is called during exponential back off of subscription // reestablishment, this starts the attempt right away From c02a0913d8bf6880eb77f9d35fda9a03a2c6210d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mi=C5=82osz=20Tomkiel?= Date: Fri, 7 Feb 2025 13:45:59 +0100 Subject: [PATCH 5/6] [Tizen] Add ifdef for version-dependent Thread API (#37430) * Add ifdef * Restyled by clang-format --------- Co-authored-by: Restyled.io --- src/platform/Tizen/ThreadStackManagerImpl.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/platform/Tizen/ThreadStackManagerImpl.cpp b/src/platform/Tizen/ThreadStackManagerImpl.cpp index 0ae0f8f84ad7c8..f69974521c55b6 100644 --- a/src/platform/Tizen/ThreadStackManagerImpl.cpp +++ b/src/platform/Tizen/ThreadStackManagerImpl.cpp @@ -521,12 +521,15 @@ CHIP_ERROR ThreadStackManagerImpl::_GetThreadVersion(uint16_t & version) { VerifyOrReturnError(mIsInitialized, CHIP_ERROR_UNINITIALIZED); +#if defined(TIZEN_NETWORK_THREAD_VERSION) && TIZEN_NETWORK_THREAD_VERSION >= 0x000900 int threadErr = thread_get_version(mThreadInstance, &version); VerifyOrReturnError(threadErr == THREAD_ERROR_NONE, TizenToChipError(threadErr), ChipLogError(DeviceLayer, "FAIL: Get thread version: %s", get_error_message(threadErr))); - ChipLogProgress(DeviceLayer, "Thread version [%u]", version); return CHIP_NO_ERROR; +#else + return CHIP_ERROR_NOT_IMPLEMENTED; +#endif } CHIP_ERROR ThreadStackManagerImpl::_GetPollPeriod(uint32_t & buf) From f53be4f36646fde87cd4dd49551a002ecfb2a414 Mon Sep 17 00:00:00 2001 From: Adrian Gielniewski Date: Fri, 7 Feb 2025 15:05:03 +0100 Subject: [PATCH 6/6] [Docker Image] Add zstd to chip-build (#37450) When running workflow locally using `act`, `Save bootstrap cache` step takes significant amount of time. This is caused by missing `zstd`. Signed-off-by: Adrian Gielniewski --- integrations/docker/images/base/chip-build/Dockerfile | 1 + integrations/docker/images/base/chip-build/version | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/integrations/docker/images/base/chip-build/Dockerfile b/integrations/docker/images/base/chip-build/Dockerfile index 2a64ab7049d2be..5c154f6e82b74a 100644 --- a/integrations/docker/images/base/chip-build/Dockerfile +++ b/integrations/docker/images/base/chip-build/Dockerfile @@ -105,6 +105,7 @@ RUN set -x \ unzip \ wget \ zlib1g-dev \ + zstd \ && rm -rf /var/lib/apt/lists/ \ && git lfs install \ && : # last line diff --git a/integrations/docker/images/base/chip-build/version b/integrations/docker/images/base/chip-build/version index b0158daf675e10..d12b99744a3e54 100644 --- a/integrations/docker/images/base/chip-build/version +++ b/integrations/docker/images/base/chip-build/version @@ -1 +1 @@ -109 : [Tizen] Fix race when storing cert credentials +110 : [Tizen] Add zstd to base image