From 439ed85b72ce1c62b7d2b91e832e025b31bfcbf3 Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Wed, 1 May 2019 11:04:13 +0100 Subject: [PATCH 1/3] Ensure launchDevTools always returns success or a valid error Fixes #617. --- packages/devtools_server/lib/src/server.dart | 78 +++++++++++--------- 1 file changed, 45 insertions(+), 33 deletions(-) diff --git a/packages/devtools_server/lib/src/server.dart b/packages/devtools_server/lib/src/server.dart index 26c912c02ec..85f492b6d55 100644 --- a/packages/devtools_server/lib/src/server.dart +++ b/packages/devtools_server/lib/src/server.dart @@ -22,6 +22,8 @@ const argMachine = 'machine'; const argPort = 'port'; const launchDevToolsService = 'launchDevTools'; +const errorLaunchingBrowserCode = 500; + final argParser = new ArgParser() ..addFlag( argHelp, @@ -200,41 +202,51 @@ Future registerLaunchDevToolsService( final VmService service = await _connectToVmService(vmServiceUri); service.registerServiceCallback(launchDevToolsService, (params) async { - final uriParams = {}; - - // Copy over queryParams passed by the client - if (params != null) { - params['queryParams']?.forEach((key, value) => uriParams[key] = value); - } + try { + final uriParams = {}; + + // Copy over queryParams passed by the client + if (params != null) { + params['queryParams'] + ?.forEach((key, value) => uriParams[key] = value); + } + + // Add the URI to the VM service + uriParams['uri'] = vmServiceUri.toString(); + + final devToolsUri = Uri.parse(devToolsUrl); + final uriToLaunch = devToolsUri.replace( + // If path is empty, we generate 'http://foo:8000?uri=' (missing `/`) and + // ChromeOS fails to detect that it's a port that's tunneled, and will + // quietly replace the IP with "penguin.linux.test". This is not valid + // for us since the server isn't bound to the containers IP (it's bound + // to the containers loopback IP). + path: devToolsUri.path.isEmpty ? '/' : devToolsUri.path, + queryParameters: uriParams, + ); - // Add the URI to the VM service - uriParams['uri'] = vmServiceUri.toString(); - - final devToolsUri = Uri.parse(devToolsUrl); - final uriToLaunch = devToolsUri.replace( - // If path is empty, we generate 'http://foo:8000?uri=' (missing `/`) and - // ChromeOS fails to detect that it's a port that's tunneled, and will - // quietly replace the IP with "penguin.linux.test". This is not valid - // for us since the server isn't bound to the containers IP (it's bound - // to the containers loopback IP). - path: devToolsUri.path.isEmpty ? '/' : devToolsUri.path, - queryParameters: uriParams, - ); - - // TODO(dantup): When ChromeOS has support for tunneling all ports we - // can change this to always use the native browser for ChromeOS - // and may wish to handle this inside `browser_launcher`. - // https://crbug.com/848063 - final useNativeBrowser = _isChromeOS && - _isAccessibleToChromeOSNativeBrowser(Uri.parse(devToolsUrl)) && - _isAccessibleToChromeOSNativeBrowser(vmServiceUri); - if (useNativeBrowser) { - await Process.start('x-www-browser', [uriToLaunch.toString()]); - } else { - await Chrome.start([uriToLaunch.toString()]); + // TODO(dantup): When ChromeOS has support for tunneling all ports we + // can change this to always use the native browser for ChromeOS + // and may wish to handle this inside `browser_launcher`. + // https://crbug.com/848063 + final useNativeBrowser = _isChromeOS && + _isAccessibleToChromeOSNativeBrowser(Uri.parse(devToolsUrl)) && + _isAccessibleToChromeOSNativeBrowser(vmServiceUri); + if (useNativeBrowser) { + await Process.start('x-www-browser', [uriToLaunch.toString()]); + } else { + await Chrome.start([uriToLaunch.toString()]); + } + + return {'result': Success().toJson()}; + } catch (e) { + return { + 'error': { + 'code': errorLaunchingBrowserCode, + 'message': 'Failed to launch browser: $e', + }, + }; } - - return {'result': Success().toJson()}; }); await service.registerService(launchDevToolsService, 'DevTools Server'); From 49a8c7484f3c547979160b0e77583b052d472457 Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Wed, 1 May 2019 11:27:57 +0100 Subject: [PATCH 2/3] Include stack in the error --- packages/devtools_server/lib/src/server.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/devtools_server/lib/src/server.dart b/packages/devtools_server/lib/src/server.dart index 85f492b6d55..f39a29e9306 100644 --- a/packages/devtools_server/lib/src/server.dart +++ b/packages/devtools_server/lib/src/server.dart @@ -239,11 +239,11 @@ Future registerLaunchDevToolsService( } return {'result': Success().toJson()}; - } catch (e) { + } catch (e, s) { return { 'error': { 'code': errorLaunchingBrowserCode, - 'message': 'Failed to launch browser: $e', + 'message': 'Failed to launch browser: $e\n$s', }, }; } From fa7539956eb7fb2f0cc6535b7de95c9af5b7c038 Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Wed, 1 May 2019 15:05:27 +0100 Subject: [PATCH 3/3] Add a note about the format of the response --- packages/devtools_server/lib/src/server.dart | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/devtools_server/lib/src/server.dart b/packages/devtools_server/lib/src/server.dart index f39a29e9306..fe087ba5877 100644 --- a/packages/devtools_server/lib/src/server.dart +++ b/packages/devtools_server/lib/src/server.dart @@ -240,6 +240,11 @@ Future registerLaunchDevToolsService( return {'result': Success().toJson()}; } catch (e, s) { + // Note: It's critical that we return responses in exactly the right format + // or the VM will unregister the service. The objects must match JSON-RPC + // however a successful response must also have a "type" field in its result. + // Otherwise, we can return an error object (instead of result) that includes + // code + message. return { 'error': { 'code': errorLaunchingBrowserCode,