Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Open DevTools in browser" doesn't work #4832

Closed
DanTup opened this issue Nov 9, 2023 · 7 comments
Closed

"Open DevTools in browser" doesn't work #4832

DanTup opened this issue Nov 9, 2023 · 7 comments
Labels
in commands Relates to commands (usually invoked from the command Palette) in devtools Relates to integration with Dart/Flutter DevTools is bug relies on sdk changes Something that requires changes in the Dart/Flutter SDK to ship before it will become available
Milestone

Comments

@DanTup
Copy link
Member

DanTup commented Nov 9, 2023

While working on #4821 I found that the browser window isn't actually opening. It's not just from the sidebar, the same happens when running the "Open DevTools" command then selected "Open in External Browser".

@DanTup DanTup added the is bug label Nov 9, 2023
@DanTup DanTup added this to the v3.78.0 milestone Nov 9, 2023
@DanTup DanTup added in commands Relates to commands (usually invoked from the command Palette) in devtools Relates to integration with Dart/Flutter DevTools labels Nov 9, 2023
@DanTup
Copy link
Member Author

DanTup commented Nov 9, 2023

Cannot reproduce after a restart :/

Edit: Reproduced but unclear whether it's consistent:

  • Opened Inspector embedded with button from toolbar
  • Made sidebar visible
  • Tried to open externally (failed)

Theory: Sidebar looks like a connected client and it's trying to reuse that window?

@DanTup
Copy link
Member Author

DanTup commented Nov 9, 2023

The sidebar is showing up as a connected client, but it's marked as not embedded so we're trying to reuse it.

We are passing it on the querystring, so it must not be being passed through somewhere.

{
	"event": "client.launch",
	"params": {
		"reused": true,
		"notified": true,
		"pid": null,
		"extra": {
			"reusableClient": {
				"currentPage": "simple",
				"embedded": false, // This is wrong
				"hasConnection": false,
				"vmServiceUri": null
			}
		}
	}
}

@DanTup
Copy link
Member Author

DanTup commented Nov 9, 2023

There's something odd happening here - when we load the sidebar, it appears that DevTools is creating the not-found page also. If I log the registration of the client to the server, it sends a not-found as the page:

image

@DanTup
Copy link
Member Author

DanTup commented Nov 9, 2023

@kenzieschmoll FYI in case you have any ideas about what might be happening here that might save me some debugging.

Seems like there's something weird when loading standalon screens.. If I run DevTools from source and navigate to the sidebar (eg. "http://localhost:55987/vsCodeFlutterPanel") I can trigger a breakpoint on the not-found widget above. The stack trace isn't very helpful but it seems like we go via DevToolsRouterDelegate -> build -> _getPage() to end up doing this. I can't figure out how we're building a not-found page that isn't being rendered anywhere though.

(my original issue is that this not-found page is being registered with the server without its embed flag so I can probably fix that so the embed flag is working correctly, but it feels like something is wrong that we're generating this page)

@kenzieschmoll
Copy link

kenzieschmoll commented Nov 9, 2023

I think this is a race condition and the first load of the routing logic is happening before the router is completely set up. I wonder if there is something else we can check here to prevent us from reaching the "not found" page while we are in this state: https://github.com/flutter/devtools/blob/master/packages/devtools_app/lib/src/app.dart/#L207

DanTup added a commit to DanTup/devtools that referenced this issue Nov 14, 2023
The router is always called with a `null` page when the app first loads. Previously this would render a not-found page and because there are also no args, it would be marked as not-embedded and would register with the server as a page that can be reused which (partly) caused Dart-Code/Dart-Code#4832.
@DanTup
Copy link
Member Author

DanTup commented Nov 14, 2023

Turns out there were a few different issues here:

  1. Rendering not-found initially - fix is in Don't render not-found page during initial setup flutter/devtools#6724
  2. Standalone screens not sending a currentPage event, so once (1) is fixed, the clients connect to the server but never tell it whether they're embedded or not (because that's sent in the currentPage request) - TODO added in Don't render not-found page during initial setup flutter/devtools#6724
  3. The DevTools server treated newly-connected clients as reusable when we'd never been sent an embedded flag - fix is in https://dart-review.googlesource.com/c/sdk/+/336041 which adds a flag to indicate whether we've ever been sent a page

DanTup added a commit to flutter/devtools that referenced this issue Nov 14, 2023
The router is always called with a `null` page when the app first loads. Previously this would render a not-found page and because there are also no args, it would be marked as not-embedded and would register with the server as a page that can be reused which (partly) caused Dart-Code/Dart-Code#4832.
copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Nov 14, 2023
…lly initialized

It's only valid to reuse a client that is not showing an embedded page, however we only get the embedded flag when a client sends a "currentPage" event.

There is a period between a client connecting and sending this event where we would consider it reusable when it's not. This fixes that by keeping a flag to indicate if a client has completed initializing (that is, it has sent its initial page).

See Dart-Code/Dart-Code#4832

Change-Id: I9f2d43d1537ee97e4e231a844831e7548cf07beb
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/336041
Reviewed-by: Ben Konyi <bkonyi@google.com>
Reviewed-by: Kenzie Davisson <kenzieschmoll@google.com>
Commit-Queue: Ben Konyi <bkonyi@google.com>
@DanTup
Copy link
Member Author

DanTup commented Nov 15, 2023

This was fixed in DevTools and the SDK by dart-lang/sdk@c74ea63 and dart-lang/sdk@c74ea63

@DanTup DanTup closed this as completed Nov 15, 2023
@DanTup DanTup added the relies on sdk changes Something that requires changes in the Dart/Flutter SDK to ship before it will become available label Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in commands Relates to commands (usually invoked from the command Palette) in devtools Relates to integration with Dart/Flutter DevTools is bug relies on sdk changes Something that requires changes in the Dart/Flutter SDK to ship before it will become available
Projects
None yet
Development

No branches or pull requests

2 participants