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.

Sign up for GitHub

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

Tweaks to browser-launching service #563

Merged
merged 12 commits into from
Apr 25, 2019
Merged

Tweaks to browser-launching service #563

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
609fe73
Allow ID to be any type
DanTup Apr 22, 2019
b86c187
Change success flag for non-VM-service to simple bool
DanTup Apr 22, 2019
e8c4b9a
Ensure we handle all types of valid VM service URIs
DanTup Apr 22, 2019
f1a1caa
Use the `method` field to decide how to handle the request
DanTup Apr 22, 2019
a75b75e
Send both `event` and `method` for compatibility
DanTup Apr 22, 2019
bb80e3e
Comment valid types for IDs
DanTup Apr 22, 2019
a794d89
Fix return type
DanTup Apr 22, 2019
8addfa3
Ensure we always respond to vm.register even if URLs are invalid
DanTup Apr 23, 2019
3a6a9c5
Fix comment
DanTup Apr 23, 2019
cecfec5
Add a TODO
DanTup Apr 23, 2019
3a19128
Move to vm_service_lib-provided helper
DanTup Apr 24, 2019
faba3b9
Fix error message
DanTup Apr 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/devtools/lib/src/framework/framework_core.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import 'dart:async';
import 'dart:html' hide Screen;

import 'package:devtools/src/utils.dart';
import 'package:vm_service_lib/utils.dart';

