devtools icon indicating copy to clipboard operation
devtools copied to clipboard

devtools_extensions: asyncEval fails on web targets

Open adsonpleal opened this issue 1 month ago • 17 comments

This issue is for devtools_extensions.

asyncEval is not working for debug services running for web targets. Other targets (tested on MacOS) work fine.

Whenever I call asyncEval on a web target I get the following error:

ChromeProxyService: Failed to evaluate expression 'postEvent': InternalError: frontend server failed to compile '() { return
postEvent; }'.

A simple eval like this already throws the error:

final eval = EvalOnDartLibrary(
  'dart:core',
  serviceManager.service!,
  serviceManager: serviceManager,
);

await eval.asyncEval('Future.value(42)', isAlive: isAlive)

@kenzieschmoll I encountered this error while working on the shared_preferences_tools issue.

adsonpleal avatar May 16 '24 19:05 adsonpleal

@elliette any idea what that ChromeProxyService error is related to? or why async evals would be broken on the web?

kenzieschmoll avatar May 16 '24 23:05 kenzieschmoll

I did a little investigation into this, but unfortunately I know very little about how expression evaluation works. @nshahan and @Markzipan might have more context here.

  • The evaluation that is causing an issue is this first eval to postEvent which I see from the comment was added as a "workaround".
  • DWDS converts the postEvent expression to a lambda function () {return postEvent;}
  • And then the call to the FrontendServerClient to compile the expression to JS is failing with the error seen above: InternalError frontend server failed to compile '() { return postEvent; }'

I think the easiest solution would probably be to avoid sending the postEvent workaround if possible, otherwise this will require modifying the frontend server client. FYI @CoderDake as well

elliette avatar May 17 '24 17:05 elliette

@kenzieschmoll, I think we'll need to solve this issue before merging https://github.com/flutter/packages/pull/6749. If we merge the PR before this fix the extension won't work for web targets.

adsonpleal avatar May 21 '24 19:05 adsonpleal

I don't think we can avoid the postEvent call here, otherwise we would not complete the future that waits for the extension event here: https://github.com/flutter/devtools/blob/0547de50860c9913a76d30bfad6526938baee7b2/packages/devtools_app_shared/lib/src/service/eval_on_dart_library.dart#L397

@nshahan or @Markzipan - do you know what would it take to fix the issue that @elliette described?

kenzieschmoll avatar May 21 '24 21:05 kenzieschmoll

@kenzieschmoll Can you help me understand the priority of this request?

I spent a little time investigating but didn't get to any definitive answers. I did notice two of our expression evaluation tests are currently skipped with comments that the features are not supported at this time. They are evaluating an expression in the context of an SDK library, and await in an expression evaluation. I believe those features could be blocking this request. From the history, I can't really tell if those are big projects to get working or not.

@sigmundch mentioned that he might have time to take a look at the feasibility of getting something working quickly.

nshahan avatar May 22 '24 23:05 nshahan

