Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Break on "unhandled" exceptions when a debugger is attached #17007

Closed
DanTup opened this issue Apr 26, 2018 · 25 comments · Fixed by #78649
Closed

Break on "unhandled" exceptions when a debugger is attached #17007

DanTup opened this issue Apr 26, 2018 · 25 comments · Fixed by #78649
Assignees
Labels
a: debugging Debugging, breakpoints, expression evaluation P2 Important issues not at the top of the work list tool Affects the "flutter" command-line tool. See also t: labels.

Comments

@DanTup
Copy link
Contributor

DanTup commented Apr 26, 2018

Currently when there are exceptions in Flutter, they get swallowed by the framework and rendered on the device. This might be better than crashing when not debugging but if a debugger is attached it seems wrong to pass up breaking and letting the user inspect the exception and state. Otherwise the user needs to read the stack, but a breakpoint in the appropriate place and then reproduce the issue (which is what break-on-unhandled-exceptions should avoid us having to do).

There was a little discussion in #2357 about this, and there was some suggestion that users could use "break on all exceptions", however I really don't think that's a good solution. Users often have exceptions being caught, or use third party code that is sloppy with it's exceptions. For all intents and purposes, these exceptions are uncaught.

screen shot 2018-04-26 at 5 02 12 pm

@DanTup DanTup added the a: debugging Debugging, breakpoints, expression evaluation label Jun 26, 2018
@zoechi zoechi added the tool Affects the "flutter" command-line tool. See also t: labels. label Dec 10, 2018
@zoechi zoechi added this to the Goals milestone Dec 10, 2018
@zoechi zoechi added the will need additional triage This issue or PR needs attention during weekly triage label Dec 10, 2018
@zoechi zoechi removed the will need additional triage This issue or PR needs attention during weekly triage label Dec 18, 2018
@timsneath
Copy link
Contributor

@DanTup who has the next action on this issue, do you think? I've just been struggling with exactly this.

@DanTup
Copy link
Contributor Author

DanTup commented May 25, 2019

I think this has been discussed a little (w/ @devoncarew @a-siva) but it sounds like there's no easy/obvious solution. The error isn't "unhandled" anymore because Flutter catches it to render the red screen/error. All the solutions I can come up with are kinda weird (like Flutter just not catching unhandled exceptions, or there being some way for Flutter to signal that this catching of an error should still break the debugger prior to handling it).

