dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.22k stars 1.57k forks source link

[vm_service] isolate.rootLib returns the wrong library on the web. #44760

Open Norbert515 opened 3 years ago

Norbert515 commented 3 years ago

When connecting to an application and invoking getIsolate, the root library differs depending on whether it is run on the web or not.

The application it connects to:

import 'package:flutter/material.dart';

void main() {
  runApp(MyApp());
}

class MyApp extends StatelessWidget {
  // This widget is the root of your application.
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        primarySwatch: Colors.blue,
        visualDensity: VisualDensity.adaptivePlatformDensity,
      ),
      home: MyHomePage(title: 'Flutter Demo Home Page'),
    );
  }
}

class MyHomePage extends StatefulWidget {
  MyHomePage({Key key, this.title}) : super(key: key);

  final String title;

  @override
  _MyHomePageState createState() => _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> {

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: Text(widget.title),
      ),
    );
  }
}

Flutter doctor:

[√] Flutter (Channel dev, 1.26.0-1.0.pre, on Microsoft Windows [Version 10.0.19042.746], locale de-DE)
    • Flutter version 1.26.0-1.0.pre at C:\Users\norbe\fvm\versions\dev
    • Framework revision 63062a6443 (6 weeks ago), 2020-12-13 23:19:13 +0800
    • Engine revision 4797b06652
    • Dart version 2.12.0 (build 2.12.0-141.0.dev)

[√] Android toolchain - develop for Android devices (Android SDK version 30.0.3)
    • Android SDK at C:\Users\norbe\AppData\Local\Android\sdk
    • Platform android-30, build-tools 30.0.3
    • Java binary at: C:\Program Files\Android\Android Studio\jre\bin\java
    • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b01)
    • All Android licenses accepted.

[√] Chrome - develop for the web
    • Chrome at C:\Program Files\Google\Chrome\Application\chrome.exe

[√] Visual Studio - develop for Windows (Visual Studio Community 2019 16.8.3)
    • Visual Studio at C:\Program Files (x86)\Microsoft Visual Studio\2019\Community
    • Visual Studio Community 2019 version 16.8.30804.86
    • Windows 10 SDK version 10.0.18362.0

[√] Android Studio (version 4.1.0)
    • Android Studio at C:\Program Files\Android\Android Studio
    • Flutter plugin can be installed from:
       https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
       https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b01)

[√] IntelliJ IDEA Community Edition (version 2020.3)
    • IntelliJ at C:\Program Files\JetBrains\IntelliJ IDEA Community Edition 2020.3.1
    • Flutter plugin version 52.1.5
    • Dart plugin version 203.6912

[√] Connected device (3 available)
    • Windows (desktop) • windows • windows-x64    • Microsoft Windows [Version 10.0.19042.746]
    • Chrome (web)      • chrome  • web-javascript • Google Chrome 88.0.4324.104
    • Edge (web)        • edge    • web-javascript • Microsoft Edge 88.0.705.50

• No issues found!
rayliverified commented 3 years ago

This issue is important because it is blocking the very wonderful detective.dev from working on Flutter Web. Upvoted!

mraleph commented 3 years ago

Hard to say which one is correct. Tentatively marking as area-vm. @bkonyi could you make a call and reassign the label if VM is right?

knopp commented 3 years ago

Mildly related, I'm experiencing issue where rootLib.uri (desktop, macos) returns package:// uri instead of file:// uri (for the same script), which breaks DartCode attach to flutter functionality, since it uses the rootLib.uri to resolve package_config.json location. Any idea why?

https://github.com/Dart-Code/Dart-Code/issues/3128

jacob314 commented 3 years ago

This is a DWDS issue. Unless there is a reason we can't or the VM behavior is clearly wrong we should always have DWDS match the VM behavior. In this case I think the VM behavior is preferred and I think DWDS running with Flutter Web should match it.

grouma commented 3 years ago

cc @annagrin

It looks like there are two issues here:

The first issue is that we are not returning the correct rootLib. The logic for that is here. It looks like we have a heuristic that probably works for pacakge:build_runner and our internal Bazel rules but doesn't work for Flutter Tools and the Frontend Server. This should be updated.

The second issue is that we are returning a package URI. This is intentional. We use the import URI contained in the DDC metadata files. We use the import URI because this is the URI that DDC understands. We use this convention throughout package:dwds, for example here.

annagrin commented 3 years ago

@grouma I will take a look at the incorrect rootLib (will check if it is due to reading libraries from the metadata)

jacob314 commented 3 years ago

We need to standardize whether we emit package uris or file paths for files technically outside of a package. This divergence has caused bugs in multiple tools now. I'm fine with converging in either direction (DDC or VM) but we need to converge. Modifying the VM implementation to match DDC would likely break more tools so by default I'm in favor of modifying DWDS to match the VM.

sigmundch commented 3 years ago

Agree with @jacob314.

I'd like to understand why we are using package:uris in the first place. In dart2js we track both import and file URIs together so we can choose the most appropriate on a given moment (import URIs are used for example when the identity of the import is relevant for semantic reasons, but file URIs are used when mapping source-maps and reporting location data, since most likely users need to fetch the file directly). Do we need to store both? Will file-Uris be sufficient in the DDC case?

grouma commented 3 years ago

cc @annagrin looked into this. The issue is that we have different bootstrapping logic which may wrap the main libraries. We should have enough information in package:dwds to do the right thing for rootlibraries.

annagrin commented 3 years ago

Update: This is taking some time to figure out correct solution that works for all our scenarios, will keep updating the thread.

annagrin commented 3 years ago

Update: to solve this properly for all scenarios, we need to communicate the entrypoint from the build to the debugger (see mentioned issue above): https://github.com/dart-lang/webdev/issues/1290.

To unblock detective.dev, I am working on a better heuristic that works for this usage scenario and works well with other fixes (such as missing dart_sdk libraries: https://github.com/dart-lang/webdev/issues/1226

Note: the main isolate for web is actually not the same as for vm - flutter tools wraps the main in another dart file called "web_entrypoint.dart", so the correct web rootLib looks like org-dartlang-app:/web_entrypoint.dart.

@jacob314 @searchy2 will this difference matter to the tools you are developing?

Flutter_tools could artificially change the rootLib to the user's main if needed, once we have the app metadata in place.

rayliverified commented 3 years ago

@Norbert515 is developing detective.dev

I would love to use detective for speeding up development when building in Flutter Web.

annagrin commented 3 years ago

@Norbert515 the PR above makes vmService.rootLib returns org-dartlang-app:/web_entrypoint.dart, which is the actual entry point of the app from the vm perspective on web platform. Will this work for your purposes?

After we add more metadata from the builds we will be able to change it to the user's main library, if it is still needed.

Norbert515 commented 3 years ago

I use the rootLib for two purposes:

  1. Iterate through all library dependencies
  2. Use its prefix to determine if a library originates from the user or not (not critical)

I suppose the PR would allow for 1 to work, but not 2. This would already be super helpful though.

annagrin commented 3 years ago

@Norbert515 item 2 will take a bit longer as it requires the build to save the entrypoint in the metadata to work for all debugging scenarios.

annagrin commented 2 years ago

Forgot to updated this. Part 1 from @Norbert515 's request is in current flutter stable.