From 6961b202c343e12893e6c664ef70336b7c3845c3 Mon Sep 17 00:00:00 2001 From: Anna Gringauze Date: Thu, 9 Nov 2023 08:16:04 -0800 Subject: [PATCH] Fix a crash during execution context detection (#2286) * Make dwds tolerant to failure of detecting dart execution context id * Update changelog * Addressed CR comments * Cleanup * Cleanup --- dwds/CHANGELOG.md | 1 + .../lib/src/connections/debug_connection.dart | 2 +- dwds/lib/src/debugging/execution_context.dart | 40 ++- dwds/lib/src/debugging/remote_debugger.dart | 2 +- dwds/lib/src/debugging/webkit_debugger.dart | 2 +- dwds/lib/src/dwds_vm_client.dart | 15 +- dwds/lib/src/loaders/legacy.dart | 1 + dwds/lib/src/servers/extension_debugger.dart | 2 +- dwds/test/execution_context_test.dart | 298 ++++++++++++++++++ dwds/test/fixtures/fakes.dart | 2 +- 10 files changed, 345 insertions(+), 20 deletions(-) create mode 100644 dwds/test/execution_context_test.dart diff --git a/dwds/CHANGELOG.md b/dwds/CHANGELOG.md index 14676ba5a..77f341200 100644 --- a/dwds/CHANGELOG.md +++ b/dwds/CHANGELOG.md @@ -1,6 +1,7 @@ ## 23.0.0-wip - Restructure `LoadStrategy` to provide build settings. - [#2270](https://github.com/dart-lang/webdev/pull/2270) - Add `FrontendServerLegacyStrategyProvider` and update bootstrap generation logic for `LegacyStrategy` - [#2285](https://github.com/dart-lang/webdev/pull/2285) +- Tolerate failures to detect a dart execution context. - [#2286](https://github.com/dart-lang/webdev/pull/2286) ## 22.1.0 - Update `package:vm_service` constraint to `^13.0.0`. - [#2265](https://github.com/dart-lang/webdev/pull/2265) diff --git a/dwds/lib/src/connections/debug_connection.dart b/dwds/lib/src/connections/debug_connection.dart index 97de4c448..8ecaf9985 100644 --- a/dwds/lib/src/connections/debug_connection.dart +++ b/dwds/lib/src/connections/debug_connection.dart @@ -37,7 +37,7 @@ class DebugConnection { VmService get vmService => _appDebugServices.dwdsVmClient.client; Future close() => _closed ??= () async { - _appDebugServices.chromeProxyService.remoteDebugger.close(); + await _appDebugServices.chromeProxyService.remoteDebugger.close(); await _appDebugServices.close(); _onDoneCompleter.complete(); }(); diff --git a/dwds/lib/src/debugging/execution_context.dart b/dwds/lib/src/debugging/execution_context.dart index a65e582a6..978bd845e 100644 --- a/dwds/lib/src/debugging/execution_context.dart +++ b/dwds/lib/src/debugging/execution_context.dart @@ -9,8 +9,9 @@ import 'package:dwds/src/debugging/remote_debugger.dart'; import 'package:logging/logging.dart'; abstract class ExecutionContext { - /// Returns the context ID that contains the running Dart application. - Future get id; + /// Returns the context ID that contains the running Dart application, + /// if available. + Future get id; } /// The execution context in which to do remote evaluations. @@ -23,12 +24,12 @@ class RemoteDebuggerExecutionContext extends ExecutionContext { /// Context can be null if an error has occurred and we cannot detect /// and parse the context ID. late StreamQueue _contexts; + final _contextController = StreamController(); + final _seenContexts = []; int? _id; - @override - Future get id async { - if (_id != null) return _id!; + Future _lookUpId() async { _logger.fine('Looking for Dart execution context...'); const timeoutInMs = 100; while (await _contexts.hasNext.timeout( @@ -41,6 +42,7 @@ class RemoteDebuggerExecutionContext extends ExecutionContext { }, )) { final context = await _contexts.next; + _seenContexts.add(context); _logger.fine('Checking context id: $context'); try { final result = await _remoteDebugger.sendCommand( @@ -51,24 +53,34 @@ class RemoteDebuggerExecutionContext extends ExecutionContext { }, ); if (result.result?['result']?['value'] != null) { - _logger.fine('Found valid execution context: $context'); - _id = context; - break; + _logger.fine('Found dart execution context: $context'); + return context; } } catch (_) { // Errors may be thrown if we attempt to evaluate in a stale a context. // Ignore and continue. - _logger.fine('Invalid execution context: $context'); + _logger.fine('Stale execution context: $context'); + _seenContexts.remove(context); } } + return null; + } + + @override + Future get id async { + if (_id != null) return _id; + + _id = await _lookUpId(); if (_id == null) { - throw StateError('No context with the running Dart application.'); + // Add seen contexts back to the queue in case the injected + // client is still loading, so the next call to `id` succeeds. + _seenContexts.forEach(_contextController.add); + _seenContexts.clear(); } - return _id!; + return _id; } RemoteDebuggerExecutionContext(this._id, this._remoteDebugger) { - final contextController = StreamController(); _remoteDebugger .eventStream('Runtime.executionContextsCleared', (e) => e) .listen((_) => _id = null); @@ -86,8 +98,8 @@ class RemoteDebuggerExecutionContext extends ExecutionContext { } return parsedId; }).listen((e) { - if (e != null) contextController.add(e); + if (e != null) _contextController.add(e); }); - _contexts = StreamQueue(contextController.stream); + _contexts = StreamQueue(_contextController.stream); } } diff --git a/dwds/lib/src/debugging/remote_debugger.dart b/dwds/lib/src/debugging/remote_debugger.dart index e181eb4fb..48657ce81 100644 --- a/dwds/lib/src/debugging/remote_debugger.dart +++ b/dwds/lib/src/debugging/remote_debugger.dart @@ -72,5 +72,5 @@ abstract class RemoteDebugger { Stream eventStream(String method, WipEventTransformer transformer); - void close(); + Future close(); } diff --git a/dwds/lib/src/debugging/webkit_debugger.dart b/dwds/lib/src/debugging/webkit_debugger.dart index 1804abadb..23bbd0c8f 100644 --- a/dwds/lib/src/debugging/webkit_debugger.dart +++ b/dwds/lib/src/debugging/webkit_debugger.dart @@ -32,7 +32,7 @@ class WebkitDebugger implements RemoteDebugger { _wipDebugger.sendCommand(command, params: params); @override - void close() => _closed ??= _wipDebugger.connection.close(); + Future close() => _closed ??= _wipDebugger.connection.close(); @override Future disable() => _wipDebugger.disable(); diff --git a/dwds/lib/src/dwds_vm_client.dart b/dwds/lib/src/dwds_vm_client.dart index c1ebeab60..145b7354a 100644 --- a/dwds/lib/src/dwds_vm_client.dart +++ b/dwds/lib/src/dwds_vm_client.dart @@ -213,6 +213,19 @@ void _recordDwdsStats(DwdsStats dwdsStats, String screen) { } } +Future tryGetContextId( + ChromeProxyService chromeProxyService, { + int retries = 3, +}) async { + const waitInMs = 50; + for (var retry = 0; retry < retries; retry++) { + final tryId = await chromeProxyService.executionContext.id; + if (tryId != null) return tryId; + await Future.delayed(const Duration(milliseconds: waitInMs)); + } + throw StateError('No context with the running Dart application.'); +} + Future> _hotRestart( ChromeProxyService chromeProxyService, VmService client, @@ -223,7 +236,7 @@ Future> _hotRestart( await _disableBreakpointsAndResume(client, chromeProxyService); try { _logger.info('Attempting to get execution context ID.'); - await chromeProxyService.executionContext.id; + await tryGetContextId(chromeProxyService); _logger.info('Got execution context ID.'); } on StateError catch (e) { // We couldn't find the execution context. `hotRestart` may have been diff --git a/dwds/lib/src/loaders/legacy.dart b/dwds/lib/src/loaders/legacy.dart index 4e17bcf3d..bfaa50315 100644 --- a/dwds/lib/src/loaders/legacy.dart +++ b/dwds/lib/src/loaders/legacy.dart @@ -66,6 +66,7 @@ class LegacyStrategy extends LoadStrategy { /// packages/path/path -> d348c2a4647e998011fe305f74f22961 /// final Future> Function(MetadataProvider metadataProvider) + // ignore: unused_field _digestsProvider; /// Returns the module for the corresponding server path. diff --git a/dwds/lib/src/servers/extension_debugger.dart b/dwds/lib/src/servers/extension_debugger.dart index 8dda548b8..6c0736198 100644 --- a/dwds/lib/src/servers/extension_debugger.dart +++ b/dwds/lib/src/servers/extension_debugger.dart @@ -192,7 +192,7 @@ class ExtensionDebugger implements RemoteDebugger { int newId() => _completerId++; @override - void close() => _closed ??= () { + Future close() => _closed ??= () { _closeController.add({}); return Future.wait([ sseConnection.sink.close(), diff --git a/dwds/test/execution_context_test.dart b/dwds/test/execution_context_test.dart new file mode 100644 index 000000000..90d961d1b --- /dev/null +++ b/dwds/test/execution_context_test.dart @@ -0,0 +1,298 @@ +// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +@Timeout(Duration(minutes: 2)) +import 'dart:async'; +import 'dart:convert'; + +import 'package:dwds/data/devtools_request.dart'; +import 'package:dwds/data/extension_request.dart'; +import 'package:dwds/data/serializers.dart'; +import 'package:dwds/src/debugging/execution_context.dart'; +import 'package:dwds/src/servers/extension_debugger.dart'; +import 'package:test/test.dart'; +import 'package:test_common/logging.dart'; +import 'package:webkit_inspection_protocol/webkit_inspection_protocol.dart'; + +import 'fixtures/fakes.dart'; + +void main() async { + final bool debug = false; + + group('ExecutionContext', () { + setUpAll(() { + setCurrentLogWriter(debug: debug); + }); + + TestDebuggerConnection? debugger; + TestDebuggerConnection getDebugger() => debugger!; + + setUp(() async { + setCurrentLogWriter(debug: debug); + debugger = TestDebuggerConnection(); + }); + + tearDown(() async { + await debugger?.close(); + }); + + test('is created on devtools request', () async { + final debugger = getDebugger(); + await debugger.createDebuggerExecutionContext(TestContextId.dartDefault); + + // Expect the context ID to be set. + expect(await debugger.defaultContextId(), TestContextId.dartDefault); + }); + + test('clears context ID', () async { + final debugger = getDebugger(); + await debugger.createDebuggerExecutionContext(TestContextId.dartDefault); + + debugger.sendContextsClearedEvent(); + + // Expect non-dart context. + expect(await debugger.defaultContextId(), TestContextId.none); + }); + + test('finds dart context ID', () async { + final debugger = getDebugger(); + await debugger.createDebuggerExecutionContext(TestContextId.none); + + debugger.sendContextCreatedEvent(TestContextId.dartNormal); + + // Expect dart context. + expect(await debugger.dartContextId(), TestContextId.dartNormal); + }); + + test('does not find dart context ID if not available', () async { + final debugger = getDebugger(); + await debugger.createDebuggerExecutionContext(TestContextId.none); + + // No context IDs received yet. + expect(await debugger.defaultContextId(), TestContextId.none); + + debugger.sendContextCreatedEvent(TestContextId.dartLate); + + // Expect no dart context. + // This mocks injected client still loading. + expect(await debugger.noContextId(), TestContextId.none); + + // Expect dart context. + // This mocks injected client loading later for previously + // received context ID. + expect(await debugger.dartContextId(), TestContextId.dartLate); + }); + + test('works with stale contexts', () async { + final debugger = getDebugger(); + await debugger.createDebuggerExecutionContext(TestContextId.none); + + debugger.sendContextCreatedEvent(TestContextId.stale); + + // Expect no dart context. + expect(await debugger.noContextId(), TestContextId.none); + + debugger.sendContextsClearedEvent(); + debugger.sendContextCreatedEvent(TestContextId.dartNormal); + + // Expect dart context. + expect(await debugger.dartContextId(), TestContextId.dartNormal); + }); + }); +} + +enum TestContextId { + none, + dartDefault, + dartNormal, + dartLate, + nonDart, + stale; + + factory TestContextId.from(int? value) { + return switch (value) { + null => none, + 0 => dartDefault, + 1 => dartNormal, + 2 => dartLate, + 3 => nonDart, + 4 => stale, + _ => throw StateError('$value is not a TestContextId'), + }; + } + + int? get id { + return switch (this) { + none => null, + dartDefault => 0, + dartNormal => 1, + dartLate => 2, + nonDart => 3, + stale => 4, + }; + } +} + +class TestExtensionDebugger extends ExtensionDebugger { + TestExtensionDebugger(FakeSseConnection super.sseConnection); + + @override + Future sendCommand( + String command, { + Map? params, + }) { + final id = params?['contextId']; + final response = super.sendCommand(command, params: params); + + /// Mock stale contexts that cause the evaluation to throw. + if (command == 'Runtime.evaluate' && + TestContextId.from(id) == TestContextId.stale) { + throw Exception('Stale execution context'); + } + return response; + } +} + +class TestDebuggerConnection { + late final TestExtensionDebugger extensionDebugger; + late final FakeSseConnection connection; + + int _evaluateRequestId = 0; + + TestDebuggerConnection() { + connection = FakeSseConnection(); + extensionDebugger = TestExtensionDebugger(connection); + } + + /// Create a new execution context in the debugger. + Future createDebuggerExecutionContext(TestContextId contextId) { + _sendDevToolsRequest(contextId: contextId.id); + return _executionContext(); + } + + /// Flush the streams and close debugger connection. + Future close() async { + unawaited(connection.controllerOutgoing.stream.any((e) => false)); + unawaited(extensionDebugger.devToolsRequestStream.any((e) => false)); + + await connection.controllerIncoming.sink.close(); + await connection.controllerOutgoing.sink.close(); + + await extensionDebugger.close(); + } + + /// Return the initial context ID from the DevToolsRequest. + Future defaultContextId() async { + // Give the previous events time to propagate. + await Future.delayed(Duration(milliseconds: 100)); + return TestContextId.from(await extensionDebugger.executionContext!.id); + } + + /// Mock receiving dart context ID in the execution context. + /// + /// Note: dart context is detected by evaluation of + /// `window.$dartAppInstanceId` in that context returning + /// a non-null value. + Future dartContextId() async { + // Try getting execution id. + final executionContextId = extensionDebugger.executionContext!.id; + + // Give it time to send the evaluate request. + await Future.delayed(Duration(milliseconds: 100)); + + // Respond to the evaluate request. + _sendEvaluationResponse({ + 'result': {'value': 'dart'}, + }); + + return TestContextId.from(await executionContextId); + } + + /// Mock receiving non-dart context ID in the execution context. + /// + /// Note: dart context is detected by evaluation of + /// `window.$dartAppInstanceId` in that context returning + /// a null value. + Future noContextId() async { + // Try getting execution id. + final executionContextId = extensionDebugger.executionContext!.id; + + // Give it time to send the evaluate request. + await Future.delayed(Duration(milliseconds: 100)); + + // Respond to the evaluate request. + _sendEvaluationResponse({ + 'result': {'value': null}, + }); + + return TestContextId.from(await executionContextId); + } + + /// Send `Runtime.executionContextsCleared` event to the execution + /// context in the extension debugger. + void sendContextsClearedEvent() { + final extensionEvent = ExtensionEvent( + (b) => b + ..method = jsonEncode('Runtime.executionContextsCleared') + ..params = jsonEncode({}), + ); + connection.controllerIncoming.sink + .add(jsonEncode(serializers.serialize(extensionEvent))); + } + + /// Send `Runtime.executionContextCreated` event to the execution + /// context in the extension debugger. + void sendContextCreatedEvent(TestContextId contextId) { + final extensionEvent = ExtensionEvent( + (b) => b + ..method = jsonEncode('Runtime.executionContextCreated') + ..params = jsonEncode({ + 'context': {'id': '${contextId.id}'}, + }), + ); + connection.controllerIncoming.sink + .add(jsonEncode(serializers.serialize(extensionEvent))); + } + + void _sendEvaluationResponse(Map response) { + // Respond to the evaluate request. + final extensionResponse = ExtensionResponse( + (b) => b + ..result = jsonEncode(response) + ..id = _evaluateRequestId++ + ..success = true, + ); + connection.controllerIncoming.sink + .add(jsonEncode(serializers.serialize(extensionResponse))); + } + + void _sendDevToolsRequest({int? contextId}) { + final devToolsRequest = DevToolsRequest( + (b) => b + ..contextId = contextId + ..appId = 'app' + ..instanceId = '0', + ); + connection.controllerIncoming.sink + .add(jsonEncode(serializers.serialize(devToolsRequest))); + } + + Future _executionContext() async { + final executionContext = await _waitForExecutionContext().timeout( + const Duration(milliseconds: 100), + onTimeout: () { + expect(fail, 'Timeout getting execution context'); + return null; + }, + ); + expect(executionContext, isNotNull); + } + + Future _waitForExecutionContext() async { + while (extensionDebugger.executionContext == null) { + await Future.delayed(Duration(milliseconds: 20)); + } + return extensionDebugger.executionContext; + } +} diff --git a/dwds/test/fixtures/fakes.dart b/dwds/test/fixtures/fakes.dart index 5470108dc..a24381d7a 100644 --- a/dwds/test/fixtures/fakes.dart +++ b/dwds/test/fixtures/fakes.dart @@ -275,7 +275,7 @@ class FakeWebkitDebugger implements WebkitDebugger { Stream get onExceptionThrown => Stream.empty(); @override - void close() {} + Future close() async {} @override Stream get onClose => Stream.empty();