Improve error reporting (#4025)

Don't suggest filing an issue when we can definitively say that the
assertion in question was not thrown from within the flutter package.
Fixes https://github.com/flutter/flutter/issues/3812.

Put the stack trace first after the message, with more details below the
stack trace, since the stack is often very useful and the stacks are no
longer stupidly long.

Better document the 'rescheduling' feature, and improve the error
handling asserts when it is misused.
Fixes https://github.com/flutter/flutter/issues/3888.

Improve the wording around the stack dump for exceptions during
callbacks.

Improve the color and font size of messages in live tests.
Fixes https://github.com/flutter/flutter/issues/4018.

packages/flutter/lib/src/foundation/assertions.dart

@@ -198,31 +198,48 @@ class FlutterError extends AssertionError {
         debugPrint('The following $errorName was $verb:', wrapWidth: _kWrapWidth);
         debugPrint('${details.exception}', wrapWidth: _kWrapWidth);
       }
+      Iterable<String> stackLines = (details.stack != null) ? details.stack.toString().trimRight().split('\n') : null;
       if ((details.exception is AssertionError) && (details.exception is! FlutterError)) {
-        debugPrint('Either the assertion indicates an error in the framework itself, or we should '
-                   'provide substantially more information in this error message to help you determine '
-                   'and fix the underlying cause.', wrapWidth: _kWrapWidth);
-        debugPrint('In either case, please report this assertion by filing a bug on GitHub:', wrapWidth: _kWrapWidth);
-        debugPrint('  https://github.com/flutter/flutter/issues/new');
-      }
-      if (details.informationCollector != null) {
-        StringBuffer information = new StringBuffer();
-        details.informationCollector(information);
-        debugPrint(information.toString(), wrapWidth: _kWrapWidth);
+        bool ourFault = true;
+        if (stackLines != null) {
+          List<String> stackList = stackLines.take(2).toList();
+          if (stackList.length >= 2) {
+            final RegExp throwPattern = new RegExp(r'^#0 +_AssertionError._throwNew \(dart:.+\)$');
+            final RegExp assertPattern = new RegExp(r'^#1 +[^(]+ \((.+?):([0-9]+)(?::[0-9]+)?\)$');
+            if (throwPattern.hasMatch(stackList[0])) {
+              final Match assertMatch = assertPattern.firstMatch(stackList[1]);
+              if (assertMatch != null) {
+                assert(assertMatch.groupCount == 2);
+                final RegExp ourLibraryPattern = new RegExp(r'^package:flutter/');
+                ourFault = ourLibraryPattern.hasMatch(assertMatch.group(1));
+              }
+            }
+          }
+        }
+        if (ourFault) {
+          debugPrint('\nEither the assertion indicates an error in the framework itself, or we should '
+                     'provide substantially more information in this error message to help you determine '
+                     'and fix the underlying cause.', wrapWidth: _kWrapWidth);
+          debugPrint('In either case, please report this assertion by filing a bug on GitHub:', wrapWidth: _kWrapWidth);
+          debugPrint('  https://github.com/flutter/flutter/issues/new');
+        }
       }
       if (details.stack != null) {
-        debugPrint('When the exception was thrown, this was the stack:', wrapWidth: _kWrapWidth);
-        Iterable<String> stackLines = details.stack.toString().trimRight().split('\n');
+        debugPrint('\nWhen the exception was thrown, this was the stack:', wrapWidth: _kWrapWidth);
         if (details.stackFilter != null) {
           stackLines = details.stackFilter(stackLines);
         } else {
           stackLines = defaultStackFilter(stackLines);
         }
-        debugPrint(stackLines.join('\n'), wrapWidth: _kWrapWidth);
-        debugPrint(footer);
-      } else {
-        debugPrint(footer);
+        for (String line in stackLines)
+          debugPrint(line, wrapWidth: _kWrapWidth);
+      }
+      if (details.informationCollector != null) {
+        StringBuffer information = new StringBuffer();
+        details.informationCollector(information);
+        debugPrint('\n$information', wrapWidth: _kWrapWidth);
       }
+      debugPrint(footer);
     } else {
       debugPrint('Another exception was thrown: ${details.exception.toString().split("\n")[0]}');
     }

packages/flutter/lib/src/scheduler/binding.dart

