diff --git a/packages/devtools_app/test/shared/eval_integration_test.dart b/packages/devtools_app/test/shared/eval_integration_test.dart index 3dea8867f24..ccc6c7caa79 100644 --- a/packages/devtools_app/test/shared/eval_integration_test.dart +++ b/packages/devtools_app/test/shared/eval_integration_test.dart @@ -5,7 +5,6 @@ import 'package:devtools_app/src/shared/globals.dart'; import 'package:devtools_app_shared/service.dart'; import 'package:devtools_app_shared/utils.dart'; -import 'package:devtools_test/helpers.dart'; import 'package:flutter_test/flutter_test.dart'; import '../test_infra/flutter_test_driver.dart'; @@ -71,39 +70,58 @@ void main() { expect(instance2.classRef!.name, '_Future'); }, timeout: const Timeout.factor(2), - // TODO(https://github.com/flutter/devtools/issues/9484): if this flake - // is addressed, we can unskip this for the Flutter customer tests. - tags: skipForCustomerTestsTag, ); - test( - 'returns the result of the future completion', - () async { - await env.setupEnvironment(); - final mainIsolate = - serviceConnection.serviceManager.isolateManager.mainIsolate; - expect(mainIsolate, isNotNull); - - final eval = EvalOnDartLibrary( - 'dart:core', - serviceConnection.serviceManager.service!, - serviceManager: serviceConnection.serviceManager, - isolate: mainIsolate, + test('returns the result of the future completion', () async { + await env.setupEnvironment(); + final mainIsolate = + serviceConnection.serviceManager.isolateManager.mainIsolate; + + final eval = EvalOnDartLibrary( + 'dart:core', + serviceConnection.serviceManager.service!, + serviceManager: serviceConnection.serviceManager, + isolate: mainIsolate, + ); + + final instance = (await eval.asyncEval( + // The delay asserts that there is no issue with garbage collection + 'await Future.delayed(const Duration(milliseconds: 500), () => 42)', + isAlive: isAlive, + ))!; + + expect(instance.valueAsString, '42'); + }, timeout: const Timeout.factor(2)); + + test('survives garbage collection while the future is pending', () async { + await env.setupEnvironment(); + final mainIsolate = + serviceConnection.serviceManager.isolateManager.mainIsolate; + + final eval = EvalOnDartLibrary( + 'dart:core', + serviceConnection.serviceManager.service!, + serviceManager: serviceConnection.serviceManager, + isolate: mainIsolate, + ); + + final evalFuture = eval.asyncEval( + 'await Future.delayed(const Duration(milliseconds: 500), () => 42)', + isAlive: isAlive, + ); + + // Force garbage collection in the target isolate while the future is pending. + for (var i = 0; i < 3; i++) { + await Future.delayed(const Duration(milliseconds: 100)); + await serviceConnection.serviceManager.service!.getAllocationProfile( + mainIsolate.value!.id!, + gc: true, ); + } - final instance = (await eval.asyncEval( - // The delay asserts that there is no issue with garbage collection - 'await Future.delayed(const Duration(milliseconds: 500), () => 42)', - isAlive: isAlive, - ))!; - - expect(instance.valueAsString, '42'); - }, - timeout: const Timeout.factor(2), - // TODO(https://github.com/flutter/devtools/issues/9484): if this flake - // is addressed, we can unskip this for the Flutter customer tests. - tags: skipForCustomerTestsTag, - ); + final instance = (await evalFuture)!; + expect(instance.valueAsString, '42'); + }, timeout: const Timeout.factor(2)); test( 'throws FutureFailedException when the future is rejected', @@ -152,12 +170,7 @@ void main() { expect(error.valueAsString, 'foo'); }, timeout: const Timeout.factor(2), - // TODO(https://github.com/flutter/devtools/issues/9484): if this flake - // is addressed, we can unskip this for the Flutter customer tests. - tags: skipForCustomerTestsTag, ); - // TODO(https://github.com/flutter/devtools/issues/9484): if this flake - // is addressed, we can remove the retry. - }, retry: 3); + }); }); } diff --git a/packages/devtools_app/test/test_infra/flutter_test_environment.dart b/packages/devtools_app/test/test_infra/flutter_test_environment.dart index d85b3281dd2..71906b77fb1 100644 --- a/packages/devtools_app/test/test_infra/flutter_test_environment.dart +++ b/packages/devtools_app/test/test_infra/flutter_test_environment.dart @@ -107,7 +107,8 @@ class FlutterTestEnvironment { if (force || _needsSetup || !reuseTestEnvironment || - _isNewRunConfig(config)) { + _isNewRunConfig(config) || + !serviceConnection.serviceManager.connectedState.value.connected) { _setupInProgress = Completer(); try { // If we already have a running test device, stop it before setting up a diff --git a/packages/devtools_app_shared/CHANGELOG.md b/packages/devtools_app_shared/CHANGELOG.md index 83756dfe4eb..1547923e98c 100644 --- a/packages/devtools_app_shared/CHANGELOG.md +++ b/packages/devtools_app_shared/CHANGELOG.md @@ -6,7 +6,7 @@ found in the LICENSE file or at https://developers.google.com/open-source/licens ## 0.5.2-wip * Fix a `RangeError` thrown by `SplitPane` when the number of children changes between rebuilds. -* Fix garbage collection issues with the result list in `asyncEval` on both native VM and web. +* Fix garbage collection and timeout issues with the result list in `asyncEval` on both native VM and web. * Safely handle RPC errors and unexpected exceptions when calling service extensions in `ServiceExtensionManager`. * The minimum Dart SDK version is bumped to 3.11.0. * The minimum Flutter SDK version is bumped to 3.41.0. diff --git a/packages/devtools_app_shared/lib/src/service/eval_on_dart_library.dart b/packages/devtools_app_shared/lib/src/service/eval_on_dart_library.dart index 8df2f921353..e831211384c 100644 --- a/packages/devtools_app_shared/lib/src/service/eval_on_dart_library.dart +++ b/packages/devtools_app_shared/lib/src/service/eval_on_dart_library.dart @@ -418,43 +418,50 @@ class EvalOnDartLibrary extends DisposableController isAlive: isAlive, ); - final readerId = await safeEval( - // since we are awaiting the Future, we need to make sure that during the awaiting, - // the "reader" is not GCed - 'widgetInspectorService.toId([], "$readerGroup")', - isAlive: isAlive, - scope: {'widgetInspectorService': widgetInspectorServiceRef.id!}, - ).then((ref) => ref.valueAsString!); - await safeEval( '() async {' - ' final reader = widgetInspectorService.toObject("$readerId", "$readerGroup") as List;' - ' /* Keep a strong reference to `reader` in the target app to prevent it' - ' from being garbage collected by Chrome/VM before the future resolves.' - ' Without this, the reader is only weakly referenced by the inspector' - ' service and is aggressively GCed, causing a TypeError/TimeoutException' - ' or failing the assertion that the retrieved result length is 1 or 2.' - ' We use Future.delayed in a loop instead of Timer because Future is in' - ' dart:core and guaranteed to be resolved without requiring dart:async. */' - ' bool isDone = false;' - ' () async {' - ' int bufferTicks = 0;' - ' /* Stop pinning after a 1-second buffer when the future has completed. */' - ' while (!isDone && ++bufferTicks <=20) {' - ' final _ = reader;' - ' await Future.delayed(const Duration(milliseconds: 50));' - ' }' - ' }();' + ' String? readerId;' ' try {' + ' final reader = [];' + ' readerId = widgetInspectorService.toId(reader, "$readerGroup") as String;' + ' try {' // Cast as dynamic so that it is possible to await Future - ' dynamic result = ($expression) as dynamic;' - ' reader.add(result);' - ' } catch (err, stack) {' - ' reader.add(err);' - ' reader.add(stack);' - ' } finally {' - ' isDone = true;' - ' postEvent("future_completed", {"future_id": $futureId, "client_id": $_clientId});' + ' dynamic result = ($expression) as dynamic;' + ' reader.add(result);' + ' } catch (err, stack) {' + ' reader.add(err);' + ' reader.add(stack);' + ' } finally {' + ' postEvent("future_completed", {' + ' "future_id": $futureId,' + ' "client_id": $_clientId,' + ' "reader_id": readerId,' + ' });' + ' /* Keep a strong reference to `reader` in the target app to prevent it' + ' from being garbage collected by Chrome/VM before DevTools retrieves it.' + ' Without this, the reader is only weakly referenced by the inspector' + ' service and is aggressively GCed, causing a TypeError/TimeoutException' + ' or failing the assertion that the retrieved result length is 1 or 2.' + ' We use Future.delayed in a loop instead of Timer because Future is in' + ' dart:core and guaranteed to be resolved without requiring dart:async. */' + ' for (int i = 0; i < 200; i++) {' + ' await Future.delayed(const Duration(milliseconds: 50));' + ' try {' + ' if (widgetInspectorService.toObject(readerId!, "$readerGroup") == null) {' + ' break;' + ' }' + ' } catch (_) {' + ' break;' + ' }' + ' final _ = reader.length;' + ' }' + ' }' + ' } catch (err) {' + ' postEvent("future_completed", {' + ' "future_id": $futureId,' + ' "client_id": $_clientId,' + ' "error": err.toString(),' + ' });' ' }' '}()', isAlive: isAlive, @@ -465,7 +472,15 @@ class EvalOnDartLibrary extends DisposableController }, ); - await future; + final event = await future; + if (event.extensionData?.data['error'] != null) { + throw UnknownEvalException( + expression: expression, + scope: scope, + exception: event.extensionData!.data['error'], + ); + } + final readerId = event.extensionData!.data['reader_id'] as String; final resultRef = await evalInstance( '() {'