Skip to content

catch StateError thrown from Chromium.close #154366

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
Merged
auto-submit merged 4 commits into from
Aug 30, 2024
Merged

catch StateError thrown from Chromium.close #154366

auto-submit merged 4 commits into from
Aug 30, 2024

Conversation

@andrewkolos
Copy link
Contributor

@andrewkolos andrewkolos commented Aug 29, 2024

Fixes crasher #154203.

Also does a little refactoring to move exception handling into the getChromeTabGuarded utility function.

I plan on cherry-picking a subset of this change. This would be included in the custom CP patch for #153064.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@andrewkolos andrewkolos added the tool Affects the "flutter" command-line tool. See also t: labels. label Aug 29, 2024
andrewkolos
andrewkolos commented Aug 29, 2024
View reviewed changes
packages/flutter_tools/lib/src/web/chrome.dart
}) async {
try {
return asyncGuard(() => chromeConnection.getTab(accept, retryFor: retryFor));
} on IOException catch (error, stackTrace) {
Copy link
Contributor Author

@andrewkolos andrewkolos Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should just be a blanket on (e) catch, but I'm being conservative.

@andrewkolos andrewkolos marked this pull request as ready for review August 29, 2024 21:26
christopherfujino
christopherfujino reviewed Aug 29, 2024
View reviewed changes
packages/flutter_tools/lib/src/isolated/resident_web_runner.dart Outdated
return !chromeTab.url.startsWith('chrome-extension');
},
retryFor: const Duration(seconds: 5),
onError: (Object error, StackTrace stackTrace) {
Copy link
Contributor

@christopherfujino christopherfujino Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this now mean we're swallowing all errors?

Copy link
Contributor

@christopherfujino christopherfujino Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, it looks like this only gets delegated IOException and StateError?

Copy link
Contributor Author

@andrewkolos andrewkolos Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yeah. Anything that's not IOException or StateError will fall through. Maybe it should just catch everything.

Copy link
Contributor Author

@andrewkolos andrewkolos Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in refactor: add clarifying docs. I decided to just rename the parameter to not imply that any error would be caught.

christopherfujino
christopherfujino reviewed Aug 29, 2024
View reviewed changes
packages/flutter_tools/lib/src/test/flutter_web_platform.dart
} on FormatException catch (ex) {
_logger.printError('Caught FormatException: $ex');
return shelf.Response.ok('Caught exception: $ex');
} on IOException catch (ex) {
Copy link
Contributor

@christopherfujino christopherfujino Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we not catching this here?

Copy link
Contributor Author

@andrewkolos andrewkolos Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This catch was introduced recently, but before I introduced getChromeTabGuarded. getChromeTabGuarded should already catch this.

christopherfujino
christopherfujino approved these changes Aug 29, 2024
View reviewed changes
Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@andrewkolos andrewkolos added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 29, 2024
@auto-submit auto-submit bot merged commit a8528c7 into flutter:master Aug 30, 2024
@andrewkolos andrewkolos mentioned this pull request Aug 30, 2024
Merged
9 tasks
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 30, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2024
@engine-flutter-autoroll
a8528c7 catch StateError thrown from Chromium.close (flutter/flut…
a79fa7a 
…ter#154366)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 30, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2024
@engine-flutter-autoroll
a8528c7 catch StateError thrown from Chromium.close (flutter/flut…
27c0a8e 
…ter#154366)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 30, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2024
@engine-flutter-autoroll
a8528c7 catch StateError thrown from Chromium.close (flutter/flut…
ee2b976 
…ter#154366)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 30, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2024
@engine-flutter-autoroll
a8528c7 catch StateError thrown from Chromium.close (flutter/flut…
bd0b00e 
…ter#154366)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 30, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2024
@engine-flutter-autoroll
a8528c7 catch StateError thrown from Chromium.close (flutter/flut…
d04981d 
…ter#154366)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 30, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2024
@engine-flutter-autoroll
a8528c7 catch StateError thrown from Chromium.close (flutter/flut…
35d7cac 
…ter#154366)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 30, 2024
Closed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 1, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 1, 2024
@engine-flutter-autoroll
a8528c7 catch StateError thrown from Chromium.close (flutter/flut…
6517455 
…ter#154366)
Buchimi pushed a commit to Buchimi/flutter that referenced this pull request Sep 2, 2024
@andrewkolos @Buchimi
catch StateError thrown from Chromium.close (flutter#154366)
9b0818e 
Fixes crasher flutter#154203.

Also does a little refactoring to move exception handling into the `getChromeTabGuarded` utility function.

I plan on cherry-picking a subset of this change. This would be included in the custom CP patch for flutter#153064.
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 3, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 3, 2024
@engine-flutter-autoroll
a8528c7 catch StateError thrown from Chromium.close (flutter/flut…
1de891f 
…ter#154366)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 3, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 3, 2024
@engine-flutter-autoroll
a8528c7 catch StateError thrown from Chromium.close (flutter/flut…
6bd885b 
…ter#154366)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 3, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 3, 2024
@engine-flutter-autoroll
a8528c7 catch StateError thrown from Chromium.close (flutter/flut…
848a352 
…ter#154366)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 3, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 3, 2024
@engine-flutter-autoroll
a8528c7 catch StateError thrown from Chromium.close (flutter/flut…
cb58138 
…ter#154366)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 3, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 3, 2024
@engine-flutter-autoroll
a8528c7 catch StateError thrown from Chromium.close (flutter/flut…
b2c9537 
…ter#154366)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 3, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 3, 2024
@engine-flutter-autoroll
a8528c7 catch StateError thrown from Chromium.close (flutter/flut…
1557b2f 
…ter#154366)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 4, 2024
@engine-flutter-autoroll
a8528c7 catch StateError thrown from Chromium.close (flutter/flut…
7231c4f 
…ter#154366)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 4, 2024
@engine-flutter-autoroll
a8528c7 catch StateError thrown from Chromium.close (flutter/flut…
a8f4d96 
…ter#154366)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 4, 2024
@engine-flutter-autoroll
a8528c7 catch StateError thrown from Chromium.close (flutter/flut…
4924b02 
…ter#154366)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 4, 2024
@engine-flutter-autoroll
a8528c7 catch StateError thrown from Chromium.close (flutter/flut…
b19f74a 
…ter#154366)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 4, 2024
@engine-flutter-autoroll
a8528c7 catch StateError thrown from Chromium.close (flutter/flut…
e530f0a 
…ter#154366)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 4, 2024
@engine-flutter-autoroll
a8528c7 catch StateError thrown from Chromium.close (flutter/flut…
9298a2b 
…ter#154366)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 5, 2024
@engine-flutter-autoroll
a8528c7 catch StateError thrown from Chromium.close (flutter/flut…
74d01f6 
…ter#154366)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 5, 2024
@engine-flutter-autoroll
a8528c7 catch StateError thrown from Chromium.close (flutter/flut…
49ba6e9 
…ter#154366)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 5, 2024
@engine-flutter-autoroll
a8528c7 catch StateError thrown from Chromium.close (flutter/flut…
7733ecb 
…ter#154366)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 5, 2024
@engine-flutter-autoroll
a8528c7 catch StateError thrown from Chromium.close (flutter/flut…
6062a72 
…ter#154366)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 5, 2024
@engine-flutter-autoroll
a8528c7 catch StateError thrown from Chromium.close (flutter/flut…
82f5b14 
…ter#154366)
@andrewkolos andrewkolos mentioned this pull request Sep 10, 2024
Closed
auto-submit bot pushed a commit that referenced this pull request Sep 11, 2024
@andrewkolos
[CP-stable] When shutting down chrome, guard the call to `ChromeConne…
90c7ddb 
…ction.getTab` (#154380)

This is a custom hotfix patch to fix #154366 and #153064 on the stable channel. The reason for a custom patch rather than a bot-created PR is that the fixes on master were done by two sequential PRs (#153978 and #154366). This CP patch is also a subset of those changes (calls to Chrome when shutting down at the end of `flutter run -d chrome`).
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 11, 2024
@engine-flutter-autoroll
a8528c7 catch StateError thrown from Chromium.close (flutter/flut…
c08102d 
…ter#154366)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 12, 2024
@engine-flutter-autoroll
a8528c7 catch StateError thrown from Chromium.close (flutter/flut…
40e5782 
…ter#154366)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@christopherfujino christopherfujino christopherfujino approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@andrewkolos @christopherfujino