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.

Sign up for GitHub

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

Jump to bottom

Engine->Flutter autoroller blocked after dart roll #133677

Closed
gaaclarke opened this issue Aug 30, 2023 · 7 comments
Closed

Engine->Flutter autoroller blocked after dart roll #133677

gaaclarke opened this issue Aug 30, 2023 · 7 comments
Labels
dependency: dart Dart team may need to help us P1 High-priority issues at the top of the work list

Comments

@gaaclarke
Copy link
Member

First example of roll failure: #133624
dart roll: flutter/engine#45262
dart commits in roll: https://dart.googlesource.com/sdk.git/+log/0cea73a8d3c3..96d3a79547fc

I looked through the commit messages and nothing immediately jumped out at me.

error log

11:14 +4: test/integration.shard/break_on_framework_exceptions_test.dart: breaks when platform message callback throws                                                                                 
11:14 +4 -1: test/integration.shard/break_on_framework_exceptions_test.dart: breaks when platform message callback throws [E]                                                                          
  Timed out waiting for VM service pause debug event
  package:matcher                                                        fail
  test/integration.shard/break_on_framework_exceptions_test.dart 623:11  _timeoutAfter.<fn>
  
55s            Spawning flutter [test, --disable-service-auth-codes, --machine, --start-paused, test/test.dart] in /Volumes/Work/s/w/ir/x/t/flutter_break_on_framework_exceptions.dl81v8

56s <=stdout=  {"protocolVersion":"0.1.1","runnerVersion":null,"pid":92785,"type":"start","time":0}

<=stdout=  {"suite":{"id":0,"platform":"vm","path":"/Volumes/Work/s/w/ir/x/t/flutter_break_on_framework_exceptions.dl81v8/test/test.dart"},"type":"suite","time":0}

<=stdout=  {"test":{"id":1,"name":"loading /Volumes/Work/s/w/ir/x/t/flutter_break_on_framework_exceptions.dl81v8/test/test.dart","suiteID":0,"groupIDs":[],"metadata":{"skip":false,"skipReason":null},"line":null,"column":null,"url":null},"type":"testStart","time":1}

66s <=stdout=

<=stdout=  [{"event":"test.startedProcess","params":{"vmServiceUri":"[http://127.0.0.1:52961/","observatoryUri":"http://127.0.0.1:52961](http://127.0.0.1:52961/%22,%22observatoryUri%22:%22http://127.0.0.1:52961)/"}}]

OK (test.startedProcess event)
@gaaclarke gaaclarke added dependency: dart Dart team may need to help us P1 High-priority issues at the top of the work list labels Aug 30, 2023
@gaaclarke gaaclarke mentioned this issue Aug 30, 2023
Merged
@whesse
Copy link
Contributor

whesse commented Aug 30, 2023

The related change in the commit range is https://dart-review.googlesource.com/c/sdk/+/323280

@mraleph

@whesse
Copy link
Contributor

whesse commented Aug 30, 2023

That commit is a fix for dart-lang/sdk#53334 which is copied from Flutter #129121. dart-lang/sdk#47985 may also be related.

zanderso pushed a commit to flutter/engine that referenced this issue Aug 30, 2023
@gaaclarke
Revert "Roll Dart SDK from 0cea73a8d3c3 to 96d3a79547fc" (#45285)
62cf36e 
reverts: #45262

issue: flutter/flutter#133677
@mraleph
Copy link
Member

mraleph commented Aug 30, 2023

I would think that the test is wrong somewhere - it expects this exception to be unhandled but somewhere in the chain of futures there is a future with onError attached to it, which was previously invisible to the debugger, but now is visible.

@gaaclarke
Copy link
Member Author

attn @goderbauer who is the author of the test ( https://github.com/flutter/flutter/blame/main/packages/flutter_tools/test/integration.shard/break_on_framework_exceptions_test.dart#L123 )

@goderbauer, it appears the test is asserting that platform channel handlers that throw cause the debugger to pause, but that is no longer happening because the behavior of the debugger has been changed in a way that is more correct. Specifically, it was reporting exception as "uncaught" erroneously when an Future.then(onError:) was specified.

Our options are:

  1. Hunt down and remove the onError that is catching the exception being thrown in the channel handler. This will cause the debugger to pause again, but potentially the error that happens will be less informative / less helpful when the debugger is not attached. This could turn a warning message to a crash, no?
  2. Delete the test, removing our assertion that this causes the debugger to pause. I imagine there is a workaround for users, they can manually add a breakpoint or add a breakpoint on throw?

@gaaclarke gaaclarke mentioned this issue Aug 30, 2023
Closed
8 tasks
@mraleph
Copy link
Member

mraleph commented Aug 30, 2023

On the second thought it might be a bug in the change as well. I did not account for Zones. I can check a bit later today or you should feel free to revert.

copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Aug 30, 2023
@a-siva
Revert "[vm] Treat Future.then(..., onError:...) as catch all handler"
98b9b6c 
This reverts commit 38e0046.

Reason for revert: flutter/flutter#133677

Original change's description:
> [vm] Treat Future.then(..., onError:...) as catch all handler
>
> Commit a52f2b9 which reworked awaiter stack unwinding and its
> integration with debugger introduced the following regression:
> it stopped treating `Future` listeners which had both `onValue`
> and `onError` callbacks as catch all exception handlers. Only
> listners with `onError` callback (those created with
> `Future.onError`) were treated as a catch all handler.
>
> This meant that debugger started to treat exceptions in the
> code below as uncaught:
>
> ```
> Future<void> foo() {
>   await 0;
>   throw '';
> }
>
> await foo().then(..., onError: (e, st) {
>
> });
> ```
>
> This change fixes this regression by checking if
> `FutureListener.state & stateCatchError != 0` instead of
> more narrow `FutureListener.state == stateCatchError` which
> only detects listeners which only catch errors but do not
> handle values. The new predicate matches
> `FutureListener.handlesError`.
>
> Fixes #53334
>
> TEST=service/pause_on_unhandled_async_exceptions_test
>
> Fixed: 53334
> Change-Id: I374114b7a7b2ef86b7ed6bf7d5e7cf71ba6054dc
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/323280
> Reviewed-by: Alexander Markov <alexmarkov@google.com>
> Reviewed-by: Ben Konyi <bkonyi@google.com>
> Commit-Queue: Slava Egorov <vegorov@google.com>

Change-Id: Id20a6929aac93511173c7467caee91e2488696ed
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/323429
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Siva Annamalai <asiva@google.com>
Reviewed-by: Ben Konyi <bkonyi@google.com>
@a-siva
Copy link
Contributor

a-siva commented Aug 30, 2023

revert of above CL has landed, closing issue.

@a-siva a-siva closed this as completed Aug 30, 2023
@github-actions
Copy link

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 Sep 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
dependency: dart Dart team may need to help us P1 High-priority issues at the top of the work list
Projects
None yet
Milestone
No milestone
4 participants
@mraleph @whesse @a-siva @gaaclarke