Rework service extensions to be safe

sdk/lib/developer/extension.dart

Index: sdk/lib/developer/extension.dart
diff --git a/sdk/lib/developer/extension.dart b/sdk/lib/developer/extension.dart
index f23b5ad5630b266f35c0ef1576cedf6595374ff8..543e8a76c5bb39a21a94003f15b69891bc337242 100644
--- a/sdk/lib/developer/extension.dart
+++ b/sdk/lib/developer/extension.dart
@@ -86,12 +86,15 @@ class ServiceExtensionResponse {
 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) {
@@ -99,59 +102,57 @@ void registerExtension(String method, ServiceExtensionHandler 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';
-      response = new ServiceExtensionResponse.error(
+  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,
-          errorDetails);
-      _postResponse(replyPort, id, response);
-      return;
-    }
-    if (response is! Future) {
+          "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,
-            "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);
-    });
+          "Extension handler returned null");
+    }
+    _postResponse(replyPort, id, response);
   });
-  return true;
 }
 
+// This code is only invoked by _runExtension.
 _postResponse(SendPort replyPort,
               Object id,
               ServiceExtensionResponse response) {