@@ -46,7 +46,20 @@ class _FrameCallbackEntry {
   _FrameCallbackEntry(this.callback, { bool rescheduling: false }) {
     assert(() {
       if (rescheduling) {
-        assert(currentCallbackStack != null);
+        assert(() {
+          if (currentCallbackStack == null) {
+            throw new FlutterError(
+              'addFrameCallback or scheduleFrameCallback called with rescheduling true, but no callback is in scope.\n'
+              'The "rescheduling" argument should only be set to true if the '
+              'callback is being reregistered from within the callback itself, '
+              'and only then if the callback itself is entirely synchronous. '
+              'If this is the initial registration of the callback, or if the '
+              'callback is asynchronous, then do not use the "rescheduling" '
+              'argument.'
+            );
+          }
+          return true;
+        });
         stack = currentCallbackStack;
       } else {
         stack = StackTrace.current;
@@ -176,10 +189,17 @@ abstract class SchedulerBinding extends BindingBase {
   /// Adds the given callback to the list of frame callbacks and ensures that a
   /// frame is scheduled.
   ///
-  /// If `rescheduling` is true, the call must be in the context of a
-  /// frame callback, and for debugging purposes the stack trace
-  /// stored for this callback will be the same stack trace as for the
-  /// current callback.
+  /// If this is a one-off registration, ignore the `rescheduling` argument.
+  ///
+  /// If this is a callback that will be reregistered each time it fires, then
+  /// when you reregister the callback, set the `rescheduling` argument to true.
+  /// This has no effect in release builds, but in debug builds, it ensures that
+  /// the stack trace that is stored for this callback is the original stack
+  /// trace for when the callback was _first_ registered, rather than the stack
+  /// trace for when the callback is reregistered. This makes it easier to track
+  /// down the original reason that a particular callback was invoked. If
+  /// `rescheduling` is true, the call must be in the context of a frame
+  /// callback.
   ///
   /// Callbacks registered with this method can be canceled using
   /// [cancelFrameCallbackWithId].
@@ -200,10 +220,17 @@ abstract class SchedulerBinding extends BindingBase {
   /// a frame is requested. To register a callback and ensure that a
   /// frame is immediately scheduled, use [scheduleFrameCallback].
   ///
-  /// If `rescheduling` is true, the call must be in the context of a
-  /// frame callback, and for debugging purposes the stack trace
-  /// stored for this callback will be the same stack trace as for the
-  /// current callback.
+  /// If this is a one-off registration, ignore the `rescheduling` argument.
+  ///
+  /// If this is a callback that will be reregistered each time it fires, then
+  /// when you reregister the callback, set the `rescheduling` argument to true.
+  /// This has no effect in release builds, but in debug builds, it ensures that
+  /// the stack trace that is stored for this callback is the original stack
+  /// trace for when the callback was _first_ registered, rather than the stack
+  /// trace for when the callback is reregistered. This makes it easier to track
+  /// down the original reason that a particular callback was invoked. If
+  /// `rescheduling` is true, the call must be in the context of a frame
+  /// callback.
   ///
   /// Callbacks registered with this method can be canceled using
   /// [cancelFrameCallbackWithId].
@@ -390,6 +417,7 @@ abstract class SchedulerBinding extends BindingBase {
   void _invokeFrameCallback(FrameCallback callback, Duration timeStamp, [ StackTrace callbackStack ]) {
     assert(callback != null);
     assert(_FrameCallbackEntry.currentCallbackStack == null);
+    // TODO(ianh): Consider using a Zone instead to track the current callback registration stack
     assert(() { _FrameCallbackEntry.currentCallbackStack = callbackStack; return true; });
     try {
       callback(timeStamp);
@@ -400,8 +428,12 @@ abstract class SchedulerBinding extends BindingBase {
         library: 'scheduler library',
         context: 'during a scheduler callback',
         informationCollector: (callbackStack == null) ? null : (StringBuffer information) {
-          // callbackStack ends with a newline, so don't introduce one artificially here
-          information.write('When this callback was registered, this was the stack:\n$callbackStack');
+          information.writeln(
+            '\nThis exception was thrown in the context of a scheduler callback. '
+            'When the scheduler callback was _registered_ (as opposed to when the '
+            'exception was thrown), this was the stack:'
+          );
+          FlutterError.defaultStackFilter(callbackStack.toString().trimRight().split('\n')).forEach(information.writeln);
         }
       ));
     }

packages/flutter_test/lib/src/binding.dart

@@ -179,17 +179,22 @@ abstract class TestWidgetsFlutterBinding extends BindingBase
   FlutterExceptionHandler _oldExceptionHandler;
   FlutterErrorDetails _pendingExceptionDetails;
 
+  static const TextStyle _kMessageStyle = const TextStyle(
+    color: const Color(0xFF917FFF),
+    fontSize: 40.0
+  );
+
   static final Widget _kPreTestMessage = new Center(
     child: new Text(
       'Test starting...',
-      style: const TextStyle(color: const Color(0xFFFF0000))
+      style: _kMessageStyle
     )
   );
 
   static final Widget _kPostTestMessage = new Center(
     child: new Text(
       'Test finished.',
-      style: const TextStyle(color: const Color(0xFFFF0000))
+      style: _kMessageStyle
     )
   );