From 2c7e5dd93548d84dad61bf2de171cdb89fa3efc6 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Mon, 6 Jul 2020 10:07:36 -0700 Subject: [PATCH] Revert "Fix remaining holes in stack trace demangling (#60478)" (#60916) This reverts commit d986fdc31afb4fc3236d8333e11f58ba2b078fea. --- .../lib/src/foundation/assertions.dart | 58 ++------------- .../lib/src/foundation/stack_frame.dart | 7 -- .../flutter/lib/src/scheduler/binding.dart | 4 +- .../test/foundation/stack_frame_test.dart | 25 ------- packages/flutter_test/lib/src/binding.dart | 31 ++++---- .../test/bindings_async_gap_test.dart | 2 +- packages/flutter_test/test/demangle_test.dart | 73 ------------------- 7 files changed, 24 insertions(+), 176 deletions(-) delete mode 100644 packages/flutter_test/test/demangle_test.dart diff --git a/packages/flutter/lib/src/foundation/assertions.dart b/packages/flutter/lib/src/foundation/assertions.dart index e296f3b5ea356..393a69b05b33b 100644 --- a/packages/flutter/lib/src/foundation/assertions.dart +++ b/packages/flutter/lib/src/foundation/assertions.dart @@ -28,14 +28,6 @@ typedef DiagnosticPropertiesTransformer = Iterable Function(Ite /// and other callbacks that collect information describing an error. typedef InformationCollector = Iterable Function(); -/// Signature for a function that demangles [StackTrace] objects into a format -/// that can be parsed by [StackFrame]. -/// -/// See also: -/// -/// * [FlutterError.demangleStackTrace], which shows an example implementation. -typedef StackTraceDemangler = StackTrace Function(StackTrace details); - /// Partial information from a stack frame for stack filtering purposes. /// /// See also: @@ -659,7 +651,7 @@ class FlutterErrorDetails with Diagnosticable { // If not: Error is in user code (user violated assertion in framework). // If so: Error is in Framework. We either need an assertion higher up // in the stack, or we've violated our own assertions. - final List stackFrames = StackFrame.fromStackTrace(FlutterError.demangleStackTrace(stack)) + final List stackFrames = StackFrame.fromStackTrace(stack) .skipWhile((StackFrame frame) => frame.packageScheme == 'dart') .toList(); final bool ourFault = stackFrames.length >= 2 @@ -871,31 +863,6 @@ class FlutterError extends Error with DiagnosticableTreeMixin implements Asserti /// recommended. static FlutterExceptionHandler onError = (FlutterErrorDetails details) => presentError(details); - /// Called by the Flutter framework before attempting to parse a [StackTrace]. - /// - /// Some [StackTrace] implementations have a different toString format from - /// what the framework expects, like ones from package:stack_trace. To make - /// sure we can still parse and filter mangled [StackTrace]s, the framework - /// first calls this function to demangle them. - /// - /// This should be set in any environment that could propagate a non-standard - /// stack trace to the framework. Otherwise, the default behavior is to assume - /// all stack traces are in a standard format. - /// - /// The following example demangles package:stack_trace traces by converting - /// them into vm traces, which the framework is able to parse: - /// - /// ```dart - /// FlutterError.demangleStackTrace = (StackTrace stackTrace) { - /// if (stack is stack_trace.Trace) - // return stack.vmTrace; - // if (stack is stack_trace.Chain) - // return stack.toTrace().vmTrace; - // return stack; - /// }; - /// ``` - static StackTraceDemangler demangleStackTrace = (StackTrace stackTrace) => stackTrace; - /// Called whenever the Flutter framework wants to present an error to the /// users. /// @@ -1102,11 +1069,7 @@ class FlutterError extends Error with DiagnosticableTreeMixin implements Asserti void debugPrintStack({StackTrace stackTrace, String label, int maxFrames}) { if (label != null) debugPrint(label); - if (stackTrace == null) { - stackTrace = StackTrace.current; - } else { - stackTrace = FlutterError.demangleStackTrace(stackTrace); - } + stackTrace ??= StackTrace.current; Iterable lines = stackTrace.toString().trimRight().split('\n'); if (kIsWeb && lines.isNotEmpty) { // Remove extra call to StackTrace.current for web platform. @@ -1142,7 +1105,11 @@ class DiagnosticsStackTrace extends DiagnosticsBlock { }) : super( name: name, value: stack, - properties: _applyStackFilter(stack, stackFilter), + properties: stack == null + ? [] + : (stackFilter ?? FlutterError.defaultStackFilter)(stack.toString().trimRight().split('\n')) + .map(_createStackFrame) + .toList(), style: DiagnosticsTreeStyle.flat, showSeparator: showSeparator, allowTruncate: true, @@ -1160,17 +1127,6 @@ class DiagnosticsStackTrace extends DiagnosticsBlock { showSeparator: showSeparator, ); - static List _applyStackFilter( - StackTrace stack, - IterableFilter stackFilter, - ) { - if (stack == null) - return []; - final IterableFilter filter = stackFilter ?? FlutterError.defaultStackFilter; - final Iterable frames = filter('${FlutterError.demangleStackTrace(stack)}'.trimRight().split('\n')); - return frames.map(_createStackFrame).toList(); - } - static DiagnosticsNode _createStackFrame(String frame) { return DiagnosticsNode.message(frame, allowWrap: false); } diff --git a/packages/flutter/lib/src/foundation/stack_frame.dart b/packages/flutter/lib/src/foundation/stack_frame.dart index 0521c5b069238..ee039e083f06f 100644 --- a/packages/flutter/lib/src/foundation/stack_frame.dart +++ b/packages/flutter/lib/src/foundation/stack_frame.dart @@ -190,13 +190,6 @@ class StackFrame { return stackOverFlowElision; } - assert( - line != '===== asynchronous gap ===========================', - 'Got a stack frame from package:stack_trace, where a vm or web frame was expected. ' - 'This can happen if FlutterError.demangleStackTrace was not set in an environment ' - 'that propagates non-standard stack traces to the framework, such as during tests.' - ); - // Web frames. if (!line.startsWith('#')) { return _parseWebFrame(line); diff --git a/packages/flutter/lib/src/scheduler/binding.dart b/packages/flutter/lib/src/scheduler/binding.dart index cbcbbdc1ddb1a..9477eb68f9e18 100644 --- a/packages/flutter/lib/src/scheduler/binding.dart +++ b/packages/flutter/lib/src/scheduler/binding.dart @@ -621,9 +621,7 @@ mixin SchedulerBinding on BindingBase { debugPrint('When the current transient callback was registered, this was the stack:'); debugPrint( FlutterError.defaultStackFilter( - FlutterError.demangleStackTrace( - _FrameCallbackEntry.debugCurrentCallbackStack, - ).toString().trimRight().split('\n') + _FrameCallbackEntry.debugCurrentCallbackStack.toString().trimRight().split('\n') ).join('\n') ); } else { diff --git a/packages/flutter/test/foundation/stack_frame_test.dart b/packages/flutter/test/foundation/stack_frame_test.dart index 6cfb62f12f35a..4f53495431646 100644 --- a/packages/flutter/test/foundation/stack_frame_test.dart +++ b/packages/flutter/test/foundation/stack_frame_test.dart @@ -73,16 +73,6 @@ void main() { }, skip: isBrowser); // The VM test harness can handle a stack overflow, but // the browser cannot - running this test in a browser will cause it to become // unresponsive. - - test('Traces from package:stack_trace throw assertion', () { - try { - StackFrame.fromStackString(mangledStackString); - assert(false, 'StackFrame.fromStackString did not throw on a mangled stack trace'); - } catch (e) { - expect(e, isA()); - expect('$e', contains('Got a stack frame from package:stack_trace')); - } - }); } const String stackString = ''' @@ -162,21 +152,6 @@ const String asyncStackString = ''' #37 _startIsolate. (dart:isolate-patch/isolate_patch.dart:307:19) #38 _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:174:12)'''; -const String mangledStackString = ''' -dart:async/future_impl.dart 23:44 _Completer.completeError -test\\bindings_async_gap_test.dart 42:17 main.. -package:flutter_test/src/binding.dart 744:19 TestWidgetsFlutterBinding._runTestBody -===== asynchronous gap =========================== -dart:async/zone.dart 1121:19 _CustomZone.registerUnaryCallback -dart:async-patch/async_patch.dart 83:23 _asyncThenWrapperHelper -dart:async/zone.dart 1222:13 _rootRunBinary -dart:async/zone.dart 1107:19 _CustomZone.runBinary -package:flutter_test/src/binding.dart 724:14 TestWidgetsFlutterBinding._runTest -package:flutter_test/src/binding.dart 1124:24 AutomatedTestWidgetsFlutterBinding.runTest. -package:fake_async/fake_async.dart 177:54 FakeAsync.run.. -dart:async/zone.dart 1190:13 _rootRun -'''; - const List asyncStackFrames = [ StackFrame(number: 0, className: '', method: 'getSampleStack', packageScheme: 'file', package: '', packagePath: '/path/to/flutter/packages/flutter/test/foundation/error_reporting_test.dart', line: 40, column: 57, source: '#0 getSampleStack. (file:///path/to/flutter/packages/flutter/test/foundation/error_reporting_test.dart:40:57)'), StackFrame(number: 1, className: 'Future', method: 'sync', packageScheme: 'dart', package: 'async', packagePath: 'future.dart', line: 224, column: 31, isConstructor: true, source: '#1 new Future.sync (dart:async/future.dart:224:31)'), diff --git a/packages/flutter_test/lib/src/binding.dart b/packages/flutter_test/lib/src/binding.dart index a808cd24276ca..5f63366b41e4c 100644 --- a/packages/flutter_test/lib/src/binding.dart +++ b/packages/flutter_test/lib/src/binding.dart @@ -516,7 +516,6 @@ abstract class TestWidgetsFlutterBinding extends BindingBase return result; } FlutterExceptionHandler _oldExceptionHandler; - StackTraceDemangler _oldStackTraceDemangler; FlutterErrorDetails _pendingExceptionDetails; static const TextStyle _messageStyle = TextStyle( @@ -580,6 +579,10 @@ abstract class TestWidgetsFlutterBinding extends BindingBase // our main future completing. assert(Zone.current == _parentZone); if (_pendingExceptionDetails != null) { + assert( + _unmangle(_pendingExceptionDetails.stack) == _pendingExceptionDetails.stack, + 'The test binding presented an unmangled stack trace to the framework.', + ); debugPrint = debugPrintOverride; // just in case the test overrides it -- otherwise we won't see the error! reportTestException(_pendingExceptionDetails, testDescription); _pendingExceptionDetails = null; @@ -610,9 +613,9 @@ abstract class TestWidgetsFlutterBinding extends BindingBase assert(description != null); assert(inTest); _oldExceptionHandler = FlutterError.onError; - _oldStackTraceDemangler = FlutterError.demangleStackTrace; int _exceptionCount = 0; // number of un-taken exceptions FlutterError.onError = (FlutterErrorDetails details) { + details = details.copyWith(stack: _unmangle(details.stack)); if (_pendingExceptionDetails != null) { debugPrint = debugPrintOverride; // just in case the test overrides it -- otherwise we won't see the errors! if (_exceptionCount == 0) { @@ -631,17 +634,6 @@ abstract class TestWidgetsFlutterBinding extends BindingBase _pendingExceptionDetails = details; } }; - FlutterError.demangleStackTrace = (StackTrace stack) { - // package:stack_trace uses ZoneSpecification.errorCallback to add useful - // information to stack traces, in this case the Trace and Chain classes - // can be present. Because these StackTrace implementations do not follow - // the format the framework expects, we covert them to a vm trace here. - if (stack is stack_trace.Trace) - return stack.vmTrace; - if (stack is stack_trace.Chain) - return stack.toTrace().vmTrace; - return stack; - }; final Completer testCompleter = Completer(); final VoidCallback testCompletionHandler = _createTestCompletionHandler(description, testCompleter); void handleUncaughtError(dynamic exception, StackTrace stack) { @@ -655,7 +647,7 @@ abstract class TestWidgetsFlutterBinding extends BindingBase debugPrint = debugPrintOverride; // just in case the test overrides it -- otherwise we won't see the error! FlutterError.dumpErrorToConsole(FlutterErrorDetails( exception: exception, - stack: stack, + stack: _unmangle(stack), context: ErrorDescription('running a test (but after the test had completed)'), library: 'Flutter test framework', ), forceReport: true); @@ -702,7 +694,7 @@ abstract class TestWidgetsFlutterBinding extends BindingBase final int stackLinesToOmit = reportExpectCall(stack, omittedFrames); FlutterError.reportError(FlutterErrorDetails( exception: exception, - stack: stack, + stack: _unmangle(stack), context: ErrorDescription('running a test'), library: 'Flutter test framework', stackFilter: (Iterable frames) { @@ -850,7 +842,6 @@ abstract class TestWidgetsFlutterBinding extends BindingBase void postTest() { assert(inTest); FlutterError.onError = _oldExceptionHandler; - FlutterError.demangleStackTrace = _oldStackTraceDemangler; _pendingExceptionDetails = null; _parentZone = null; buildOwner.focusManager = FocusManager(); @@ -1722,3 +1713,11 @@ class _LiveTestRenderView extends RenderView { _label?.paint(context.canvas, offset - const Offset(0.0, 10.0)); } } + +StackTrace _unmangle(StackTrace stack) { + if (stack is stack_trace.Trace) + return stack.vmTrace; + if (stack is stack_trace.Chain) + return stack.toTrace().vmTrace; + return stack; +} diff --git a/packages/flutter_test/test/bindings_async_gap_test.dart b/packages/flutter_test/test/bindings_async_gap_test.dart index aced2bfec21f0..27b7cb1c59ddc 100644 --- a/packages/flutter_test/test/bindings_async_gap_test.dart +++ b/packages/flutter_test/test/bindings_async_gap_test.dart @@ -49,4 +49,4 @@ Future main() async { class CustomException implements Exception { const CustomException(); -} \ No newline at end of file +} diff --git a/packages/flutter_test/test/demangle_test.dart b/packages/flutter_test/test/demangle_test.dart deleted file mode 100644 index 85a9d0cc3dc41..0000000000000 --- a/packages/flutter_test/test/demangle_test.dart +++ /dev/null @@ -1,73 +0,0 @@ -// Copyright 2014 The Flutter Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -import 'dart:async'; - -import 'package:flutter/foundation.dart'; -import 'package:flutter_test/flutter_test.dart'; -import 'package:stack_trace/stack_trace.dart' as stack_trace; - -Future main() async { - // We use AutomatedTestWidgetsFlutterBinding to allow the test binding to set - // FlutterError.demangleStackTrace and FlutterError.onError without testWidgets. - final AutomatedTestWidgetsFlutterBinding binding = AutomatedTestWidgetsFlutterBinding(); - - test('FlutterErrorDetails demangles', () async { - await binding.runTest(() async { - // When we call toString on a FlutterErrorDetails, it attempts to parse and - // filter the stack trace, which fails if demangleStackTrace returns a - // mangled stack trace. - FlutterErrorDetails( - exception: const CustomException(), - stack: await getMangledStack(), - ).toString(); - - // Additional logic is used to parse assertion stack traces. - FlutterErrorDetails( - exception: AssertionError('Some assertion'), - stack: await getMangledStack(), - ).toString(); - }, () {}); - binding.postTest(); - }); - - test('debugPrintStack demangles', () async { - await binding.runTest(() async { - final DebugPrintCallback oldDebugPrint = debugPrint; - try { - debugPrint = (String message, {int wrapWidth}) {}; - debugPrintStack( - stackTrace: await getMangledStack(), - ); - } finally { - debugPrint = oldDebugPrint; - } - }, () {}); - binding.postTest(); - }); -} - -Future getMangledStack() { - // package:test uses package:stack_trace to wrap tests in a Zone that overrides - // errorCallback, the error callback transforms any StackTrace propagated - // to futures into a Chain, which has a format different from the vm. - final Completer stackCompleter = Completer(); - final Completer completer = Completer(); - completer.future.then( - (void value) { - assert(false); - }, - onError: (Object error, StackTrace stack) { - expect(error, isA()); - expect(stack, isA()); - stackCompleter.complete(stack); - }, - ); - completer.completeError(const CustomException()); - return stackCompleter.future; -} - -class CustomException implements Exception { - const CustomException(); -}