Add a timeout to every command (enforced on device and host) (#7391)

packages/flutter_driver/lib/src/driver.dart

@@ -98,7 +98,7 @@ class FlutterDriver {
   static const String _kFlutterExtensionMethod = 'ext.flutter.driver';
   static const String _kSetVMTimelineFlagsMethod = '_setVMTimelineFlags';
   static const String _kGetVMTimelineMethod = '_getVMTimeline';
-  static const Duration _kDefaultTimeout = const Duration(seconds: 5);
+  static const Duration _kRpcGraceTime = const Duration(seconds: 2);
 
   /// Connects to a Flutter application.
   ///
@@ -227,10 +227,17 @@ class FlutterDriver {
   final VMIsolateRef _appIsolate;
 
   Future<Map<String, dynamic>> _sendCommand(Command command) async {
-    Map<String, String> parameters = <String, String>{'command': command.kind}
-      ..addAll(command.serialize());
+    Map<String, dynamic> response;
     try {
-      return await _appIsolate.invokeExtension(_kFlutterExtensionMethod, parameters);
+       response = await _appIsolate
+          .invokeExtension(_kFlutterExtensionMethod, command.serialize())
+          .timeout(command.timeout + _kRpcGraceTime);
+    } on TimeoutException catch (error, stackTrace) {
+      throw new DriverError(
+        'Failed to fulfill ${command.runtimeType}: Flutter application not responding',
+        error,
+        stackTrace
+      );
     } catch (error, stackTrace) {
       throw new DriverError(
         'Failed to fulfill ${command.runtimeType} due to remote error',
@@ -238,6 +245,9 @@ class FlutterDriver {
         stackTrace
       );
     }
+    if (response['isError'])
+      throw new DriverError('Error in Flutter application: ${response['response']}');
+    return response['response'];
   }
 
   /// Checks the status of the Flutter Driver extension.
@@ -275,7 +285,7 @@ class FlutterDriver {
   }
 
   /// Waits until [finder] locates the target.
-  Future<Null> waitFor(SerializableFinder finder, {Duration timeout: _kDefaultTimeout}) async {
+  Future<Null> waitFor(SerializableFinder finder, {Duration timeout}) async {
     await _sendCommand(new WaitFor(finder, timeout: timeout));
     return null;
   }

packages/flutter_driver/lib/src/extension.dart

@@ -74,15 +74,15 @@ class _FlutterDriverExtension {
     });
 
     _commandDeserializers.addAll(<String, CommandDeserializerCallback>{
-      'get_health': GetHealth.deserialize,
-      'get_render_tree': GetRenderTree.deserialize,
-      'tap': Tap.deserialize,
-      'get_text': GetText.deserialize,
-      'scroll': Scroll.deserialize,
-      'scrollIntoView': ScrollIntoView.deserialize,
-      'setInputText': SetInputText.deserialize,
-      'submitInputText': SubmitInputText.deserialize,
-      'waitFor': WaitFor.deserialize,
+      'get_health': (Map<String, dynamic> json) => new GetHealth.deserialize(json),
+      'get_render_tree': (Map<String, dynamic> json) => new GetRenderTree.deserialize(json),
+      'tap': (Map<String, dynamic> json) => new Tap.deserialize(json),
+      'get_text': (Map<String, dynamic> json) => new GetText.deserialize(json),
+      'scroll': (Map<String, dynamic> json) => new Scroll.deserialize(json),
+      'scrollIntoView': (Map<String, dynamic> json) => new ScrollIntoView.deserialize(json),
+      'setInputText': (Map<String, dynamic> json) => new SetInputText.deserialize(json),
+      'submitInputText': (Map<String, dynamic> json) => new SubmitInputText.deserialize(json),
+      'waitFor': (Map<String, dynamic> json) => new WaitFor.deserialize(json),
     });
 
     _finders.addAll(<String, FinderConstructor>{
@@ -108,21 +108,34 @@ class _FlutterDriverExtension {
   /// The returned JSON is command specific. Generally the caller deserializes
   /// the result into a subclass of [Result], but that's not strictly required.
   Future<Map<String, dynamic>> call(Map<String, String> params) async {
+    String commandKind = params['command'];
     try {
-      String commandKind = params['command'];
       CommandHandlerCallback commandHandler = _commandHandlers[commandKind];
       CommandDeserializerCallback commandDeserializer =
           _commandDeserializers[commandKind];
       if (commandHandler == null || commandDeserializer == null)
         throw 'Extension $_extensionMethod does not support command $commandKind';
       Command command = commandDeserializer(params);
-      return (await commandHandler(command)).toJson();
+      Result response = await commandHandler(command).timeout(command.timeout);
+      return _makeResponse(response.toJson());
+    } on TimeoutException catch (error, stackTrace) {
+      String msg = 'Timeout while executing $commandKind: $error\n$stackTrace';
+     _log.error(msg);
+      return _makeResponse(msg, isError: true);
     } catch (error, stackTrace) {
-      _log.error('Uncaught extension error: $error\n$stackTrace');
-      rethrow;
+      String msg = 'Uncaught extension error while executing $commandKind: $error\n$stackTrace';
+      _log.error(msg);
+      return _makeResponse(msg, isError: true);
     }
   }
 
+  Map<String, dynamic> _makeResponse(dynamic response, {bool isError: false}) {
+    return <String, dynamic>{
+      'isError': isError,
+      'response': response,
+    };
+  }
+
   Stream<Duration> _onFrameReadyStream;
   Stream<Duration> get _onFrameReady {
     if (_onFrameReadyStream == null) {

packages/flutter_driver/lib/src/find.dart

@@ -20,11 +20,16 @@ DriverError _createInvalidKeyValueTypeError(String invalidType) {
 /// and add more keys to the returned map.
 abstract class CommandWithTarget extends Command {
   /// Constructs this command given a [finder].
-  CommandWithTarget(this.finder) {
+  CommandWithTarget(this.finder, {Duration timeout}) : super(timeout: timeout) {
     if (finder == null)
       throw new DriverError('${this.runtimeType} target cannot be null');
   }
 
+  /// Deserializes the command from JSON generated by [serialize].
+  CommandWithTarget.deserialize(Map<String, String> json)
+      : finder = SerializableFinder.deserialize(json),
+        super.deserialize(json);
+
   /// Locates the object or objects targeted by this command.
   final SerializableFinder finder;
 
@@ -37,7 +42,8 @@ abstract class CommandWithTarget extends Command {
   ///       'foo': this.foo,
   ///     });
   @override
-  Map<String, String> serialize() => finder.serialize();
+  Map<String, String> serialize() =>
+      super.serialize()..addAll(finder.serialize());
 }
 
 /// Waits until [finder] can locate the target.
@@ -49,33 +55,11 @@ class WaitFor extends CommandWithTarget {
   /// appear within the [timeout] amount of time.
   ///
   /// If [timeout] is not specified the command times out after 5 seconds.
-  WaitFor(SerializableFinder finder, {this.timeout})
-      : super(finder);
-
-  /// The maximum amount of time to wait for the [finder] to locate the desired
-  /// widgets before timing out the command.
-  ///
-  /// Defaults to 5 seconds.
-  final Duration timeout;
+  WaitFor(SerializableFinder finder, {Duration timeout})
+      : super(finder, timeout: timeout);
 
   /// Deserializes the command from JSON generated by [serialize].
-  static WaitFor deserialize(Map<String, String> json) {
-    Duration timeout = json['timeout'] != null
-      ? new Duration(milliseconds: int.parse(json['timeout']))
-      : null;
-    return new WaitFor(SerializableFinder.deserialize(json), timeout: timeout);
-  }
-
-  @override
-  Map<String, String> serialize() {
-    Map<String, String> json = super.serialize();
-
-    if (timeout != null) {
-      json['timeout'] = '${timeout.inMilliseconds}';
-    }
-
-    return json;
-  }
+  WaitFor.deserialize(Map<String, String> json) : super.deserialize(json);
 }
 
 /// The result of a [WaitFor] command.
@@ -207,16 +191,14 @@ class ByValueKey extends SerializableFinder {
 
 /// Command to read the text from a given element.
 class GetText extends CommandWithTarget {
-  /// [finder] looks for an element that contains a piece of text.
-  GetText(SerializableFinder finder) : super(finder);
-
   @override
   final String kind = 'get_text';
 
+  /// [finder] looks for an element that contains a piece of text.
+  GetText(SerializableFinder finder) : super(finder);
+
   /// Deserializes the command from JSON generated by [serialize].
-  static GetText deserialize(Map<String, String> json) {
-    return new GetText(SerializableFinder.deserialize(json));
-  }
+  GetText.deserialize(Map<String, dynamic> json) : super.deserialize(json);
 
   @override
   Map<String, String> serialize() => super.serialize();

packages/flutter_driver/lib/src/gesture.dart

@@ -14,9 +14,7 @@ class Tap extends CommandWithTarget {
   Tap(SerializableFinder finder) : super(finder);
 
   /// Deserializes this command from JSON generated by [serialize].
-  static Tap deserialize(Map<String, String> json) {
-    return new Tap(SerializableFinder.deserialize(json));
-  }
+  Tap.deserialize(Map<String, String> json) : super.deserialize(json);
 
   @override
   Map<String, String> serialize() => super.serialize();
@@ -50,15 +48,12 @@ class Scroll extends CommandWithTarget {
   ) : super(finder);
 
   /// Deserializes this command from JSON generated by [serialize].
-  static Scroll deserialize(Map<String, dynamic> json) {
-    return new Scroll(
-      SerializableFinder.deserialize(json),
-      double.parse(json['dx']),
-      double.parse(json['dy']),
-      new Duration(microseconds: int.parse(json['duration'])),
-      int.parse(json['frequency'])
-    );
-  }
+  Scroll.deserialize(Map<String, dynamic> json)
+      : this.dx = double.parse(json['dx']),
+        this.dy = double.parse(json['dy']),
+        this.duration = new Duration(microseconds: int.parse(json['duration'])),
+        this.frequency = int.parse(json['frequency']),
+        super.deserialize(json);
 
   /// Delta X offset per move event.
   final double dx;
@@ -103,9 +98,8 @@ class ScrollIntoView extends CommandWithTarget {
   ScrollIntoView(SerializableFinder finder) : super(finder);
 
   /// Deserializes this command from JSON generated by [serialize].
-  static ScrollIntoView deserialize(Map<String, dynamic> json) {
-    return new ScrollIntoView(SerializableFinder.deserialize(json));
-  }
+  ScrollIntoView.deserialize(Map<String, dynamic> json)
+      : super.deserialize(json);
 
   // This is here just to be clear that this command isn't adding any extra
   // fields.

packages/flutter_driver/lib/src/health.dart

@@ -6,15 +6,14 @@ import 'enum_util.dart';
 import 'message.dart';
 
 /// Requests an application health check.
-class GetHealth implements Command {
+class GetHealth extends Command {
   @override
   final String kind = 'get_health';
 
-  /// Deserializes the command from JSON generated by [serialize].
-  static GetHealth deserialize(Map<String, String> json) => new GetHealth();
+  GetHealth({Duration timeout}) : super(timeout: timeout);
 
-  @override
-  Map<String, String> serialize() => const <String, String>{};
+  /// Deserializes the command from JSON generated by [serialize].
+  GetHealth.deserialize(Map<String, String> json) : super.deserialize(json);
 }
 
 /// Application health status.

packages/flutter_driver/lib/src/input.dart

@@ -20,10 +20,9 @@ class SetInputText extends CommandWithTarget {
   final String text;
 
   /// Deserializes this command from JSON generated by [serialize].
-  static SetInputText deserialize(Map<String, dynamic> json) {
-    String text = json['text'];
-    return new SetInputText(SerializableFinder.deserialize(json), text);
-  }
+  SetInputText.deserialize(Map<String, dynamic> json)
+      : this.text = json['text'],
+        super.deserialize(json);
 
   @override
   Map<String, String> serialize() {
@@ -56,9 +55,8 @@ class SubmitInputText extends CommandWithTarget {
   SubmitInputText(SerializableFinder finder) : super(finder);
 
   /// Deserializes this command from JSON generated by [serialize].
-  static SubmitInputText deserialize(Map<String, dynamic> json) {
-    return new SubmitInputText(SerializableFinder.deserialize(json));
-  }
+  SubmitInputText.deserialize(Map<String, dynamic> json)
+      : super.deserialize(json);
 }
 
 /// The result of a [SubmitInputText] command.

packages/flutter_driver/lib/src/message.dart

@@ -5,11 +5,25 @@
 /// An object sent from the Flutter Driver to a Flutter application to instruct
 /// the application to perform a task.
 abstract class Command {
+  Command({Duration timeout})
+      : this.timeout = timeout ?? const Duration(seconds: 5);
+
+  Command.deserialize(Map<String, String> json)
+      : timeout = new Duration(milliseconds: int.parse(json['timeout']));
+
+  /// The maximum amount of time to wait for the command to complete.
+  ///
+  /// Defaults to 5 seconds.
+  final Duration timeout;
+
   /// Identifies the type of the command object and of the handler.
   String get kind;
 
   /// Serializes this command to parameter name/value pairs.
-  Map<String, String> serialize();
+  Map<String, String> serialize() => <String, String>{
+    'command': kind,
+    'timeout': '${timeout.inMilliseconds}',
+  };
 }
 
 /// An object sent from a Flutter application back to the Flutter Driver in

packages/flutter_driver/lib/src/render_tree.dart

@@ -5,15 +5,14 @@
 import 'message.dart';
 
 /// A request for a string representation of the render tree.
-class GetRenderTree implements Command {
+class GetRenderTree extends Command {
   @override
   final String kind = 'get_render_tree';
 
-  /// Deserializes the command from JSON generated by [serialize].
-  static GetRenderTree deserialize(Map<String, String> json) => new GetRenderTree();
+  GetRenderTree({Duration timeout}) : super(timeout: timeout);
 
-  @override
-  Map<String, String> serialize() => const <String, String>{};
+  /// Deserializes the command from JSON generated by [serialize].
+  GetRenderTree.deserialize(Map<String, String> json) : super.deserialize(json);
 }
 
 /// A string representation of the render tree.
@@ -34,4 +33,3 @@ class RenderTree extends Result {
     'tree': tree
   };
 }
-

packages/flutter_driver/test/flutter_driver_test.dart

@@ -35,7 +35,7 @@ void main() {
       when(mockVM.isolates).thenReturn(<VMRunnableIsolate>[mockIsolate]);
       when(mockIsolate.loadRunnable()).thenReturn(mockIsolate);
       when(mockIsolate.invokeExtension(any, any))
-          .thenReturn(new Future<Map<String, dynamic>>.value(<String, String>{'status': 'ok'}));
+          .thenReturn(makeMockResponse(<String, dynamic>{'status': 'ok'}));
       vmServiceConnectFunction = (String url) {
         return new Future<VMServiceClientConnection>.value(
           new VMServiceClientConnection(mockClient, null)
@@ -106,9 +106,8 @@ void main() {
     });
 
     test('checks the health of the driver extension', () async {
-      when(mockIsolate.invokeExtension(any, any)).thenReturn(new Future<Map<String, dynamic>>.value(<String, dynamic>{
-        'status': 'ok',
-      }));
+      when(mockIsolate.invokeExtension(any, any)).thenReturn(
+          makeMockResponse(<String, dynamic>{'status': 'ok'}));
       Health result = await driver.checkHealth();
       expect(result.status, HealthStatus.ok);
     });
@@ -128,11 +127,12 @@ void main() {
         when(mockIsolate.invokeExtension(any, any)).thenAnswer((Invocation i) {
           expect(i.positionalArguments[1], <String, String>{
             'command': 'tap',
+            'timeout': '5000',
             'finderType': 'ByValueKey',
             'keyValueString': 'foo',
             'keyValueType': 'String'
           });
-          return new Future<Null>.value();
+          return makeMockResponse(<String, dynamic>{});
         });
         await driver.tap(find.byValueKey('foo'));
       });
@@ -147,10 +147,11 @@ void main() {
         when(mockIsolate.invokeExtension(any, any)).thenAnswer((Invocation i) {
           expect(i.positionalArguments[1], <String, dynamic>{
             'command': 'tap',
+            'timeout': '5000',
             'finderType': 'ByText',
             'text': 'foo',
           });
-          return new Future<Map<String, dynamic>>.value();
+          return makeMockResponse(<String, dynamic>{});
         });
         await driver.tap(find.text('foo'));
       });
@@ -165,11 +166,12 @@ void main() {
         when(mockIsolate.invokeExtension(any, any)).thenAnswer((Invocation i) {
           expect(i.positionalArguments[1], <String, dynamic>{
             'command': 'get_text',
+            'timeout': '5000',
             'finderType': 'ByValueKey',
             'keyValueString': '123',
             'keyValueType': 'int'
           });
-          return new Future<Map<String, dynamic>>.value(<String, String>{
+          return makeMockResponse(<String, String>{
             'text': 'hello'
           });
         });
@@ -191,7 +193,7 @@ void main() {
             'text': 'foo',
             'timeout': '1000',
           });
-          return new Future<Map<String, dynamic>>.value(<String, dynamic>{});
+          return makeMockResponse(<String, dynamic>{});
         });
         await driver.waitFor(find.byTooltip('foo'), timeout: new Duration(seconds: 1));
       });
@@ -279,6 +281,45 @@ void main() {
         expect(timeline.events.single.name, 'test event');
       });
     });
+
+    group('sendCommand error conditions', () {
+      test('local timeout', () async {
+        when(mockIsolate.invokeExtension(any, any)).thenAnswer((Invocation i) {
+          // completer never competed to trigger timeout
+          return new Completer<Map<String, dynamic>>().future;
+        });
+        try {
+          await driver.waitFor(find.byTooltip('foo'), timeout: new Duration(milliseconds: 100));
+          fail('expected an exception');
+        } catch(error) {
+          expect(error is DriverError, isTrue);
+          expect(error.message, 'Failed to fulfill WaitFor: Flutter application not responding');
+        }
+      });
+
+      test('remote error', () async {
+        when(mockIsolate.invokeExtension(any, any)).thenAnswer((Invocation i) {
+          return makeMockResponse(<String, dynamic>{
+            'message': 'This is a failure'
+          }, isError: true);
+        });
+        try {
+          await driver.waitFor(find.byTooltip('foo'));
+          fail('expected an exception');
+        } catch(error) {
+          expect(error is DriverError, isTrue);
+          expect(error.message, 'Error in Flutter application: {message: This is a failure}');
+        }
+      });
+    });
+  });
+}
+
+Future<Map<String, dynamic>> makeMockResponse(
+    Map<String, dynamic> response, {bool isError: false}) {
+  return new Future<Map<String, dynamic>>.value(<String, dynamic>{
+    'isError': isError,
+    'response': response
   });
 }