This is blocking the package:shared_preferences DevTools extension from landing (https://github.com/flutter/flutter/issues/145433), and I think this is causing errors in the package:provider DevTools extension as well, which also uses asyncEval: https://github.com/flutter/devtools/issues/6103.

@sigmundch if there was a way to get something working quickly, that would be ideal. Otherwise, we may have to have these extension author disable the extension for Web users, or remove the asyncEval utility completely from our devtools_app_shared package (which would be breaking for all DevTools extensions already using this utility).

CC @bkonyi if you have any suggestions based on how these evaluations are supported for the VM.

kenzieschmoll avatar May 23 '24 15:05 kenzieschmoll

I took brief look yesterday and don't have a good gauge on the complexity, yet. Looking back at the history behind this, the skip has been there for nearly 4 years, so I wouldn't be surprised if adding eval support within the SDK could be something that requires some non-trivial effort.

If that's the case, my inclination is to either look for workarounds or disable these extensions on Web. There are two workarounds I think are feasible here to remove the use of the evan-in-SDK from this workflow:

  • expose postEvent in the bootstrap logic
  • import postEvent in the snippet of JavaScript we generate for expression evaluation

sigmundch avatar May 23 '24 16:05 sigmundch

Looking back at the history behind this, the skip has been there for nearly 4 years, so I wouldn't be surprised if adding eval support within the SDK could be something that requires some non-trivial effort.

Another possibility is that the support was not needed at the time and just went to the backlog until someone requested it like they are now. It could be not much work but our problem right now is that no one is knowledgeable enough about this part of the stack to know if it is or not.

nshahan avatar May 23 '24 16:05 nshahan

This would also be quite a bit of work (both for web and non-web), but I wonder if long-term it would make sense to add an asyncEvaluate method to the VM Service? From my understanding, DevTools has its own asyncEval implementation because the regular evaluate provided by the VM service doesn't support awaiting Futures. That way DevTools wouldn't need a custom implementation depending on postEvent.

elliette avatar May 23 '24 16:05 elliette

Quick update: currently the expression-compiler (used indirectly via dwds) relies on many pieces to properly support expression evaluation that are not available for the Dart SDK today (e.g. properly plumbed full dill files, source-maps, potentially other metadata). As such, I believe that full expression evaluation support in the SDK would be large effort and not worth it at the moment.

However, since the use here for postEvent only requires a library-level eval, I believe we can elide some of the steps of expression evaluation. In particular, there is no need for a full dill to find the enclosing scope, since we know the scope is simply a library. It's not implemented that way today, but it may be possible to change his to support this use case. I'll investigate this further and share any updates.

sigmundch avatar May 23 '24 23:05 sigmundch

I found a way to support library level evaluation locally in the expression compiler service. I just sent CL 367981 out for review.

The CL only covers the changes needed in the SDK. I don't believe we need changes in DWDS or flutter tools, but I haven't tested that either, so there is a chance we need to do some additional tweaks.

@kenzieschmoll or @elliette - do you have instructions or an easy way to validate it all works end to end with a custom build of the SDK? Thanks!

sigmundch avatar May 24 '24 14:05 sigmundch

@kenzieschmoll or @elliette - do you have instructions or an easy way to validate it all works end to end with a custom build of the SDK? Thanks!

It won't be exactly the same configuration, but the easiest way to check might be to copy your SDK changes into google3, and then patch cl/636945863 and follow the instructions there.

Otherwise, I think validation would require:

  • configuring local checkout of Flutter to use local Dart SDK build (I know this is possible but I've never done it)
  • cloning devtools and adding the changes in cl/636945863
  • run devtools locally: https://github.com/flutter/devtools/blob/master/CONTRIBUTING.md#running-and-debugging-devtools
  • connect devtools to a test flutter web app: https://github.com/flutter/devtools/blob/master/CONTRIBUTING.md#connect-devtools-to-a-test-application

elliette avatar May 24 '24 16:05 elliette

For the last two steps @elliette outlined (running DevTools and connecting DevTools to a test web app) here are some additional details that will help. I was able to repro the error by doing the following:

  1. Patch the changes at the bottom of this comment before running DevTools.
  2. Run DevTools with flutter run -d chrome from the devtools/packages/devtools_app directory.
  3. Run Flutter gallery on web, also with flutter run -d chrome, but your version of flutter will need to have your local changes. You might also be able to just use a Dart web app if it is complicated to get flutter running with a local dart sdk.
  4. Copy the VM service URI and paste into the DevTools connect dialog.
  5. Once you are connected, click the "About" button in the upper right of Devtools.

In about_dialog.dart

import 'package:devtools_app_shared/service.dart';
...

class OpenAboutAction extends ScaffoldAction {
  OpenAboutAction({super.key, super.color})
      : super(
          icon: Icons.help_outline,
          tooltip: 'About DevTools',
          onPressed: (context) async {
            final eval = EvalOnDartLibrary(
              'dart:core',
              serviceConnection.serviceManager.service!,
              serviceManager: serviceConnection.serviceManager,
            );
            await eval.asyncEval('Future.value(42)', isAlive: Disposable());
            // unawaited(
            //   showDialog(
            //     context: context,
            //     builder: (context) => DevToolsAboutDialog(
            //       Provider.of<ReleaseNotesController>(context),
            //     ),
            //   ),
            // );
          },
        );
}

kenzieschmoll avatar May 24 '24 17:05 kenzieschmoll

Thanks for the repro instructions. The internal patch was super useful, and makes testing much much easier. Testing it helped me discover an inconsistency between dwds and the expression compiler worker assumptions (the internal tests assumed a location like uri:1:1 to indicate a library level eval, but dwds used uri:0:0).

After fixing the inconsistency in the SDK change, I see the safeEval progressing and sending the request, so this is promising! I don't yet see the valid result (see a cancellation exception instead), but that may be an unrelated issue.

sigmundch avatar May 24 '24 19:05 sigmundch