devtools_extensions: asyncEval fails on web targets

adsonpleal commented 1 month ago

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.

kenzieschmoll commented 1 month ago

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

elliette commented 1 month ago

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

adsonpleal commented 1 month ago

@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.

kenzieschmoll commented 1 month ago

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?

nshahan commented 1 month ago

@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.

kenzieschmoll commented 1 month ago

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.

sigmundch commented 1 month ago

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

nshahan commented 1 month ago

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.

elliette commented 1 month ago

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.

sigmundch commented 1 month ago

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 commented 1 month ago

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!

elliette commented 1 month ago

@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

kenzieschmoll commented 1 month ago

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:

Patch the changes at the bottom of this comment before running DevTools.
Run DevTools with flutter run -d chrome from the devtools/packages/devtools_app directory.
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.
Copy the VM service URI and paste into the DevTools connect dialog.
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),
            //     ),
            //   ),
            // );
          },
        );
}

sigmundch commented 1 month ago

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.

kenzieschmoll commented 4 weeks ago

@sigmundch it appears even with your fix that we are still seeing the issue: see this comment.

sigmundch commented 4 weeks ago

bummer :( - I'm unfortunately out sick, so it will take me some time to get back to this and investigate. I'll chime in on the other thread in case there are some hints we can get from them in the meantime.

sigmundch commented 3 weeks ago

Unfortunately, the issue is that flutter-tool uses a completely different codepath than webdev and our internal integration for this feature. The original fix, which I validated it worked in the other environments, needs to be replicated into the frontend-server. This sever, however, uses a different architecture for managing its state and evaluating expressions, so it requires a brand new investigation and solution.

As such, I'd not recommend blocking the other PR on this anymore.

flutter / devtools

devtools_extensions: asyncEval fails on web targets #7766