Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix handling of double tracking for an object. #226

Merged
merged 5 commits into from
Apr 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkgs/leak_tracker/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
# 10.0.6

* Handle double tracking.

# 10.0.5

* Stop failing if finalization happened twice.
Expand Down
16 changes: 7 additions & 9 deletions pkgs/leak_tracker/lib/src/leak_tracking/_object_record.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More of a CL description would help clarify what this CL is doing. Is it handling tracking or just ignoring the double dispose problem? Would expect issue/118 would be closed if the design decision is to not handle the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Added comment to the issue: #118 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And added description.

return;
}
if (_gcedGcCount != null) {
throw Exception(
'The object $code should not be disposed after being GCed');
Expand All @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import 'primitives/model.dart';

@visibleForTesting
class ObjectRecordSet {
ObjectRecordSet({this.coder = standardIdentityHashCoder});
ObjectRecordSet({@visibleForTesting this.coder = standardIdentityHashCoder});

final IdentityHashCoder coder;

Expand Down Expand Up @@ -49,7 +49,7 @@ class ObjectRecordSet {
if (list.isEmpty) _records.remove(record.code);
}

ObjectRecord putIfAbsent(
({ObjectRecord record, bool wasAbsent}) putIfAbsent(
Object object,
Map<String, dynamic>? context,
PhaseSettings phase,
Expand All @@ -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(
Expand All @@ -78,7 +79,7 @@ class ObjectRecordSet {

list.add(result);
_length++;
return result;
return (record: result, wasAbsent: true);
}

int _length = 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
4 changes: 4 additions & 0 deletions pkgs/leak_tracker/lib/src/leak_tracking/leak_tracking.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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<String, dynamic>? context,
Expand Down
2 changes: 1 addition & 1 deletion pkgs/leak_tracker/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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<void>.delayed(const Duration(milliseconds: 4));
});
});

Expand Down Expand Up @@ -187,7 +196,7 @@ void main() {
tracker.startTracking(
theObject,
context: null,
trackedClass: 'trackedClass',
trackedClass: _trackedClass,
phase: const PhaseSettings(),
);
finalizerBuilder.gc(theObject);
Expand All @@ -206,7 +215,7 @@ void main() {
tracker.startTracking(
theObject,
context: null,
trackedClass: 'trackedClass',
trackedClass: _trackedClass,
phase: const PhaseSettings(),
);
tracker.dispatchDisposal(theObject, context: null);
Expand All @@ -232,7 +241,7 @@ void main() {
tracker.startTracking(
theObject,
context: null,
trackedClass: 'trackedClass',
trackedClass: _trackedClass,
phase: const PhaseSettings(),
);
tracker.dispatchDisposal(theObject, context: null);
Expand Down Expand Up @@ -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 =
Expand Down
4 changes: 4 additions & 0 deletions pkgs/leak_tracker_flutter_testing/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 3.0.6

* Upgrade leak_tracker to 10.0.6.

## 3.0.5

* Upgrade leak_tracker to 10.0.5.
Expand Down
4 changes: 2 additions & 2 deletions pkgs/leak_tracker_flutter_testing/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -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

Expand All @@ -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
Expand Down