(original commit was missing the newly added test file)

Instead of dealing with the mangled stack traces in onError, I think it would be more beneficial to avoid non-vm stack traces being propagated in the first place because this may still crop up in other places.

In my minimal repo, the problematic trace comes from the ZoneSpecification errorCallback defined here: https://github.com/dart-lang/stack_trace/blob/4d1ef4eddcd9b18c23bf19fd9d37245d3c81fa2e/lib/src/stack_zone_specification.dart#L172

What we could do is define an errorCallback in our own flutter_test zone which demangles it, and then just replace the FlutterErrorDetails copy in onError with an assertion.

Alternatively, we could modify package:stack_trace to make sure implementers of StackTrace only produce traces consistent with the current platform, requiring anyone that depended on mangled traces to call a separate method.

I think the probem with setting up another zone for this is that the test, as written, is directly calling FlutterError.reportError with a mangled stack trace. It's not going through the zone error handling mechanism - it's created and caught in the test itself. This is a constructed case, but it matches what I've seen in some nested network exceptions elsewhere.

It's not clear to me where or why the package:stack_trace types are getting introduced. Perhaps from package:test?

I also wouldn't bank on getting package:stack_trace changed here - part of the point of that package is to make traces look more human friendly, as defined by that package.

This is my first review, so please tell me if I'm doing anything wrong 😰

Yes, package:test is the one applying the zone to flutter tests: https://github.com/dart-lang/test/blob/658e47e99c170605f7ab7502c556aae295ed9f46/pkgs/test_api/lib/src/backend/invoker.dart#L386

I think the approach of demangling in onError will work to address any current bugs. The reason for my concerns about them being propagated to error handlers is that a few different places in flutter still make the assumption that a stack trace isn't mangled (and we could make it again in the future).

After some digging it looks like the only uses of StackFrame parsing outside of FlutterError.onError are in some tests that call toDiagnosticsNode or dumpErrorToConsole directly:

These will throw if a mangled stack is ever passed to them in the future.

All three of us (!) originally thought this was coming from the vm, so at the very least StackFrame.fromStackTraceLine should throw a better error message when given a mangled stack.

I found a bunch of other places this could fail, basically any handler of FlutterError.onError that calls toDiagnosticsNode on the error details, directly or indirectly.

TestRenderingFlutterBinding:

Tap tests:

A scaffold test:

Row tests: https://github.com/flutter/flutter/blob/4d7525f05c05a6df0b29396bc9eb78c3bf1e9f89/packages/flutter/test/widgets/row_test.dart

An object test:

A lot of those other areas are more specialized and not exposed for use in end-users tests. I think we should fix them because it could be confusing for framework developers, but probably do so in another patch - unless you think there's a generalized solution for them.

One thing I don't think we can do is have this check exist in the stack filter, since we don't want to add a dependency on package:stack_trace to the framework.

Yeah, I couldn't come up with a good way for these to be filtered in the framework either (w/o the added dependency), maybe hix or goderbaur knows more.

We now call _unmangle in three places; does this make any of the preexisting ones redundant?

AFAICT, we could reduce the calls to _unmangle in handleUncaughtError, but that's a different path than FlutterError.reportError, at least potentially.

Basically, we have to deal with the case where an uncaught exception (sync or async) is thrown in the test, and we have to deal with the case where FlutterError.reportError is called directly. Either one can happen in a test, and they go through two different paths here.

To explain - we could reassing the stack variable using _unmangle instead of calling _unmangle on it in two places. But we'd still have two distinct places in real code (plus one in the assert I added).

I've no problem it being in three places so long as we're confident that none of those places are entirely redundant (i.e. are entirely subsumed by one of the others). Test frameworks are inherently pretty crazy because they have to deal with unpredictable failures almost by definition.

Google testing passed!