Rework service extensions to be safe

sdk/lib/developer/extension.dart

 // Copyright (c) 2015, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 part of dart.developer;
 
 class ServiceExtensionResponse {
   final String _result;
   final int _errorCode;
   final String _errorDetail;
@@ skipping 68 matching lines @@
 
 /// A service protocol extension handler. Registered with [registerExtension].
 ///
 /// Must complete to a [ServiceExtensionResponse].
 ///
 /// [method] - the method name.
 /// [parameters] - the parameters.
 typedef Future<ServiceExtensionResponse>
     ServiceExtensionHandler(String method, Map parameters);
 
-final _extensions = new Map<String, ServiceExtensionHandler>();
-
 /// Register a [ServiceExtensionHandler] that will be invoked in this isolate
 /// for [method].
 void registerExtension(String method, ServiceExtensionHandler handler) {

[Ivan Posva, 2015/08/26 04:47:00]

Can you clear/remove an extension by registering a null handler?

[Cutch, 2015/08/26 14:28:00]

No. Registering a null handler is not allowed by the check:

if (handler is! ServiceExtensionHandler) 

I do not intend for extensions to be removable.

-  if (_extensions[method] != null) {
+  if (method is! String) {
+    throw new ArgumentError.value(method,
+                                  'method',
+                                  'Must be a String');
+  }
+  if (_lookupExtension(method) != null) {
     throw new ArgumentError('Extension already registered: $method');
   }
   if (handler is! ServiceExtensionHandler) {
     throw new ArgumentError.value(handler,
                                   'handler',
                                   'Must be a ServiceExtensionHandler');
   }
-  _extensions[method] = handler;
+  _registerExtension(method, handler);
 }
 
-bool _scheduleExtension(String method,
-                        List<String> parameterKeys,
-                        List<String> parameterValues,
-                        SendPort replyPort,
-                        Object id) {
-  ServiceExtensionHandler handler = _extensions[method];
-  if (handler == null) {
-    return false;
+external ServiceExtensionHandler _lookupExtension(String method);
+external _registerExtension(String method, ServiceExtensionHandler handler);
+
+// This code is only invoked when there is no other Dart code on the stack.
+_runExtension(ServiceExtensionHandler handler,
+              String method,
+              List<String> parameterKeys,
+              List<String> parameterValues,
+              SendPort replyPort,
+              Object id) {
+  var parameters = {};
+  for (var i = 0; i < parameterKeys.length; i++) {
+    parameters[parameterKeys[i]] = parameterValues[i];
   }
-  // Defer execution of handler until next event loop.
-  Timer.run(() {
-    var parameters = {};
-    for (var i = 0; i < parameterKeys.length; i++) {
-      parameters[parameterKeys[i]] = parameterValues[i];
-    }
-    var response;
-    try {
-      response = handler(method, parameters);
-    } catch (e, st) {
-      var errorDetails = (st == null) ? '$e' : '$e\n$st';
+  var response;
+  try {
+    response = handler(method, parameters);
+  } catch (e, st) {
+    var errorDetails = (st == null) ? '$e' : '$e\n$st';
+    response = new ServiceExtensionResponse.error(
+        ServiceExtensionResponse.kExtensionError,
+        errorDetails);
+    _postResponse(replyPort, id, response);
+    return;
+  }
+  if (response is! Future) {
+    response = new ServiceExtensionResponse.error(
+          ServiceExtensionResponse.kExtensionError,
+          "Extension handler must return a Future");
+    _postResponse(replyPort, id, response);
+    return;
+  }
+  response.catchError((e, st) {

[Ivan Posva, 2015/08/26 04:47:00]

Usually I would expect to chain this in a normal try-catch fashion:
response.then(block, onError: catch_block)

Also, what happens if errors are thrown in the then or catchError blocks?

[Cutch, 2015/08/26 14:28:00]

I'm eagerly catching errors and wrapping them in a ServiceExtensionResponse so
that .then branch can just post the error as a response. I've added comments to
explain this.

No errors can be thrown in the .then or the .catchError blocks. The code in
those blocks is internal library code that doesn't call any non-internal library
code. As a precaution, I've added a .catchError block at the end that silently
consumes the error.

Finally, if any errors do escape _runExtension the code that invokes them
(Isolate::InvokePendingExtensionCalls) handles the errors in C++ and sends them
over the service protocol.

+    var errorDetails = (st == null) ? '$e' : '$e\n$st';
+    return new ServiceExtensionResponse.error(
+        ServiceExtensionResponse.kExtensionError,
+        errorDetails);
+  }).then((response) {
+    if (response == null) {
       response = new ServiceExtensionResponse.error(
           ServiceExtensionResponse.kExtensionError,
-          errorDetails);
-      _postResponse(replyPort, id, response);
-      return;
+          "Extension handler returned null");
     }
-    if (response is! Future) {
-      response = new ServiceExtensionResponse.error(
-            ServiceExtensionResponse.kExtensionError,
-            "Extension handler must return a Future");
-      _postResponse(replyPort, id, response);
-      return;
-    }
-    response.catchError((e, st) {
-      var errorDetails = (st == null) ? '$e' : '$e\n$st';
-      return new ServiceExtensionResponse.error(
-          ServiceExtensionResponse.kExtensionError,
-          errorDetails);
-    }).then((response) {
-      if (response == null) {
-        response = new ServiceExtensionResponse.error(
-            ServiceExtensionResponse.kExtensionError,
-            "Extension handler returned null");
-      }
-      _postResponse(replyPort, id, response);
-    });
+    _postResponse(replyPort, id, response);
   });
-  return true;
 }
 
+// This code is only invoked by _runExtension.
 _postResponse(SendPort replyPort,
               Object id,
               ServiceExtensionResponse response) {
   assert(replyPort != null);
   if (id == null) {
     // No id -> no response.
     replyPort.send(null);
     return;
   }
   assert(id != null);
   StringBuffer sb = new StringBuffer();
   sb.write('{"jsonrpc":"2.0",');
   if (response._isError()) {
     sb.write('"error":');
   } else {
     sb.write('"result":');
   }
   sb.write('${response._toString()},');
   if (id is String) {
     sb.write('"id":"$id"}');
   } else {
     sb.write('"id":$id}');
   }
   replyPort.send(sb.toString());
 }