Fix performance when JSON is invalid (#10005) * Fixes https://github.com/flutter/devtools/issues/4172 1. **Fixed performance_model.dart:68-71**: * Updated isEmpty to verify that there is no trace binary, no frames, and no rebuild model. 2. **Guarded Perfetto trace loading in timeline_events_controller.dart:501-506**: * Avoided attempting to process track events or load a trace into Perfetto when perfettoTraceBinary is null or empty, allowing offline Flutter frames to display cleanly. 3. **Added validation in import_export.dart:72-106**: * Added null-safety check for activeScreenId with user-facing notification if missing from the snapshot. * Added a check verifying that the JSON contains data for activeScreenId (json.containsKey(activeScreenId) && json[activeScreenId] != null), preventing navigation to a broken snapshot screen and immediately notifying the user. 4. **Added error handling and notifications in offline_data.dart:173-200**: * Pushes a notification if the file data for that screen is empty (!shouldLoad(screenData)). * Wraps deserialization and loading in a try / catch block to log and display an error message if parsing fails. 5. Tests & Release Notes: * Added tests in import_export_test.dart for missing activeScreenId and missing screen data. * Added unit tests in performance_model_test.dart for OfflinePerformanceData.isEmpty. * Created offline_data_test.dart testing OfflineScreenControllerMixin loading, empty payload notifications, and error handling. * Documented the changes in NEXT_RELEASE_NOTES.md. * feedback * feedback
diff --git a/packages/devtools_app/lib/src/screens/performance/panes/timeline_events/timeline_events_controller.dart b/packages/devtools_app/lib/src/screens/performance/panes/timeline_events/timeline_events_controller.dart index c372911..043bac5 100644 --- a/packages/devtools_app/lib/src/screens/performance/panes/timeline_events/timeline_events_controller.dart +++ b/packages/devtools_app/lib/src/screens/performance/panes/timeline_events/timeline_events_controller.dart
@@ -498,11 +498,12 @@ @override Future<void> setOfflineData(OfflinePerformanceData offlineData) async { - if (offlineData.perfettoTraceBinary != null) { - _updatePerfettoTrace(offlineData.perfettoTraceBinary!); + final perfettoTraceBinary = offlineData.perfettoTraceBinary; + if (perfettoTraceBinary != null && perfettoTraceBinary.isNotEmpty) { + _updatePerfettoTrace(perfettoTraceBinary); + processTrackEvents(); + await loadPerfettoTrace(); } - processTrackEvents(); - await loadPerfettoTrace(); if (offlineData.selectedFrame != null) { perfettoController.scrollToTimeRange(
diff --git a/packages/devtools_app/lib/src/screens/performance/performance_model.dart b/packages/devtools_app/lib/src/screens/performance/performance_model.dart index 100eab0..84ce55a 100644 --- a/packages/devtools_app/lib/src/screens/performance/performance_model.dart +++ b/packages/devtools_app/lib/src/screens/performance/performance_model.dart
@@ -81,7 +81,10 @@ /// tab they exported from. final int selectedTab; - bool get isEmpty => perfettoTraceBinary == null; + bool get isEmpty => + (perfettoTraceBinary == null || perfettoTraceBinary!.isEmpty) && + frames.isEmpty && + rebuildCountModel == null; /// The JSON serializable form of this data. ///
diff --git a/packages/devtools_app/lib/src/shared/config_specific/import_export/import_export.dart b/packages/devtools_app/lib/src/shared/config_specific/import_export/import_export.dart index eed968a..2f3f0e9 100644 --- a/packages/devtools_app/lib/src/shared/config_specific/import_export/import_export.dart +++ b/packages/devtools_app/lib/src/shared/config_specific/import_export/import_export.dart
@@ -69,6 +69,14 @@ final devToolsOfflineData = _DevToolsOfflineData(json); // TODO(kenz): support imports for more than one screen at a time. final activeScreenId = devToolsOfflineData.activeScreenId; + if (activeScreenId == null) { + notificationService.push( + 'The imported file is not a valid DevTools snapshot because it does ' + 'not contain an activeScreenId field.', + ); + return; + } + if (expectedScreenId != null && activeScreenId != expectedScreenId) { notificationService.push( 'Expected a data file for screen \'$expectedScreenId\' but received one' @@ -89,6 +97,14 @@ } } + if (!devToolsOfflineData.json.containsKey(activeScreenId) || + devToolsOfflineData.json[activeScreenId] == null) { + notificationService.push( + 'The imported file does not contain data for screen \'$activeScreenId\'.', + ); + return; + } + final connectedApp = OfflineConnectedApp.parse( devToolsOfflineData.connectedApp, ); @@ -106,8 +122,10 @@ return connectedApp == null ? {} : connectedApp.cast<String, Object?>(); } - String get activeScreenId => - json[DevToolsExportKeys.activeScreenId.name] as String; + String? get activeScreenId { + final value = json[DevToolsExportKeys.activeScreenId.name]; + return value is String ? value : null; + } } enum ExportFileType {
diff --git a/packages/devtools_app/lib/src/shared/offline/offline_data.dart b/packages/devtools_app/lib/src/shared/offline/offline_data.dart index c0be579..6de315f 100644 --- a/packages/devtools_app/lib/src/shared/offline/offline_data.dart +++ b/packages/devtools_app/lib/src/shared/offline/offline_data.dart
@@ -10,6 +10,7 @@ import 'package:devtools_app_shared/service.dart'; import 'package:devtools_app_shared/utils.dart'; import 'package:flutter/foundation.dart'; +import 'package:logging/logging.dart'; import '../config_specific/import_export/import_export.dart'; import '../framework/routing.dart'; @@ -67,6 +68,8 @@ } } +final _log = Logger('offline_data'); + /// Mixin that provides offline support for a DevTools screen controller. /// /// The [Screen] that is associated with this controller must have @@ -131,6 +134,7 @@ /// ), /// } /// ``` + mixin OfflineScreenControllerMixin<T> on DevToolsScreenController, AutoDisposeControllerMixin { final _exportController = ExportController(); @@ -168,19 +172,41 @@ required FutureOr<void> Function(T data) loadData, }) async { if (offlineDataController.shouldLoadOfflineData(screenId)) { - // TODO(kenz): investigate this line of code. Do we need to be creating a - // second copy of the Map from offlineDataController.offlineDataJson or - // can we use it directly to save this `Map.of` call? - final json = Map<String, Object?>.of( - (offlineDataController.offlineDataJson[screenId] as Map) - .cast<String, Object?>(), - ); - final screenData = createData(json); + final T screenData; + try { + // TODO(kenz): investigate this line of code. Do we need to be creating a + // second copy of the Map from offlineDataController.offlineDataJson or + // can we use it directly to save this `Map.of` call? + final json = Map<String, Object?>.of( + (offlineDataController.offlineDataJson[screenId] as Map) + .cast<String, Object?>(), + ); + screenData = createData(json); + } catch (e, st) { + _log.shout('Error parsing offline data for $screenId', e, st); + notificationService.push( + 'Failed to load offline data for screen \'$screenId\': $e', + ); + return false; + } + if (shouldLoad(screenData)) { _loadingOfflineData.value = true; - await loadData(screenData); - _loadingOfflineData.value = false; - return true; + try { + await loadData(screenData); + return true; + } catch (e, st) { + _log.shout('Error loading offline data for $screenId', e, st); + notificationService.push( + 'Failed to load offline data for screen \'$screenId\': $e', + ); + } finally { + _loadingOfflineData.value = false; + } + } else { + notificationService.push( + 'The imported file does not contain any data for screen \'$screenId\'.', + ); } } return false;
diff --git a/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md b/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md index 7e9c505..6462d13 100644 --- a/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md +++ b/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md
@@ -15,8 +15,11 @@ ## General updates -* Remove the legacy Provider screen, which was a placeholder to point users to +* Removed the legacy Provider screen, which was a placeholder to point users to the `package:provider` DevTools extension. [#10046](https://github.com/flutter/devtools/pull/10046) +* Added user-facing error notifications when importing data files that are + missing required fields or contain no data for the screen. + [#10005](https://github.com/flutter/devtools/pull/10005) ## Inspector updates @@ -30,6 +33,9 @@ still be opened. [#10034](https://github.com/flutter/devtools/pull/10034), [#10010](https://github.com/flutter/devtools/issues/10010) +* Fixed an issue where importing performance data with Flutter frames but no + timeline trace would treat the data as empty. + [#10005](https://github.com/flutter/devtools/pull/10005) ## CPU profiler updates
diff --git a/packages/devtools_app/test/screens/performance/performance_model_test.dart b/packages/devtools_app/test/screens/performance/performance_model_test.dart index 93da445..9dc2a3f 100644 --- a/packages/devtools_app/test/screens/performance/performance_model_test.dart +++ b/packages/devtools_app/test/screens/performance/performance_model_test.dart
@@ -23,6 +23,25 @@ expect(offlineData.selectedTab, 0); }); + test('isEmpty', () { + expect(OfflinePerformanceData().isEmpty, isTrue); + expect( + OfflinePerformanceData(perfettoTraceBinary: Uint8List(0)).isEmpty, + isTrue, + ); + expect( + OfflinePerformanceData( + perfettoTraceBinary: Uint8List.fromList([1, 2, 3]), + ).isEmpty, + isFalse, + ); + expect(OfflinePerformanceData(frames: [testFrame0]).isEmpty, isFalse); + expect( + OfflinePerformanceData(rebuildCountModel: RebuildCountModel()).isEmpty, + isFalse, + ); + }); + test('init from parse', () { OfflinePerformanceData offlineData = OfflinePerformanceData.fromJson({}); expect(offlineData.frames, isEmpty);
diff --git a/packages/devtools_app/test/shared/import_export_test.dart b/packages/devtools_app/test/shared/import_export_test.dart index 409531b..c723bb8 100644 --- a/packages/devtools_app/test/shared/import_export_test.dart +++ b/packages/devtools_app/test/shared/import_export_test.dart
@@ -88,6 +88,46 @@ equals(attemptingToImportMessage('example')), ); }); + + test('importData pushes notification when activeScreenId is missing', () { + importController.importData(devToolsFileJsonWithoutActiveScreenId); + expect(notifications.activeMessages.length, equals(1)); + expect( + notifications.activeMessages.first.text, + equals( + 'The imported file is not a valid DevTools snapshot because it does ' + 'not contain an activeScreenId field.', + ), + ); + }); + + test( + 'importData pushes notification when activeScreenId is not a String', + () { + importController.importData( + devToolsFileJsonWithNonStringActiveScreenId, + ); + expect(notifications.activeMessages.length, equals(1)); + expect( + notifications.activeMessages.first.text, + equals( + 'The imported file is not a valid DevTools snapshot because it does ' + 'not contain an activeScreenId field.', + ), + ); + }, + ); + + test('importData pushes notification when screen data is missing', () { + importController.importData(devToolsFileJsonWithoutScreenData); + expect(notifications.activeMessages.length, equals(1)); + expect( + notifications.activeMessages.first.text, + equals( + 'The imported file does not contain data for screen \'example\'.', + ), + ); + }); }); } @@ -110,3 +150,28 @@ 'example': {'title': 'example custom tools'}, }, ); +final devToolsFileJsonWithoutActiveScreenId = DevToolsJsonFile( + name: 'devToolsFileJsonWithoutActiveScreenId', + lastModifiedTime: DateTime.fromMicrosecondsSinceEpoch(3000), + data: <String, Object?>{ + 'devToolsSnapshot': true, + 'example': {'title': 'example custom tools'}, + }, +); +final devToolsFileJsonWithNonStringActiveScreenId = DevToolsJsonFile( + name: 'devToolsFileJsonWithNonStringActiveScreenId', + lastModifiedTime: DateTime.fromMicrosecondsSinceEpoch(3500), + data: <String, Object?>{ + 'devToolsSnapshot': true, + 'activeScreenId': 12345, + 'example': {'title': 'example custom tools'}, + }, +); +final devToolsFileJsonWithoutScreenData = DevToolsJsonFile( + name: 'devToolsFileJsonWithoutScreenData', + lastModifiedTime: DateTime.fromMicrosecondsSinceEpoch(4000), + data: <String, Object?>{ + 'devToolsSnapshot': true, + 'activeScreenId': 'example', + }, +);
diff --git a/packages/devtools_app/test/shared/offline/offline_data_test.dart b/packages/devtools_app/test/shared/offline/offline_data_test.dart new file mode 100644 index 0000000..d0f08a8 --- /dev/null +++ b/packages/devtools_app/test/shared/offline/offline_data_test.dart
@@ -0,0 +1,178 @@ +// Copyright 2026 The Flutter Authors +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file or at https://developers.google.com/open-source/licenses/bsd. + +import 'package:devtools_app/devtools_app.dart'; +import 'package:devtools_app/src/shared/config_specific/import_export/import_export.dart'; +import 'package:devtools_app_shared/utils.dart'; +import 'package:devtools_test/devtools_test.dart'; +import 'package:flutter_test/flutter_test.dart'; + +class _TestScreenController extends DevToolsScreenController + with + AutoDisposeControllerMixin, + OfflineScreenControllerMixin<Map<String, Object?>> { + Map<String, Object?>? loadedData; + + @override + String get screenId => 'test_screen'; + + Future<bool> initOfflineData( + String screenId, { + bool shouldLoad = true, + bool throwOnLoad = false, + }) { + return maybeLoadOfflineData( + screenId, + createData: (json) { + if (json.containsKey('throw')) { + throw Exception('Corrupted data'); + } + return json; + }, + shouldLoad: (data) => shouldLoad && data.isNotEmpty, + loadData: (data) { + if (throwOnLoad) { + throw Exception('Failed during loadData'); + } + loadedData = data; + }, + ); + } + + @override + OfflineScreenData prepareOfflineScreenData() => + OfflineScreenData(screenId: 'test_screen', data: loadedData ?? {}); +} + +void main() { + group('OfflineScreenControllerMixin', () { + late _TestScreenController controller; + late NotificationService notifications; + + setUp(() { + notifications = NotificationService(); + setGlobal(NotificationService, notifications); + setGlobal(OfflineDataController, OfflineDataController()); + setGlobal(ServiceConnectionManager, FakeServiceConnectionManager()); + controller = _TestScreenController(); + }); + + test('loads offline data when data is valid and non-empty', () async { + offlineDataController + ..startShowingOfflineData(offlineApp: MockConnectedApp()) + ..offlineDataJson = { + DevToolsExportKeys.activeScreenId.name: 'test_screen', + 'test_screen': {'key': 'value'}, + }; + + final result = await controller.initOfflineData('test_screen'); + expect(result, isTrue); + expect(controller.loadedData, equals({'key': 'value'})); + expect(notifications.activeMessages, isEmpty); + }); + + test('notifies when screen data is empty', () async { + offlineDataController + ..startShowingOfflineData(offlineApp: MockConnectedApp()) + ..offlineDataJson = { + DevToolsExportKeys.activeScreenId.name: 'test_screen', + 'test_screen': <String, Object?>{}, + }; + + final result = await controller.initOfflineData('test_screen'); + expect(result, isFalse); + expect(controller.loadedData, isNull); + expect(notifications.activeMessages.length, equals(1)); + expect( + notifications.activeMessages.first.text, + equals( + 'The imported file does not contain any data for screen \'test_screen\'.', + ), + ); + }); + + test('notifies when shouldLoad returns false', () async { + offlineDataController + ..startShowingOfflineData(offlineApp: MockConnectedApp()) + ..offlineDataJson = { + DevToolsExportKeys.activeScreenId.name: 'test_screen', + 'test_screen': {'key': 'value'}, + }; + + final result = await controller.initOfflineData( + 'test_screen', + shouldLoad: false, + ); + expect(result, isFalse); + expect(controller.loadedData, isNull); + expect(notifications.activeMessages.length, equals(1)); + expect( + notifications.activeMessages.first.text, + equals( + 'The imported file does not contain any data for screen \'test_screen\'.', + ), + ); + }); + + test('notifies when creating/loading data throws an error', () async { + offlineDataController + ..startShowingOfflineData(offlineApp: MockConnectedApp()) + ..offlineDataJson = { + DevToolsExportKeys.activeScreenId.name: 'test_screen', + 'test_screen': {'throw': true}, + }; + + final result = await controller.initOfflineData('test_screen'); + expect(result, isFalse); + expect(controller.loadedData, isNull); + expect(notifications.activeMessages.length, equals(1)); + expect( + notifications.activeMessages.first.text, + contains('Failed to load offline data for screen \'test_screen\':'), + ); + }); + + test( + 'resets loadingOfflineData and notifies when loadData throws', + () async { + offlineDataController + ..startShowingOfflineData(offlineApp: MockConnectedApp()) + ..offlineDataJson = { + DevToolsExportKeys.activeScreenId.name: 'test_screen', + 'test_screen': {'key': 'value'}, + }; + + expect(controller.loadingOfflineData.value, isFalse); + final result = await controller.initOfflineData( + 'test_screen', + throwOnLoad: true, + ); + expect(result, isFalse); + expect(controller.loadingOfflineData.value, isFalse); + expect(notifications.activeMessages.length, equals(1)); + expect( + notifications.activeMessages.first.text, + contains('Failed to load offline data for screen \'test_screen\':'), + ); + }, + ); + + test( + 'returns false without notifying if screen is not in offlineDataJson', + () async { + offlineDataController + ..startShowingOfflineData(offlineApp: MockConnectedApp()) + ..offlineDataJson = { + DevToolsExportKeys.activeScreenId.name: 'other_screen', + 'other_screen': {'key': 'value'}, + }; + + final result = await controller.initOfflineData('test_screen'); + expect(result, isFalse); + expect(controller.loadedData, isNull); + expect(notifications.activeMessages, isEmpty); + }, + ); + }); +}