From 76678240e7c08efd6684d5b3a43c39f4cfbc9f8e Mon Sep 17 00:00:00 2001 From: Polina Cherkasova Date: Mon, 1 Apr 2024 09:47:13 -0700 Subject: [PATCH] Fix handling of double tracking for an object. (#226) --- pkgs/leak_tracker/CHANGELOG.md | 4 +++ .../lib/src/leak_tracking/_object_record.dart | 16 +++++----- .../src/leak_tracking/_object_record_set.dart | 11 +++---- .../src/leak_tracking/_object_tracker.dart | 4 ++- .../lib/src/leak_tracking/leak_tracking.dart | 4 +++ pkgs/leak_tracker/pubspec.yaml | 2 +- .../_object_record_set_test.dart | 17 +++++++---- .../leak_tracking/_object_tracker_test.dart | 29 ++++++++++++------- ...orm_test.dart => dart_developer_test.dart} | 0 .../leak_tracker_flutter_testing/CHANGELOG.md | 4 +++ .../leak_tracker_flutter_testing/pubspec.yaml | 4 +-- 11 files changed, 62 insertions(+), 33 deletions(-) rename pkgs/leak_tracker/test/tests/leak_tracking/{platform_test.dart => dart_developer_test.dart} (100%) diff --git a/pkgs/leak_tracker/CHANGELOG.md b/pkgs/leak_tracker/CHANGELOG.md index de205dc8..15b3feb2 100644 --- a/pkgs/leak_tracker/CHANGELOG.md +++ b/pkgs/leak_tracker/CHANGELOG.md @@ -1,3 +1,7 @@ +# 10.0.6 + +* Handle double tracking. + # 10.0.5 * Stop failing if finalization happened twice. diff --git a/pkgs/leak_tracker/lib/src/leak_tracking/_object_record.dart b/pkgs/leak_tracker/lib/src/leak_tracking/_object_record.dart index 31dd4966..78659e75 100644 --- a/pkgs/leak_tracker/lib/src/leak_tracking/_object_record.dart +++ b/pkgs/leak_tracker/lib/src/leak_tracking/_object_record.dart @@ -48,10 +48,10 @@ class ObjectRecord { int? _disposalGcCount; void setDisposed(int gcTime, DateTime time) { - // TODO(polina-c): handle double disposal in a better way - // https://github.com/dart-lang/leak_tracker/issues/118 - // Noop if object is already disposed. - if (_disposalGcCount != null) return; + if (_disposalGcCount != null) { + // It is not responsibility of leak tracker to check for double disposal. + return; + } if (_gcedGcCount != null) { throw Exception( 'The object $code should not be disposed after being GCed'); @@ -63,11 +63,9 @@ class ObjectRecord { DateTime? _gcedTime; int? _gcedGcCount; void setGCed(int gcCount, DateTime time) { - // TODO: throw exception if object is already GCed after fix of https://github.com/dart-lang/sdk/issues/55330 - // Normally it should not happen, but sometimes finalizer is called twice. - // To repro, update next line to throw exception and run flutter tests with - // the updated leak_tracker. - if (_gcedGcCount != null) return; + if (_gcedGcCount != null) { + throw Exception('$trackedClass, $code is GCed twice'); + } _gcedGcCount = gcCount; _gcedTime = time; } diff --git a/pkgs/leak_tracker/lib/src/leak_tracking/_object_record_set.dart b/pkgs/leak_tracker/lib/src/leak_tracking/_object_record_set.dart index c23013b1..7966979b 100644 --- a/pkgs/leak_tracker/lib/src/leak_tracking/_object_record_set.dart +++ b/pkgs/leak_tracker/lib/src/leak_tracking/_object_record_set.dart @@ -12,7 +12,7 @@ import 'primitives/model.dart'; @visibleForTesting class ObjectRecordSet { - ObjectRecordSet({this.coder = standardIdentityHashCoder}); + ObjectRecordSet({@visibleForTesting this.coder = standardIdentityHashCoder}); final IdentityHashCoder coder; @@ -49,7 +49,7 @@ class ObjectRecordSet { if (list.isEmpty) _records.remove(record.code); } - ObjectRecord putIfAbsent( + ({ObjectRecord record, bool wasAbsent}) putIfAbsent( Object object, Map? context, PhaseSettings phase, @@ -59,8 +59,9 @@ class ObjectRecordSet { final list = _records.putIfAbsent(code, () => []); - final existing = list.firstWhereOrNull((r) => r.ref.target == object); - if (existing != null) return existing; + final existing = + list.firstWhereOrNull((r) => identical(r.ref.target, object)); + if (existing != null) return (record: existing, wasAbsent: false); final creationChecker = phase.ignoredLeaks.createdByTestHelpers ? CreationChecker( @@ -78,7 +79,7 @@ class ObjectRecordSet { list.add(result); _length++; - return result; + return (record: result, wasAbsent: true); } int _length = 0; diff --git a/pkgs/leak_tracker/lib/src/leak_tracking/_object_tracker.dart b/pkgs/leak_tracker/lib/src/leak_tracking/_object_tracker.dart index 52aa82ce..86e4e0f5 100644 --- a/pkgs/leak_tracker/lib/src/leak_tracking/_object_tracker.dart +++ b/pkgs/leak_tracker/lib/src/leak_tracking/_object_tracker.dart @@ -62,9 +62,11 @@ class ObjectTracker implements LeakProvider { throwIfDisposed(); if (phase.ignoreLeaks) return; - final record = + final (:record, :wasAbsent) = _objects.notGCed.putIfAbsent(object, context, phase, trackedClass); + if (!wasAbsent) return; + if (phase.leakDiagnosticConfig.collectStackTraceOnStart) { record.setContext(ContextKeys.startCallstack, StackTrace.current); } diff --git a/pkgs/leak_tracker/lib/src/leak_tracking/leak_tracking.dart b/pkgs/leak_tracker/lib/src/leak_tracking/leak_tracking.dart index 74e8bc50..0f1a404e 100644 --- a/pkgs/leak_tracker/lib/src/leak_tracking/leak_tracking.dart +++ b/pkgs/leak_tracker/lib/src/leak_tracking/leak_tracking.dart @@ -114,6 +114,8 @@ abstract class LeakTracking { /// Use [context] to provide additional information, /// that may help in leak troubleshooting. /// The value must be serializable. + /// + /// Noop if object creation is already dispatched. static void dispatchObjectCreated({ required String library, required String className, @@ -138,6 +140,8 @@ abstract class LeakTracking { /// Dispatches object disposal to the leak_tracker. /// /// See [dispatchObjectCreated] for parameters documentation. + /// + /// Noop if object disposal is already dispatched. static void dispatchObjectDisposed({ required Object object, Map? context, diff --git a/pkgs/leak_tracker/pubspec.yaml b/pkgs/leak_tracker/pubspec.yaml index d6ac3442..a92d7ec3 100644 --- a/pkgs/leak_tracker/pubspec.yaml +++ b/pkgs/leak_tracker/pubspec.yaml @@ -1,5 +1,5 @@ name: leak_tracker -version: 10.0.5 +version: 10.0.6 description: A framework for memory leak tracking for Dart and Flutter applications. repository: https://github.com/dart-lang/leak_tracker/tree/main/pkgs/leak_tracker diff --git a/pkgs/leak_tracker/test/tests/leak_tracking/_object_record_set_test.dart b/pkgs/leak_tracker/test/tests/leak_tracking/_object_record_set_test.dart index 18a96bc3..6180fd31 100644 --- a/pkgs/leak_tracker/test/tests/leak_tracking/_object_record_set_test.dart +++ b/pkgs/leak_tracker/test/tests/leak_tracking/_object_record_set_test.dart @@ -46,21 +46,28 @@ void main() { ObjectRecord _addItemAndValidate(ObjectRecordSet theSet, Object item) { final length = theSet.length; - final record = theSet.putIfAbsent(item, {}, _phase, ''); + // Add first time. + final (record: record1, wasAbsent: wasAbsent1) = + theSet.putIfAbsent(item, {}, _phase, ''); expect(theSet.length, length + 1); - expect(theSet.contains(record), true); + expect(theSet.contains(record1), true); expect(theSet.contains(_record), false); + expect(wasAbsent1, true); - expect(theSet.putIfAbsent(item, {}, _phase, ''), record); + // Add second time. + final (record: record2, wasAbsent: wasAbsent2) = + theSet.putIfAbsent(item, {}, _phase, ''); + expect(identical(record1, record2), true); expect(theSet.length, length + 1); + expect(wasAbsent2, false); var count = 0; theSet.forEach((record) => count++); expect(count, theSet.length); - expect(theSet.record(item), record); + expect(theSet.record(item), record1); - return record; + return record1; } void _removeItemAndValidate(ObjectRecordSet theSet, ObjectRecord record) { diff --git a/pkgs/leak_tracker/test/tests/leak_tracking/_object_tracker_test.dart b/pkgs/leak_tracker/test/tests/leak_tracking/_object_tracker_test.dart index 013274f5..fff8251c 100644 --- a/pkgs/leak_tracker/test/tests/leak_tracking/_object_tracker_test.dart +++ b/pkgs/leak_tracker/test/tests/leak_tracking/_object_tracker_test.dart @@ -13,7 +13,7 @@ import 'package:test/test.dart'; import '../../test_infra/data/dart_classes.dart'; -const String _trackedClass = 'trackedClass'; +const String _trackedClass = 'MyClass'; const _disposalTime = Duration(milliseconds: 100); void main() { @@ -86,23 +86,32 @@ void main() { ); }); - test('without failures.', () { - final object1 = [1, 2, 3]; - final object2 = ['-']; + test('without failures', () async { + Object? object = [1, 2, 3]; + final ref = WeakReference(object); tracker.startTracking( - object1, + object, context: null, trackedClass: _trackedClass, phase: const PhaseSettings(), ); tracker.startTracking( - object2, + object, context: null, trackedClass: _trackedClass, phase: const PhaseSettings(), ); + + tracker.dispatchDisposal(object, context: null); + tracker.dispatchDisposal(object, context: null); + + object = null; + await forceGC(); + expect(ref.target, null); + // Pause to allow finalizers to run. + await Future.delayed(const Duration(milliseconds: 4)); }); }); @@ -187,7 +196,7 @@ void main() { tracker.startTracking( theObject, context: null, - trackedClass: 'trackedClass', + trackedClass: _trackedClass, phase: const PhaseSettings(), ); finalizerBuilder.gc(theObject); @@ -206,7 +215,7 @@ void main() { tracker.startTracking( theObject, context: null, - trackedClass: 'trackedClass', + trackedClass: _trackedClass, phase: const PhaseSettings(), ); tracker.dispatchDisposal(theObject, context: null); @@ -232,7 +241,7 @@ void main() { tracker.startTracking( theObject, context: null, - trackedClass: 'trackedClass', + trackedClass: _trackedClass, phase: const PhaseSettings(), ); tracker.dispatchDisposal(theObject, context: null); @@ -355,7 +364,7 @@ void main() { time = time.add(_disposalTime); gcCounter.gcCount = gcCounter.gcCount + defaultNumberOfGcCycles; - // GC and verify leak contains callstacks. + // GC and verify leak contains callstack. await withClock(Clock.fixed(time), () async { finalizerBuilder.gc(theObject); final theLeak = diff --git a/pkgs/leak_tracker/test/tests/leak_tracking/platform_test.dart b/pkgs/leak_tracker/test/tests/leak_tracking/dart_developer_test.dart similarity index 100% rename from pkgs/leak_tracker/test/tests/leak_tracking/platform_test.dart rename to pkgs/leak_tracker/test/tests/leak_tracking/dart_developer_test.dart diff --git a/pkgs/leak_tracker_flutter_testing/CHANGELOG.md b/pkgs/leak_tracker_flutter_testing/CHANGELOG.md index 56e02a5d..d8f89da8 100644 --- a/pkgs/leak_tracker_flutter_testing/CHANGELOG.md +++ b/pkgs/leak_tracker_flutter_testing/CHANGELOG.md @@ -1,3 +1,7 @@ +## 3.0.6 + +* Upgrade leak_tracker to 10.0.6. + ## 3.0.5 * Upgrade leak_tracker to 10.0.5. diff --git a/pkgs/leak_tracker_flutter_testing/pubspec.yaml b/pkgs/leak_tracker_flutter_testing/pubspec.yaml index 0c2dcc24..7fc166d5 100644 --- a/pkgs/leak_tracker_flutter_testing/pubspec.yaml +++ b/pkgs/leak_tracker_flutter_testing/pubspec.yaml @@ -1,5 +1,5 @@ name: leak_tracker_flutter_testing -version: 3.0.5 +version: 3.0.6 description: An internal package to test leak tracking with Flutter. repository: https://github.com/dart-lang/leak_tracker/tree/main/pkgs/leak_tracker_flutter_testing @@ -10,7 +10,7 @@ environment: dependencies: flutter: sdk: flutter - leak_tracker: '>=10.0.5 <11.0.0' + leak_tracker: '>=10.0.6 <11.0.0' leak_tracker_testing: '>=3.0.1 <4.0.0' matcher: ^0.12.16 meta: ^1.8.0