diff --git a/lib/features/workspace/result_grid_view.dart b/lib/features/workspace/result_grid_view.dart index 9d385f71..13a7c306 100644 --- a/lib/features/workspace/result_grid_view.dart +++ b/lib/features/workspace/result_grid_view.dart @@ -159,12 +159,20 @@ class _VirtualResultGridState extends material.State { ResultGridColumnWindow? _currentWindow; double _currentRowsViewportHeight = 0.0; + /// Handlers that live in `extension _Grid* on _VirtualResultGridState`. Tearing + /// off an extension method makes a new closure each time and two of them are + /// never equal, so `removeListener(_onPiiMaskingChanged)` would not find what + /// `addListener(_onPiiMaskingChanged)` stored (#1555). Keep one closure and + /// pass that to both. + late final void Function() _piiMaskingListener = _onPiiMaskingChanged; + late final void Function() _horizontalScrollListener = _onHorizontalScroll; + @override void initState() { super.initState(); - _horizontalController.addListener(_onHorizontalScroll); + _horizontalController.addListener(_horizontalScrollListener); _verticalController.addListener(_onVerticalScroll); - PiiMaskingController.instance.addListener(_onPiiMaskingChanged); + PiiMaskingController.instance.addListener(_piiMaskingListener); widget.stagingBuffer?.addListener(_onStagingBufferChanged); _lastKnownStagedRowCount = _baseRows.length; _updateSortedRows(); @@ -389,11 +397,11 @@ class _VirtualResultGridState extends material.State { void dispose() { _hideCellTooltip(); _selectionValuesDebounce?.cancel(); - PiiMaskingController.instance.removeListener(_onPiiMaskingChanged); + PiiMaskingController.instance.removeListener(_piiMaskingListener); _stopAutoScroll(); _rowWidgets.clear(); widget.stagingBuffer?.removeListener(_onStagingBufferChanged); - _horizontalController.removeListener(_onHorizontalScroll); + _horizontalController.removeListener(_horizontalScrollListener); _horizontalController.dispose(); _verticalController.removeListener(_onVerticalScroll); _verticalController.dispose(); diff --git a/test/features/workspace/result_grid_listener_leak_test.dart b/test/features/workspace/result_grid_listener_leak_test.dart new file mode 100644 index 00000000..9f43a0c3 --- /dev/null +++ b/test/features/workspace/result_grid_listener_leak_test.dart @@ -0,0 +1,44 @@ +import 'package:flutter/material.dart' as material; +import 'package:flutter_test/flutter_test.dart'; +import 'package:querya_desktop/core/security/pii_masking_controller.dart'; +import 'package:querya_desktop/features/workspace/result_grid_view.dart'; + +import '../../support/querya_theme_test_shell.dart'; + +material.Widget _shell({required material.Widget child}) { + return queryaThemeTestShell(child: material.Scaffold(body: child)); +} + +void main() { + group('VirtualResultGrid listeners (#1555)', () { + testWidgets('disposing a grid removes its listener from PiiMaskingController', + (tester) async { + // ignore: invalid_use_of_protected_member + final before = PiiMaskingController.instance.hasListeners; + + await tester.pumpWidget( + _shell( + child: const material.SizedBox( + width: 800, + height: 400, + child: VirtualResultGrid( + columns: ['id', 'name'], + rows: [ + ['1', 'Alice'], + ], + ), + ), + ), + ); + await tester.pumpAndSettle(); + // ignore: invalid_use_of_protected_member + expect(PiiMaskingController.instance.hasListeners, isTrue); + + await tester.pumpWidget(_shell(child: const material.SizedBox())); + await tester.pumpAndSettle(); + + // ignore: invalid_use_of_protected_member + expect(PiiMaskingController.instance.hasListeners, before); + }); + }); +} diff --git a/test/guards/no_extension_method_listeners_test.dart b/test/guards/no_extension_method_listeners_test.dart new file mode 100644 index 00000000..70eb109b --- /dev/null +++ b/test/guards/no_extension_method_listeners_test.dart @@ -0,0 +1,58 @@ +import 'dart:io'; + +import 'package:flutter_test/flutter_test.dart'; + +/// #1555: an extension method torn off twice gives two different closures, so +/// `removeListener(_onX)` never removes what `addListener(_onX)` stored and the +/// listener keeps its owner alive. Handlers passed to `addListener` / +/// `removeListener` must be class members or a stored closure. +void main() { + test('addListener / removeListener never take an extension method', () { + final extensionBody = RegExp( + r'^extension\s+\w+(?:<[^>]*>)?\s+on\s+[^{]+\{', + multiLine: true, + ); + final methodDecl = RegExp( + r'^ (?:[\w<>?,\s]+\s)?(_?\w+)\s*\([^)]*\)\s*(?:async\s*)?(?:\{|=>)', + multiLine: true, + ); + final listenerArg = RegExp(r'\b(?:add|remove)Listener\(\s*(_?\w+)\s*[,)]'); + + final extensionMethods = {}; + final files = []; + for (final f in Directory('lib').listSync(recursive: true)) { + if (f is! File || !f.path.endsWith('.dart')) continue; + files.add(f); + final src = f.readAsStringSync(); + for (final m in extensionBody.allMatches(src)) { + // Extensions are declared at the top level; their members are indented + // by two spaces up to the first closing brace in column zero. + final end = src.indexOf('\n}', m.end); + final body = src.substring(m.end, end < 0 ? src.length : end); + for (final d in methodDecl.allMatches(body)) { + extensionMethods.add(d.group(1)!); + } + } + } + + // Comments may quote the pattern they warn about. + final commentLine = RegExp(r'^\s*//.*$', multiLine: true); + + final offenders = []; + for (final f in files) { + final code = f.readAsStringSync().replaceAll(commentLine, ''); + for (final m in listenerArg.allMatches(code)) { + final name = m.group(1)!; + if (extensionMethods.contains(name)) offenders.add('${f.path}: $name'); + } + } + + expect( + offenders, + isEmpty, + reason: 'Extension methods passed to addListener / removeListener: ' + '$offenders. Declare them in the class or keep one closure in a ' + 'field.', + ); + }); +}