From 96824e455e9cd2da642c15bdcf70ebb3a5590d11 Mon Sep 17 00:00:00 2001 From: Janez Troha Date: Thu, 4 Dec 2025 19:34:48 +0100 Subject: [PATCH] fix: Add mounted checks and fix memory leaks across UI components Address potential memory leaks and state-after-dispose issues: - AppProvider: Add dispose method to clean up listeners and callbacks - MapTab: Store and restore original location callback instead of nullifying - MessagesTab: Use Timer instead of Future.delayed for highlight cleanup - ConnectionDialog: Use named listener method for proper tab controller cleanup - RoomLoginSheet: Add _isDisposed flag to handle async callback race conditions - SettingsScreen: Clear location service callbacks in dispose - Various screens: Add mounted checks before setState after async operations (contacts_tab, device_config, home_screen, map_management, packet_log, sar_template_management, sar_update_sheet, permission_request_dialog) --- lib/providers/app_provider.dart | 15 +++++++ lib/screens/contacts_tab.dart | 2 + lib/screens/device_config_screen.dart | 2 + lib/screens/home_screen.dart | 1 + lib/screens/map_management_screen.dart | 1 + lib/screens/map_tab.dart | 14 +++--- lib/screens/messages_tab.dart | 8 +++- lib/screens/packet_log_screen.dart | 27 ++++++++---- .../sar_template_management_screen.dart | 2 + lib/screens/settings_screen.dart | 9 ++++ lib/widgets/connection_dialog.dart | 43 +++++++++++-------- .../contacts/direct_message_sheet.dart | 1 + lib/widgets/contacts/room_login_sheet.dart | 38 ++++++++-------- lib/widgets/messages/sar_update_sheet.dart | 3 ++ lib/widgets/permission_request_dialog.dart | 10 +++-- 15 files changed, 119 insertions(+), 57 deletions(-) diff --git a/lib/providers/app_provider.dart b/lib/providers/app_provider.dart index 06703e3..86b8c26 100644 --- a/lib/providers/app_provider.dart +++ b/lib/providers/app_provider.dart @@ -25,6 +25,8 @@ class AppProvider with ChangeNotifier { bool _isInitialized = false; bool get isInitialized => _isInitialized; + bool _isDisposed = false; + bool _isSimpleMode = true; bool get isSimpleMode => _isSimpleMode; @@ -683,4 +685,17 @@ class AppProvider with ChangeNotifier { 'sarMarkers': messagesProvider.sarMarkerStats, }; } + + @override + void dispose() { + _isDisposed = true; + // Remove connection state listener + connectionProvider.removeListener(_handleConnectionStateChange); + // Clear location service callbacks + locationTrackingService.onPositionUpdate = null; + locationTrackingService.onBroadcastSent = null; + locationTrackingService.onError = null; + locationTrackingService.onTrackingStateChanged = null; + super.dispose(); + } } diff --git a/lib/screens/contacts_tab.dart b/lib/screens/contacts_tab.dart index 4440aff..400ba37 100644 --- a/lib/screens/contacts_tab.dart +++ b/lib/screens/contacts_tab.dart @@ -51,9 +51,11 @@ class _ContactsTabState extends State { } Future _handleRefresh() async { + if (!mounted) return; final appProvider = context.read(); await appProvider.refresh(); // Also refresh location + if (!mounted) return; await _getCurrentLocation(); } diff --git a/lib/screens/device_config_screen.dart b/lib/screens/device_config_screen.dart index 1d1e758..a393415 100644 --- a/lib/screens/device_config_screen.dart +++ b/lib/screens/device_config_screen.dart @@ -347,6 +347,8 @@ class _DeviceConfigScreenState extends State { ), ); + if (!mounted) return; + setState(() { _latController.text = position.latitude.toStringAsFixed(6); _lonController.text = position.longitude.toStringAsFixed(6); diff --git a/lib/screens/home_screen.dart b/lib/screens/home_screen.dart index 3a2c59b..0a24480 100644 --- a/lib/screens/home_screen.dart +++ b/lib/screens/home_screen.dart @@ -279,6 +279,7 @@ class _HomeScreenState extends State final appProvider = context.watch(); if (_isMapEnabled != appProvider.isMapEnabled) { WidgetsBinding.instance.addPostFrameCallback((_) { + if (!mounted) return; _updateTabController(appProvider.isMapEnabled); }); } diff --git a/lib/screens/map_management_screen.dart b/lib/screens/map_management_screen.dart index c1d3fc6..f2ca5cb 100644 --- a/lib/screens/map_management_screen.dart +++ b/lib/screens/map_management_screen.dart @@ -409,6 +409,7 @@ class _MapManagementScreenState extends State { ); } } catch (e) { + if (!mounted) return; setState(() => _isLoading = false); _showError(AppLocalizations.of(context)!.clearCacheFailed(e.toString())); } diff --git a/lib/screens/map_tab.dart b/lib/screens/map_tab.dart index 9636c88..e4954d5 100644 --- a/lib/screens/map_tab.dart +++ b/lib/screens/map_tab.dart @@ -78,6 +78,9 @@ class _MapTabState extends State with AutomaticKeepAliveClientMixin { // MBTiles layers List _mbtilesLayers = []; + // Store original location callback to restore in dispose + void Function(Position)? _originalLocationCallback; + // WMS layers (Slovenian) late final MapLayer _slovenianAerialLayer; late final MapLayer _dtk25Layer; @@ -128,6 +131,7 @@ class _MapTabState extends State with AutomaticKeepAliveClientMixin { // Listen to map provider for navigation requests WidgetsBinding.instance.addPostFrameCallback((_) { + if (!mounted) return; // Check if widget is still mounted final mapProvider = context.read(); mapProvider.addListener(_handleMapNavigation); // Load WMS overlay state @@ -146,13 +150,13 @@ class _MapTabState extends State with AutomaticKeepAliveClientMixin { /// Note: LocationTrackingService is initialized and started by AppProvider /// This method only adds map-specific callbacks for rotation and UI updates void _setupLocationCallbacks() { - // Store the original callback from AppProvider - final originalCallback = _locationService.onPositionUpdate; + // Store the original callback from AppProvider to restore in dispose + _originalLocationCallback = _locationService.onPositionUpdate; // Add map-specific callback that chains with the original _locationService.onPositionUpdate = (position) { // Call original callback first (AppProvider's logging) - originalCallback?.call(position); + _originalLocationCallback?.call(position); // Then handle map-specific logic - early exit if not mounted or disposing if (!mounted || _isDisposing) { @@ -437,8 +441,8 @@ class _MapTabState extends State with AutomaticKeepAliveClientMixin { mapProvider.removeListener(_handleMapNavigation); // DO NOT stop location tracking - it's managed by AppProvider - // Just clear the map-specific callback - _locationService.onPositionUpdate = null; + // Restore the original callback instead of setting to null + _locationService.onPositionUpdate = _originalLocationCallback; _mapController.dispose(); _tileCache.dispose(); super.dispose(); diff --git a/lib/screens/messages_tab.dart b/lib/screens/messages_tab.dart index 58ff4d5..2bac1c8 100644 --- a/lib/screens/messages_tab.dart +++ b/lib/screens/messages_tab.dart @@ -1,3 +1,4 @@ +import 'dart:async'; import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; import 'package:provider/provider.dart'; @@ -33,6 +34,7 @@ class _MessagesTabState extends State { int _characterCount = 0; static const int _maxCharacters = 160; String? _highlightedMessageId; + Timer? _highlightTimer; // Timer for clearing message highlight // Message destination state String _destinationType = @@ -73,6 +75,7 @@ class _MessagesTabState extends State { @override void dispose() { + _highlightTimer?.cancel(); _textController.dispose(); _focusNode.dispose(); _scrollController.dispose(); @@ -112,8 +115,9 @@ class _MessagesTabState extends State { _highlightedMessageId = messageId; }); - // Clear highlight after 2 seconds - Future.delayed(const Duration(seconds: 2), () { + // Clear highlight after 2 seconds using a properly managed Timer + _highlightTimer?.cancel(); + _highlightTimer = Timer(const Duration(seconds: 2), () { if (mounted) { setState(() { _highlightedMessageId = null; diff --git a/lib/screens/packet_log_screen.dart b/lib/screens/packet_log_screen.dart index bd3a02e..b157601 100644 --- a/lib/screens/packet_log_screen.dart +++ b/lib/screens/packet_log_screen.dart @@ -73,10 +73,12 @@ class _PacketLogScreenState extends State { // Save to temporary file final tempDir = await getTemporaryDirectory(); + if (!context.mounted) return; final file = File('${tempDir.path}/ble_packets_${DateTime.now().millisecondsSinceEpoch}.csv'); await file.writeAsString(buffer.toString()); // Share the file + if (!context.mounted) return; await Share.shareXFiles( [XFile(file.path)], subject: 'MeshCore BLE Packet Logs', @@ -118,10 +120,12 @@ class _PacketLogScreenState extends State { // Save to temporary file final tempDir = await getTemporaryDirectory(); + if (!context.mounted) return; final file = File('${tempDir.path}/ble_packets_${DateTime.now().millisecondsSinceEpoch}.txt'); await file.writeAsString(buffer.toString()); // Share the file + if (!context.mounted) return; await Share.shareXFiles( [XFile(file.path)], subject: 'MeshCore BLE Packet Logs', @@ -147,27 +151,31 @@ class _PacketLogScreenState extends State { } void _clearLogs(BuildContext context) { + final parentContext = context; // Store parent context for setState showDialog( context: context, - builder: (context) => AlertDialog( - title: Text(AppLocalizations.of(context)!.clearAllData), + builder: (dialogContext) => AlertDialog( + title: Text(AppLocalizations.of(dialogContext)!.clearAllData), content: const Text('Are you sure you want to clear all packet logs? This cannot be undone.'), actions: [ TextButton( - onPressed: () => Navigator.pop(context), - child: Text(AppLocalizations.of(context)!.cancel), + onPressed: () => Navigator.pop(dialogContext), + child: Text(AppLocalizations.of(dialogContext)!.cancel), ), TextButton( onPressed: () { widget.bleService.clearPacketLogs(); - Navigator.pop(context); + Navigator.pop(dialogContext); + if (!mounted) return; setState(() {}); - ScaffoldMessenger.of(context).showSnackBar( - const SnackBar(content: Text('Packet logs cleared')), - ); + if (parentContext.mounted) { + ScaffoldMessenger.of(parentContext).showSnackBar( + const SnackBar(content: Text('Packet logs cleared')), + ); + } }, style: TextButton.styleFrom(foregroundColor: Colors.red), - child: Text(AppLocalizations.of(context)!.clear), + child: Text(AppLocalizations.of(dialogContext)!.clear), ), ], ), @@ -381,6 +389,7 @@ class _PacketLogScreenState extends State { // Auto-scroll to bottom if (_autoScroll && index == logs.length - 1) { WidgetsBinding.instance.addPostFrameCallback((_) { + if (!mounted) return; if (_scrollController.hasClients) { _scrollController.animateTo( _scrollController.position.maxScrollExtent, diff --git a/lib/screens/sar_template_management_screen.dart b/lib/screens/sar_template_management_screen.dart index 737f1b0..7484de5 100644 --- a/lib/screens/sar_template_management_screen.dart +++ b/lib/screens/sar_template_management_screen.dart @@ -24,6 +24,7 @@ class _SarTemplateManagementScreenState extends State _initializeService() async { if (!_templateService.isInitialized) { + if (!mounted) return; setState(() => _isLoading = true); await _templateService.initialize(); if (mounted) { @@ -111,6 +112,7 @@ class _SarTemplateManagementScreenState extends State _importFromClipboard() async { + if (!mounted) return; setState(() => _isLoading = true); try { diff --git a/lib/screens/settings_screen.dart b/lib/screens/settings_screen.dart index 41fb109..00b350c 100644 --- a/lib/screens/settings_screen.dart +++ b/lib/screens/settings_screen.dart @@ -57,6 +57,15 @@ class _SettingsScreenState extends State { _loadRxTxPreference(); } + @override + void dispose() { + // Clear location service callbacks to prevent memory leaks + _locationService.onError = null; + _locationService.onBroadcastSent = null; + _locationService.onTrackingStateChanged = null; + super.dispose(); + } + Future _loadPackageInfo() async { final info = await PackageInfo.fromPlatform(); if (mounted) { diff --git a/lib/widgets/connection_dialog.dart b/lib/widgets/connection_dialog.dart index e7efbc8..62af684 100644 --- a/lib/widgets/connection_dialog.dart +++ b/lib/widgets/connection_dialog.dart @@ -22,6 +22,26 @@ class _ConnectionDialogState extends State int _totalToScan = 0; String? _connectingToServerUrl; // Track which server is being connected to + // Named listener method for proper cleanup + void _onTabChanged() { + if (_tabController.index == 1) { + // Switched to network tab + if (_networkScanner.hasCachedResults && _discoveredServers.isEmpty) { + // Load cached results + setState(() { + _discoveredServers.addAll(_networkScanner.cachedServers); + }); + debugPrint( + '📦 [NetworkScanner] Loaded ${_discoveredServers.length} servers from cache', + ); + } else if (!_networkScanner.isScanning && + !_networkScanner.hasCachedResults) { + // No cache, start initial scan + _startNetworkScan(); + } + } + } + @override void initState() { super.initState(); @@ -55,25 +75,8 @@ class _ConnectionDialogState extends State } }; - // Listen to tab changes - _tabController.addListener(() { - if (_tabController.index == 1) { - // Switched to network tab - if (_networkScanner.hasCachedResults && _discoveredServers.isEmpty) { - // Load cached results - setState(() { - _discoveredServers.addAll(_networkScanner.cachedServers); - }); - debugPrint( - '📦 [NetworkScanner] Loaded ${_discoveredServers.length} servers from cache', - ); - } else if (!_networkScanner.isScanning && - !_networkScanner.hasCachedResults) { - // No cache, start initial scan - _startNetworkScan(); - } - } - }); + // Listen to tab changes using named method for proper cleanup + _tabController.addListener(_onTabChanged); } @override @@ -84,6 +87,8 @@ class _ConnectionDialogState extends State ); connectionProvider.stopScan(); _networkScanner.stopScan(); + // Remove listener before disposing to prevent memory leaks + _tabController.removeListener(_onTabChanged); _tabController.dispose(); super.dispose(); } diff --git a/lib/widgets/contacts/direct_message_sheet.dart b/lib/widgets/contacts/direct_message_sheet.dart index d4af9d3..928bf79 100644 --- a/lib/widgets/contacts/direct_message_sheet.dart +++ b/lib/widgets/contacts/direct_message_sheet.dart @@ -41,6 +41,7 @@ class _DirectMessageSheetState extends State { } void _updateCharacterCount() { + if (!mounted) return; setState(() { _characterCount = _textController.text.length; }); diff --git a/lib/widgets/contacts/room_login_sheet.dart b/lib/widgets/contacts/room_login_sheet.dart index d165ebe..3145d90 100644 --- a/lib/widgets/contacts/room_login_sheet.dart +++ b/lib/widgets/contacts/room_login_sheet.dart @@ -21,6 +21,7 @@ class _RoomLoginSheetState extends State { final FocusNode _focusNode = FocusNode(); bool _isLoggingIn = false; bool _obscurePassword = true; + bool _isDisposed = false; // Track disposal state for async callbacks @override void initState() { @@ -30,6 +31,7 @@ class _RoomLoginSheetState extends State { @override void dispose() { + _isDisposed = true; _passwordController.dispose(); _focusNode.dispose(); super.dispose(); @@ -245,15 +247,15 @@ class _RoomLoginSheetState extends State { ' Messages will be fetched when onMessageWaiting callback is triggered', ); - if (mounted) { - ScaffoldMessenger.of(context).showSnackBar( - SnackBar( - content: Text(AppLocalizations.of(context)!.loggedInSuccessfully), - backgroundColor: Theme.of(context).colorScheme.primary, - duration: const Duration(seconds: 3), - ), - ); - } + // Check both _isDisposed flag and mounted to handle race conditions + if (_isDisposed || !mounted) return; + ScaffoldMessenger.of(context).showSnackBar( + SnackBar( + content: Text(AppLocalizations.of(context)!.loggedInSuccessfully), + backgroundColor: Theme.of(context).colorScheme.primary, + duration: const Duration(seconds: 3), + ), + ); }; connectionProvider.onLoginFail = (publicKeyPrefix) { @@ -263,15 +265,15 @@ class _RoomLoginSheetState extends State { debugPrint('❌ [RoomLogin] Login failed - incorrect password'); - if (mounted) { - ScaffoldMessenger.of(context).showSnackBar( - SnackBar( - content: Text(AppLocalizations.of(context)!.loginFailed), - backgroundColor: Theme.of(context).colorScheme.error, - duration: const Duration(seconds: 3), - ), - ); - } + // Check both _isDisposed flag and mounted to handle race conditions + if (_isDisposed || !mounted) return; + ScaffoldMessenger.of(context).showSnackBar( + SnackBar( + content: Text(AppLocalizations.of(context)!.loginFailed), + backgroundColor: Theme.of(context).colorScheme.error, + duration: const Duration(seconds: 3), + ), + ); }; try { diff --git a/lib/widgets/messages/sar_update_sheet.dart b/lib/widgets/messages/sar_update_sheet.dart index 77dd25b..e3ac95e 100644 --- a/lib/widgets/messages/sar_update_sheet.dart +++ b/lib/widgets/messages/sar_update_sheet.dart @@ -231,6 +231,7 @@ class _SarUpdateSheetState extends State { try { // Check if location services are enabled bool serviceEnabled = await Geolocator.isLocationServiceEnabled(); + if (!mounted) return; if (!serviceEnabled) { setState(() { _locationError = 'Location services are disabled'; @@ -241,8 +242,10 @@ class _SarUpdateSheetState extends State { // Check permissions LocationPermission permission = await Geolocator.checkPermission(); + if (!mounted) return; if (permission == LocationPermission.denied) { permission = await Geolocator.requestPermission(); + if (!mounted) return; if (permission == LocationPermission.denied) { setState(() { _locationError = 'Location permission denied'; diff --git a/lib/widgets/permission_request_dialog.dart b/lib/widgets/permission_request_dialog.dart index 097daa8..ca03d15 100644 --- a/lib/widgets/permission_request_dialog.dart +++ b/lib/widgets/permission_request_dialog.dart @@ -38,6 +38,7 @@ class _PermissionRequestDialogState extends State { try { // Check if location service is enabled final serviceEnabled = await Geolocator.isLocationServiceEnabled(); + if (!mounted) return; if (!serviceEnabled) { setState(() { _errorMessage = 'Location services are disabled. Please enable location services in your device settings.'; @@ -48,10 +49,12 @@ class _PermissionRequestDialogState extends State { // Check current permission LocationPermission permission = await Geolocator.checkPermission(); + if (!mounted) return; if (permission == LocationPermission.denied) { // Request permission permission = await Geolocator.requestPermission(); + if (!mounted) return; } if (permission == LocationPermission.denied) { @@ -78,11 +81,10 @@ class _PermissionRequestDialogState extends State { }); // Close dialog and notify parent - if (mounted) { - Navigator.of(context).pop(); - widget.onPermissionsGranted(); - } + Navigator.of(context).pop(); + widget.onPermissionsGranted(); } catch (e) { + if (!mounted) return; setState(() { _errorMessage = 'Error requesting permissions: $e'; _isRequesting = false;