import '../../devtools.dart' as devtools show version;
import '../core/message_bus.dart';
Expand Down Expand Up @@ -45,7 +45,7 @@ class FrameworkCore {

// Map the URI (which may be Observatory web app) to a WebSocket URI for
// the VM service.
uri = getVmServiceUriFromObservatoryUri(uri);
uri = convertToWebSocketUrl(serviceProtocolUrl: uri);

try {
final VmServiceWrapper service = await connect(uri, finishedCompleter);
Expand Down
14 changes: 0 additions & 14 deletions packages/devtools/lib/src/utils.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,20 +163,6 @@ class Property<T> {
Stream<T> get onValueChange => _changeController.stream;
}

/// Map the URI (which may already be Observatory web app) to a WebSocket URI
/// for the VM service. If the URI is already a VM Service WebSocket URI it
/// will not be modified.
Uri getVmServiceUriFromObservatoryUri(Uri uri) {
final isSecure = uri.isScheme('wss') || uri.isScheme('https');
final scheme = isSecure ? 'wss' : 'ws';

final path = uri.path.endsWith('/ws')
? uri.path
: (uri.path.endsWith('/') ? '${uri.path}ws' : '${uri.path}/ws');

return uri.replace(scheme: scheme, path: path);
}

/// A typedef to represent a function taking no arguments and with no return
/// value.
typedef VoidFunction = void Function();
Expand Down
2 changes: 1 addition & 1 deletion packages/devtools/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ dependencies:
pedantic: ^1.4.0
platform_detect: ^1.3.5
rxdart: ^0.21.0
vm_service_lib: ^3.14.3-dev.4
vm_service_lib: ^3.15.1+1
# We would use local dependencies for these packages if pub publish allowed it.
octicons_css:
^0.0.1
Expand Down
4 changes: 2 additions & 2 deletions packages/devtools/test/support/cli_test_driver.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ import 'dart:async';
import 'dart:convert';
import 'dart:io';

import 'package:devtools/src/utils.dart';
import 'package:devtools/src/vm_service_wrapper.dart';
import 'package:vm_service_lib/utils.dart';
import 'package:vm_service_lib/vm_service_lib.dart';
import 'package:vm_service_lib/vm_service_lib_io.dart';

Expand Down Expand Up @@ -105,7 +105,7 @@ class CliAppFixture extends AppFixture {
}

// Map to WS URI.
uri = getVmServiceUriFromObservatoryUri(uri);
uri = convertToWebSocketUrl(serviceProtocolUrl: uri);

final VmServiceWrapper serviceConnection =
VmServiceWrapper(await vmServiceConnectUri(uri.toString()));
Expand Down
5 changes: 3 additions & 2 deletions packages/devtools/test/support/flutter_test_driver.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ import 'dart:async';
import 'dart:convert';
import 'dart:io';

import 'package:devtools/src/utils.dart';
import 'package:devtools/src/vm_service_wrapper.dart';
import 'package:pedantic/pedantic.dart';
import 'package:vm_service_lib/utils.dart';
import 'package:vm_service_lib/vm_service_lib.dart';
import 'package:vm_service_lib/vm_service_lib_io.dart';

Expand Down Expand Up @@ -324,7 +324,8 @@ class FlutterRunTestDriver extends FlutterTestDriver {
_vmServiceWsUri = Uri.parse(wsUriString);

// Map to WS URI.
_vmServiceWsUri = getVmServiceUriFromObservatoryUri(_vmServiceWsUri);
_vmServiceWsUri =
convertToWebSocketUrl(serviceProtocolUrl: _vmServiceWsUri);

vmService = VmServiceWrapper(
await vmServiceConnectUri(_vmServiceWsUri.toString()),
Expand Down
89 changes: 63 additions & 26 deletions packages/devtools_server/lib/src/server.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import 'package:path/path.dart' as path;
import 'package:shelf/shelf.dart' as shelf;
import 'package:shelf/shelf_io.dart' as shelf;
import 'package:shelf_static/shelf_static.dart';
import 'package:vm_service_lib/utils.dart';
import 'package:vm_service_lib/vm_service_lib.dart' hide Isolate;

import 'chrome.dart';
Expand Down Expand Up @@ -102,6 +103,10 @@ void serveDevTools({
printOutput(
'Serving DevTools at $devToolsUrl',
{
'event': 'server.started',
// TODO(dantup): Remove this `method` field when we're sure VS Code users
// are all on a newer version that uses `event`. We incorrectly used
// `method` for the original releases.
'method': 'server.started',
'params': {'host': server.address.host, 'port': server.port, 'pid': pid}
},
Expand All @@ -125,39 +130,68 @@ void serveDevTools({
// }
// }
_stdinCommandStream.listen((Map<String, dynamic> json) async {
final int id = json['id'];
// ID can be String, int or null
final dynamic id = json['id'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What types might we get here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VS Code has always used strings. When I changed this, I thought I checked the spec and it said "any value", however that was actually Wikipedia! The spec says:

An identifier established by the Client that MUST contain a String, Number, or NULL value if included.

I don't think we follow JSON-RPC particularly strictly, but I do think we should support both int+string here (I'm using shared code used for Flutter/analysis server/etc. which used strings, and it was just crashing on this line).

I'm hoping one day we can support int | String!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I've added a comment about the types)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment helps a lot. Thanks!

final Map<String, dynamic> params = json['params'];

if (!params.containsKey('uri')) {
printOutput(
'Invalid input: $params does not contain the key \'uri\'',
{
'id': id,
'error': 'Invalid input: $params does not contain the key \'uri\'',
},
machineMode: machineMode,
);
switch (json['method']) {
case 'vm.register':
await _handleVmRegister(id, params, machineMode, devToolsUrl);
break;
default:
printOutput(
'Unknown method ${json['method']}',
{
'id': id,
'error': 'Unknown method ${json['method']}',
},
machineMode: machineMode,
);
}

// json['uri'] should contain a vm service uri.
final uri = Uri.parse(params['uri']);

// Lots of things are considered valid URIs (including empty strings
// and single letters) since they can be relative, so we need to do some
// extra checks.
if (uri != null &&
uri.isAbsolute &&
(uri.isScheme('ws') ||
uri.isScheme('wss') ||
uri.isScheme('http') ||
uri.isScheme('https')))
await registerLaunchDevToolsService(uri, id, devToolsUrl, machineMode);
});
}

Future<void> _handleVmRegister(dynamic id, Map<String, dynamic> params,
bool machineMode, String devToolsUrl) async {
if (!params.containsKey('uri')) {
printOutput(
'Invalid input: $params does not contain the key \'uri\'',
{
'id': id,
'error': 'Invalid input: $params does not contain the key \'uri\'',
},
machineMode: machineMode,
);
}

// json['uri'] should contain a vm service uri.
final uri = Uri.tryParse(params['uri']);

// Lots of things are considered valid URIs (including empty strings
// and single letters) since they can be relative, so we need to do some
// extra checks.
if (uri != null &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while you are here we should really be failing if the uri is not valid instead of just quietly pretending everything succeeded.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good spot - this code wasn't sending any response back (so if it happened, VS Code would just sit waiting). It also would throw on the Uri.parse instead of giving a well-formed error, so I've changed that too.

uri.isAbsolute &&
(uri.isScheme('ws') ||
uri.isScheme('wss') ||
uri.isScheme('http') ||
uri.isScheme('https'))) {
await registerLaunchDevToolsService(uri, id, devToolsUrl, machineMode);
} else {
printOutput(
'Uri must be absolute with a http, https, ws or wss scheme',
{
'id': id,
'error': 'Uri must be absolute with a http, https, ws or wss scheme',
},
machineMode: machineMode,
);
}
}

Future<void> registerLaunchDevToolsService(
Uri uri,
int id,
dynamic id,
String devToolsUrl,
bool machineMode,
) async {
Expand All @@ -184,7 +218,7 @@ Future<void> registerLaunchDevToolsService(
'Successfully registered launchDevTools service',
{
'id': id,
'result': Success().toJson(),
'result': {'success': true},
},
machineMode: machineMode,
);
Expand All @@ -201,6 +235,9 @@ Future<void> registerLaunchDevToolsService(
}

Future<VmService> _connectToVmService(Uri uri) async {
// Fix up the various acceptable URI formats into a WebSocket URI to connect.
uri = convertToWebSocketUrl(serviceProtocolUrl: uri);

final WebSocket ws = await WebSocket.connect(uri.toString());

final VmService service = VmService(
Expand Down
2 changes: 1 addition & 1 deletion packages/devtools_server/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,5 @@ dependencies:
pedantic: ^1.4.0
shelf: ^0.7.4
shelf_static: ^0.2.8
vm_service_lib: ^3.14.3-dev.4
vm_service_lib: ^3.15.1+1
webkit_inspection_protocol: ^0.4.0