From f5587a4fa8fe8ec683d8d9a69c459a2339e1f041 Mon Sep 17 00:00:00 2001 From: Srujan Gaddam Date: Thu, 30 Jan 2025 18:07:19 -0800 Subject: [PATCH 1/3] [dwds] callServiceExtension should check all extensions and return kMethodNotFound when extension not found and remove reassemble from reload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://github.com/dart-lang/webdev/issues/2584 Currently, callServiceExtension checks the extensionRpcs field in the VM service. This list is populated when DWDS starts and checks the extensions currently registered in the runtime. This list is prone to becoming stale, however, as services may be registered or removed through the runtime (via dart:developer) later. This is true for ext.flutter.reassemble, where DWDS can not find the service because we computed the list too early. Instead, we should seek to always check the runtime for the current registered extensions and then update this list whenever we do. It’s still prone to becoming stale, but it is less stale. Ideally, we would override extensionRpcs and fetch the current registered extensions, but that isn’t possible since it requires awaiting a future. We also can’t ignore this field altogether because it looks like clients, like Dart DevTools, uses this field. callServiceExtension should also return kMethodNotFound instead of its current JS evaluation error when an extension is not present. Lastly, the reassemble call is removed from reload as it can now be safely called in Flutter tools instead. --- dwds/CHANGELOG.md | 7 ++++ dwds/lib/src/debugging/inspector.dart | 12 ++++--- dwds/lib/src/injected/client.js | 32 ------------------- .../src/services/chrome_proxy_service.dart | 9 +++++- .../ddc_library_bundle_restarter.dart | 11 ------- 5 files changed, 23 insertions(+), 48 deletions(-) diff --git a/dwds/CHANGELOG.md b/dwds/CHANGELOG.md index 0b4c08223..6c73b15e5 100644 --- a/dwds/CHANGELOG.md +++ b/dwds/CHANGELOG.md @@ -1,5 +1,12 @@ ## 24.3.7-wip +- `callServiceExtension` now checks the runtime for the list of registered + service extensions. It also now throws a `RPCError` with + `RPCErrorKind.kMethodNotFound` when a service extension is not found instead + of throwing a JS evaluation error. +- The registered extension `reassemble` is now no longer called when calling + `reloadSources`. Users should call `reassemble` using `callServiceExtension`. + ## 24.3.6 - Bump minimum sdk version to 3.7.0 diff --git a/dwds/lib/src/debugging/inspector.dart b/dwds/lib/src/debugging/inspector.dart index b2a8fcdd1..ee8cab6ef 100644 --- a/dwds/lib/src/debugging/inspector.dart +++ b/dwds/lib/src/debugging/inspector.dart @@ -115,8 +115,6 @@ class AppInspector implements AppInspectorInterface { await DartUri.initialize(); DartUri.recordAbsoluteUris(libraries.map((lib) => lib.uri).nonNulls); DartUri.recordAbsoluteUris(scripts.map((script) => script.uri).nonNulls); - - isolate.extensionRPCs?.addAll(await _getExtensionRpcs()); } static IsolateRef _toIsolateRef(Isolate isolate) => IsolateRef( @@ -760,11 +758,16 @@ class AppInspector implements AppInspectorInterface { scriptId == null ? null : _scriptRefsById[scriptId]; /// Runs an eval on the page to compute all existing registered extensions. - Future> _getExtensionRpcs() async { + /// + /// Combines this with the RPCs registered in the [isolate]. Use this over + /// [Isolate.extensionRPCs] as this computes a live set. + /// + /// Updates [Isolate.extensionRPCs] to this set. + Future> getExtensionRpcs() async { final expression = globalToolConfiguration.loadStrategy.dartRuntimeDebugger .getDartDeveloperExtensionNamesJsExpression(); - final extensionRpcs = []; + final extensionRpcs = {}; final params = { 'expression': expression, 'returnByValue': true, @@ -784,6 +787,7 @@ class AppInspector implements AppInspectorInterface { s, ); } + isolate.extensionRPCs = List.from(extensionRpcs); return extensionRpcs; } diff --git a/dwds/lib/src/injected/client.js b/dwds/lib/src/injected/client.js index ec9487fb4..d37211081 100644 --- a/dwds/lib/src/injected/client.js +++ b/dwds/lib/src/injected/client.js @@ -9809,34 +9809,6 @@ }); return A._asyncStartSync($async$_Debugger_maybeInvokeFlutterDisassemble, $async$completer); }, - _Debugger_maybeInvokeFlutterReassemble(_this) { - var $async$goto = 0, - $async$completer = A._makeAsyncAwaitCompleter(type$.void), - t1; - var $async$_Debugger_maybeInvokeFlutterReassemble = A._wrapJsFunctionForAsync(function($async$errorCode, $async$result) { - if ($async$errorCode === 1) - return A._asyncRethrow($async$result, $async$completer); - while (true) - switch ($async$goto) { - case 0: - // Function start - t1 = type$.JSArray_nullable_Object._as(_this.extensionNames); - $async$goto = J.contains$1$asx(type$.List_String._is(t1) ? t1 : new A.CastList(t1, A._arrayInstanceType(t1)._eval$1("CastList<1,String>")), "ext.flutter.reassemble") ? 2 : 3; - break; - case 2: - // then - $async$goto = 4; - return A._asyncAwait(A.promiseToFuture(type$.JSObject._as(_this.invokeExtension("ext.flutter.reassemble", "{}")), type$.String), $async$_Debugger_maybeInvokeFlutterReassemble); - case 4: - // returning from await. - case 3: - // join - // implicit return - return A._asyncReturn(null, $async$completer); - } - }); - return A._asyncStartSync($async$_Debugger_maybeInvokeFlutterReassemble, $async$completer); - }, DdcLibraryBundleRestarter: function DdcLibraryBundleRestarter() { }, DdcLibraryBundleRestarter_reload_closure: function DdcLibraryBundleRestarter_reload_closure(t0, t1) { @@ -26742,10 +26714,6 @@ $async$goto = 3; return A._asyncAwait(A.promiseToFuture(t3._as(t3._as(t2.dartDevEmbedder).hotReload(filesToLoad, librariesToReload)), type$.nullable_Object), $async$reload$1); case 3: - // returning from await. - $async$goto = 4; - return A._asyncAwait(A._Debugger_maybeInvokeFlutterReassemble(t3._as(t3._as(t2.dartDevEmbedder).debugger)), $async$reload$1); - case 4: // returning from await. // implicit return return A._asyncReturn(null, $async$completer); diff --git a/dwds/lib/src/services/chrome_proxy_service.dart b/dwds/lib/src/services/chrome_proxy_service.dart index 93b9f6751..e0c3a4139 100644 --- a/dwds/lib/src/services/chrome_proxy_service.dart +++ b/dwds/lib/src/services/chrome_proxy_service.dart @@ -343,7 +343,7 @@ class ChromeProxyService implements VmServiceInterface { // TODO: We shouldn't need to fire these events since they exist on the // isolate, but devtools doesn't recognize extensions after a page refresh // otherwise. - for (final extensionRpc in inspector.isolate.extensionRPCs ?? []) { + for (final extensionRpc in await inspector.getExtensionRpcs()) { _streamNotify( 'Isolate', Event( @@ -509,6 +509,13 @@ class ChromeProxyService implements VmServiceInterface { v is String ? v : jsonEncode(v), ), ); + if ((await inspector.getExtensionRpcs()).contains(method) != true) { + throw RPCError( + method, + RPCErrorKind.kMethodNotFound.code, + 'Unknown service method: $method', + ); + } final expression = globalToolConfiguration.loadStrategy.dartRuntimeDebugger .invokeExtensionJsExpression(method, jsonEncode(stringArgs)); final result = await inspector.jsEvaluate(expression, awaitPromise: true); diff --git a/dwds/web/reloader/ddc_library_bundle_restarter.dart b/dwds/web/reloader/ddc_library_bundle_restarter.dart index f91f74aaf..3ff79935f 100644 --- a/dwds/web/reloader/ddc_library_bundle_restarter.dart +++ b/dwds/web/reloader/ddc_library_bundle_restarter.dart @@ -30,13 +30,6 @@ extension type _Debugger._(JSObject _) implements JSObject { await invokeExtension(method, '{}').toDart; } } - - Future maybeInvokeFlutterReassemble() async { - final method = 'ext.flutter.reassemble'; - if (extensionNames.toDart.contains(method.toJS)) { - await invokeExtension(method, '{}').toDart; - } - } } @JS('XMLHttpRequest') @@ -92,9 +85,5 @@ class DdcLibraryBundleRestarter implements Restarter { } } await _dartDevEmbedder.hotReload(filesToLoad, librariesToReload).toDart; - // TODO(srujzs): Reassembling is slow. It's roughly almost the time it takes - // to recompile and do a hot reload. We should do some better profiling and - // see if we can improve this. - await _dartDevEmbedder.debugger.maybeInvokeFlutterReassemble(); } } From 1010d403c147d5ee8fd28af7b6b15f32ec88e5bb Mon Sep 17 00:00:00 2001 From: Srujan Gaddam Date: Thu, 6 Mar 2025 17:39:49 -0800 Subject: [PATCH 2/3] Fix test and code metrics --- dwds/lib/src/debugging/inspector.dart | 3 ++- dwds/lib/src/services/chrome_proxy_service.dart | 2 +- dwds/test/common/chrome_proxy_service_common.dart | 14 ++++++++++++++ 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/dwds/lib/src/debugging/inspector.dart b/dwds/lib/src/debugging/inspector.dart index ee8cab6ef..7128adde0 100644 --- a/dwds/lib/src/debugging/inspector.dart +++ b/dwds/lib/src/debugging/inspector.dart @@ -115,6 +115,7 @@ class AppInspector implements AppInspectorInterface { await DartUri.initialize(); DartUri.recordAbsoluteUris(libraries.map((lib) => lib.uri).nonNulls); DartUri.recordAbsoluteUris(scripts.map((script) => script.uri).nonNulls); + await getExtensionRpcs(); } static IsolateRef _toIsolateRef(Isolate isolate) => IsolateRef( @@ -787,7 +788,7 @@ class AppInspector implements AppInspectorInterface { s, ); } - isolate.extensionRPCs = List.from(extensionRpcs); + isolate.extensionRPCs = List.of(extensionRpcs); return extensionRpcs; } diff --git a/dwds/lib/src/services/chrome_proxy_service.dart b/dwds/lib/src/services/chrome_proxy_service.dart index e0c3a4139..71f496b22 100644 --- a/dwds/lib/src/services/chrome_proxy_service.dart +++ b/dwds/lib/src/services/chrome_proxy_service.dart @@ -509,7 +509,7 @@ class ChromeProxyService implements VmServiceInterface { v is String ? v : jsonEncode(v), ), ); - if ((await inspector.getExtensionRpcs()).contains(method) != true) { + if (!(await inspector.getExtensionRpcs()).contains(method)) { throw RPCError( method, RPCErrorKind.kMethodNotFound.code, diff --git a/dwds/test/common/chrome_proxy_service_common.dart b/dwds/test/common/chrome_proxy_service_common.dart index 1c165f620..178aed35a 100644 --- a/dwds/test/common/chrome_proxy_service_common.dart +++ b/dwds/test/common/chrome_proxy_service_common.dart @@ -298,6 +298,20 @@ void runTests({ ), }, ); + + test('not found', () async { + final serviceMethod = 'ext.test.missingServiceExtension'; + expect( + service.callServiceExtension(serviceMethod), + throwsA( + predicate( + (dynamic error) => + error is RPCError && + error.code == RPCErrorKind.kMethodNotFound.code, + ), + ), + ); + }); }); group('VMTimeline', () { From ec1ad11095ad4210fa663cd641f2d9213d388c1f Mon Sep 17 00:00:00 2001 From: Srujan Gaddam Date: Thu, 6 Mar 2025 18:14:51 -0800 Subject: [PATCH 3/3] Remove reassemble removal - that should be done in another PR --- dwds/CHANGELOG.md | 2 -- dwds/lib/src/injected/client.js | 32 +++++++++++++++++++ .../ddc_library_bundle_restarter.dart | 11 +++++++ 3 files changed, 43 insertions(+), 2 deletions(-) diff --git a/dwds/CHANGELOG.md b/dwds/CHANGELOG.md index 6c73b15e5..5b2c18105 100644 --- a/dwds/CHANGELOG.md +++ b/dwds/CHANGELOG.md @@ -4,8 +4,6 @@ service extensions. It also now throws a `RPCError` with `RPCErrorKind.kMethodNotFound` when a service extension is not found instead of throwing a JS evaluation error. -- The registered extension `reassemble` is now no longer called when calling - `reloadSources`. Users should call `reassemble` using `callServiceExtension`. ## 24.3.6 diff --git a/dwds/lib/src/injected/client.js b/dwds/lib/src/injected/client.js index d37211081..ec9487fb4 100644 --- a/dwds/lib/src/injected/client.js +++ b/dwds/lib/src/injected/client.js @@ -9809,6 +9809,34 @@ }); return A._asyncStartSync($async$_Debugger_maybeInvokeFlutterDisassemble, $async$completer); }, + _Debugger_maybeInvokeFlutterReassemble(_this) { + var $async$goto = 0, + $async$completer = A._makeAsyncAwaitCompleter(type$.void), + t1; + var $async$_Debugger_maybeInvokeFlutterReassemble = A._wrapJsFunctionForAsync(function($async$errorCode, $async$result) { + if ($async$errorCode === 1) + return A._asyncRethrow($async$result, $async$completer); + while (true) + switch ($async$goto) { + case 0: + // Function start + t1 = type$.JSArray_nullable_Object._as(_this.extensionNames); + $async$goto = J.contains$1$asx(type$.List_String._is(t1) ? t1 : new A.CastList(t1, A._arrayInstanceType(t1)._eval$1("CastList<1,String>")), "ext.flutter.reassemble") ? 2 : 3; + break; + case 2: + // then + $async$goto = 4; + return A._asyncAwait(A.promiseToFuture(type$.JSObject._as(_this.invokeExtension("ext.flutter.reassemble", "{}")), type$.String), $async$_Debugger_maybeInvokeFlutterReassemble); + case 4: + // returning from await. + case 3: + // join + // implicit return + return A._asyncReturn(null, $async$completer); + } + }); + return A._asyncStartSync($async$_Debugger_maybeInvokeFlutterReassemble, $async$completer); + }, DdcLibraryBundleRestarter: function DdcLibraryBundleRestarter() { }, DdcLibraryBundleRestarter_reload_closure: function DdcLibraryBundleRestarter_reload_closure(t0, t1) { @@ -26714,6 +26742,10 @@ $async$goto = 3; return A._asyncAwait(A.promiseToFuture(t3._as(t3._as(t2.dartDevEmbedder).hotReload(filesToLoad, librariesToReload)), type$.nullable_Object), $async$reload$1); case 3: + // returning from await. + $async$goto = 4; + return A._asyncAwait(A._Debugger_maybeInvokeFlutterReassemble(t3._as(t3._as(t2.dartDevEmbedder).debugger)), $async$reload$1); + case 4: // returning from await. // implicit return return A._asyncReturn(null, $async$completer); diff --git a/dwds/web/reloader/ddc_library_bundle_restarter.dart b/dwds/web/reloader/ddc_library_bundle_restarter.dart index 3ff79935f..f91f74aaf 100644 --- a/dwds/web/reloader/ddc_library_bundle_restarter.dart +++ b/dwds/web/reloader/ddc_library_bundle_restarter.dart @@ -30,6 +30,13 @@ extension type _Debugger._(JSObject _) implements JSObject { await invokeExtension(method, '{}').toDart; } } + + Future maybeInvokeFlutterReassemble() async { + final method = 'ext.flutter.reassemble'; + if (extensionNames.toDart.contains(method.toJS)) { + await invokeExtension(method, '{}').toDart; + } + } } @JS('XMLHttpRequest') @@ -85,5 +92,9 @@ class DdcLibraryBundleRestarter implements Restarter { } } await _dartDevEmbedder.hotReload(filesToLoad, librariesToReload).toDart; + // TODO(srujzs): Reassembling is slow. It's roughly almost the time it takes + // to recompile and do a hot reload. We should do some better profiling and + // see if we can improve this. + await _dartDevEmbedder.debugger.maybeInvokeFlutterReassemble(); } }