It does still bug me a lot too (I've been making lots of crashes lately doing maths in my tiler library 🙈). When an error occurs, I'd much prefer the debugger to break and let me examine variables than give me incomplete information in the Debug Console and then I have to repro it with a breakpoint.

@csells
Copy link
Contributor

csells commented Jan 13, 2020

@DanTup I'd love to see this issue fixed, too. I run into it myself. Can you post a small repro that we can use to drive a solution?

@csells csells added the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label Jan 13, 2020
@DanTup
Copy link
Contributor Author

DanTup commented Jan 13, 2020

I'm not sure how complete a repro you're after, but any code that generates some exception that you might want to examine state for works. For example I did flutter create and added some code that "accidentally" throws a null pointer exception (simulating exercising some path that didn't initialise a variable) after clicking the button a few times (highlighted code here is new):

Screenshot 2020-01-13 at 16 11 16

When this exception occurs and I'm running in debug mode, what I would like is for the debugger to pause on that line (as it would in a Dart CLI application or pub run test) so I can examine variables etc.

What actually happens if we just get info written to the Debug Console (as if we ran without debugging):

Screenshot 2020-01-13 at 16 13 12

This tells me that the variable was null, but doesn't help me understand why it is null. When this happens, I currently have to either add a breakpoint to that line or enable break-on-all-exceptions, but both have drawbacks:

  • Breakpoint will trigger lots of times, not only when the error occurs (conditional breakpoint can be used, though it's more awkward - especially since we don't provide completion/validation for the expression)
  • Break-on-all-exceptions tends to break in lots of places you don't want, requiring lots of resuming

In addition, I need to be able to reproduce the issue again - and if I don't know what triggered it the first time I hit it, this might take some time.

I understand this might not be the best way to debug some Flutter issues, but it feels like it's been "taken away" when using Flutter (it works for Dart CLI and pub run test), making it harder to use the IDE debug tools.

@no-response no-response bot removed the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label Jan 13, 2020
@jmagman jmagman added this to Awaiting triage in Tools - DevTools review via automation Jan 15, 2020
@jmagman jmagman added this to Awaiting triage in Tools - IDE support review via automation Mar 9, 2020
@jmagman jmagman removed this from Awaiting triage in Tools - DevTools review Mar 9, 2020
@christopherfujino christopherfujino moved this from Awaiting triage to Engineer reviewed in Tools - IDE support review Mar 9, 2020
@kf6gpe kf6gpe added the P2 Important issues not at the top of the work list label May 29, 2020
@devoncarew
Copy link
Member

cc @mraleph as we discussed this today. From Slava, one solution might be to mark the Flutter framework exception handling method with a special pragma (I-handle-framework-errors). When we encounter an exception, the VM looks up the stack to see if the exception is handled or not. The framework exception hander means that all exceptions from build methods are technically handled, so we'll never break on uncaught exceptions. The difference w/ this new pragma marked method would be that, for the purposes of the break-on-uncaught-exceptions, the VM would notice the pragma method on the stack, and could then decide to break at the place where the exception was thrown. The user would then be able to inspect local variables and state at the place where the exception occured.

also cc'ing @mkustermann, who's been working on improvements to async stack handling (and may have context in this area)

@devoncarew
Copy link
Member

cc'ing @goderbauer - I'm assuming that there is a good framework method that we'd want to mark w/ this pragma

@goderbauer
Copy link
Member

Could you hook into FlutterError.reportError? https://master-api.flutter.dev/flutter/foundation/FlutterError/reportError.html

That method is called from various places in the framework after we've caught an exception.

Should we also break when the user has set up some custom error-handling, e.g. by providing their own FlutterError.onError implementation? If not, we may just want to hook into Flutter's default error handling, i.e. FlutterError.dumpToConsole maybe?

@devoncarew
Copy link
Member

I believe it would need to be something that was on the stack when the exception was thrown - something that might itself call FlutterError.reportError(). From a very quick perusal of the framework code, that might be something like _debugReportException() from widgets/framework.dart?

@devoncarew
Copy link
Member

Actually, it looks like that method is not on the stack either; something like performRebuild() would need to be marked w/ the pragma (would would then call into the normal uncaught exception error handling code in the framework).

@goderbauer
Copy link
Member

If you just mark performRebuild you'd miss many other places where we catch exceptions in the framework and "swallow" them by printing an error to the console.

@Hixie Hixie removed this from the None. milestone Aug 17, 2020
@mraleph
Copy link
Member

mraleph commented Aug 19, 2020

@goderbauer I think the only robust solution here is to introduce a special method swallow which is specially marked and does essentially the following:

@pragma('vm:exception-swallower')
void swallow<T>(T Function() code) {
  try {
    return code();
  } catch (e, st) {
    // Report e & st and do nothing.
  }
}

and then rewrite all other code to use this pattern. Other patterns are very hard to support - note that we need to know at the moment when we throw exception that the catch which is going to catch it is a "swallowing" catch. Because you want to stop execution at the moment of throw - not after exception was caught.

@mkustermann
Copy link
Member

also cc'ing @mkustermann, who's been working on improvements to async stack handling (and may have context in this area)

For synchronous exceptions propagated up the stack, it would be not very difficult to make the debugger break on unhandled exceptions (as well as exceptions that would be caught by a function that was marked with a pragma annotation).

Making this also work for async functions is probably quite a bit more challenging and might only work in a subset of situations.

@mkustermann
Copy link
Member

@cskau-g Could you look into the "synchronous" part of this issue?

Some more explanation:
When an exception gets thrown we make a decision whether to notify the debugger or not. We want to notify the debugger about unhandled exceptions, even if they there is a handler if-and-only-if the handler for the exception has such a pragma. The runtime can simply walk the stack, get the function and see if it has a corresponding @pragma() annotation to make this decision.

@mkustermann mkustermann assigned ghost Dec 3, 2020
@mkustermann mkustermann added this to the 1.27 - January 2021 milestone Dec 18, 2020
dart-bot pushed a commit to dart-lang/sdk that referenced this issue Dec 22, 2020
TEST=runtime/tests/vm/dart{,_2}/notify_debugger_on_exception_test.dart

Bug: flutter/flutter#17007
Change-Id: I988d2385c3d0fc42c4eb769312278261720bb68d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/176663
Commit-Queue: Clement Skau <cskau@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
dart-bot pushed a commit to dart-lang/sdk that referenced this issue Dec 22, 2020
This reverts commit 9b45fcb.

Reason for revert: Broke kernel-precomp

Original change's description:
> [VM] Adds @pragma('vm:notify-debugger-on-exception')
>
> TEST=runtime/tests/vm/dart{,_2}/notify_debugger_on_exception_test.dart
>
> Bug: flutter/flutter#17007
> Change-Id: I988d2385c3d0fc42c4eb769312278261720bb68d
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/176663
> Commit-Queue: Clement Skau <cskau@google.com>
> Reviewed-by: Martin Kustermann <kustermann@google.com>

TBR=kustermann@google.com,cskau@google.com

Change-Id: Ib0ec0ad0a0e2e11f8088c3f57c4085ffc840d1f3
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: flutter/flutter#17007
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/176664
Reviewed-by: Clement Skau <cskau@google.com>
Commit-Queue: Clement Skau <cskau@google.com>
dart-bot pushed a commit to dart-lang/sdk that referenced this issue Jan 7, 2021
Note: This is a reland:
- Relocates the tests.
- Adds status file skip for dartkp.

TEST=runtime/observatory{,_2}/tests/service{,_2}/notify_debugger_on_exception_test.dart

Bug: flutter/flutter#17007
Change-Id: I5c79c140c5e7dee9dfeaa6d4bf750cf4d72f6e85
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/177580
Commit-Queue: Clement Skau <cskau@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
@ghost
Copy link

ghost commented Jan 8, 2021

A CL has landed, adding @pragma('vm:notify-debugger-on-exception') (doc).
This enables the approach mraleph described above.

I believe this addresses everything on the Dart side, so I'll resolve the issue here.

@ghost ghost closed this as completed Jan 8, 2021
@DanTup
Copy link
Contributor Author

DanTup commented Jan 11, 2021

Will this still require some changes in Flutter (adding that @pragma somewhere?)

@ghost
Copy link

ghost commented Jan 11, 2021

I have added the pragma to the VM, but I have not otherwise changed anything about the way exceptions are handled in Dart or Flutter. So for the new mechanism to take effect you'll need some function in the path of the exception to be annotated with the pragma.
The question to me is what the most appropriate place is to put it.

If you're just interested in temporarily pausing the debugger on all exceptions as if they were uncaught, you can add the pragma in places like _incrementCounter() in your example above.

But it might also be that this could be used through-out Flutter for more general effect - but I'm definitely not the right person to make such a call.

@mraleph
Copy link
Member

mraleph commented Jan 11, 2021

I think we should probably reopen because somebody needs to go through Flutter framework and annotate places where exceptions are swallowed, e.g. example from #17007 (comment) needs annotation on GestureRecognizer.invokeCallback.

@mraleph mraleph reopened this Jan 11, 2021
Tools - IDE support review automation moved this from Engineer reviewed to Awaiting triage Jan 11, 2021
@ghost ghost removed their assignment Jan 13, 2021
@ghost
Copy link

ghost commented Jan 13, 2021

Unassigning myself since I'm probably not the right person for the Flutter changes.

@goderbauer
Copy link
Member

@cskau-g It appears that the pragma doesn't work on async methods:

void main() {
  foo();
}

@pragma('vm:notify-debugger-on-exception')
Future<void> foo() async {   // <<<< note the async here
  try {
    throw 'Foo';
  } catch (e) {
    print('exception caught');
  }
  return Future.value();
}

Is that a known limitation or a bug that could be addressed?

There's one occurrence in framework where we'd need to apply it to an async method:

Future<void> handlePlatformMessage(

@mraleph
Copy link
Member

mraleph commented Apr 13, 2021

Yeah, I think I know why it does not work - we missed this in the implementation.

If this is high priority let me know and I will take a look myself, otherwise we wait until @cskau-g is back, he is currently away for a bit.

@goderbauer
Copy link
Member

It's not urgent. I filed a bug over on the dart sdk to keep track of this specific issue: dart-lang/sdk#45673

@goderbauer
Copy link
Member

goderbauer commented Apr 13, 2021

@mraleph Is there a way to apply the pragma to anonymous closures?

void main() {
  bar('Hello World');
}

@pragma('vm:notify-debugger-on-exception')
void bar(String someObject) {
  foo(() {
    try {
      print(someObject);
      throw 'ERROR';
    } catch (e) { // <-- this catch should notify the debugger
      print('exception caught');
    }
  });
}

void foo(Function function) {
  function();
}

Short of refactoring the code to have the try/catch not be part of the closure, I haven't found a way. For the particular instance in the framework, that kind of refactor would make the code a lot more complicated, I believe.

This is the code in the framework that has this problem:

void _layout(ConstraintType constraints) {
owner!.buildScope(this, () {
Widget built;
try {
built = widget.builder(this, constraints);
debugWidgetBuilderValue(widget, built);
} catch (e, stack) {
built = ErrorWidget.builder(
_debugReportException(
ErrorDescription('building $widget'),
e,
stack,
informationCollector: () sync* {
yield DiagnosticsDebugCreator(DebugCreator(this));
},
),
);
}
try {
_child = updateChild(_child, built, null);
assert(_child != null);
} catch (e, stack) {
built = ErrorWidget.builder(
_debugReportException(
ErrorDescription('building $widget'),
e,
stack,
informationCollector: () sync* {
yield DiagnosticsDebugCreator(DebugCreator(this));
},
),
);
_child = updateChild(null, built, slot);
}
});
}

@goderbauer
Copy link
Member

I ran into another issue with the debugger not breaking on the line that causes the exception, but on a (random?) different line instead. This one appears to be unrelated to the pragma and reproduces even without it. I've filed dart-lang/sdk#45684 with steps to reproduce.

@goderbauer
Copy link
Member

After a good night's sleep I thought the problem described in #17007 (comment) can be resolved by refactoring the code as follows:

void main() {
  bar('Hello World');
}

void bar(String someObject) {
  @pragma('vm:notify-debugger-on-exception')
  void myFunction() {
    try {
      print(someObject);
      throw 'ERROR'; // <-- attached debugger expected to break here
    } catch (e) { 
      print('exception caught');
    }
  }
  foo(myFunction);
}

void foo(Function function) {
  function();
}

Unfortunately, the debugger still doesn't break at all. I've filed dart-lang/sdk#45710 for this issue.

@github-actions
Copy link

github-actions bot commented Aug 3, 2021

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a: debugging Debugging, breakpoints, expression evaluation P2 Important issues not at the top of the work list tool Affects the "flutter" command-line tool. See also t: labels.
Projects
Tools - IDE support review
  
Engineer reviewed
Development

Successfully merging a pull request may close this issue.

10 participants