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

[CP][stable?] Prevent dart2js crash on flutter web code using package:freezed #48559

Closed
sigmundch opened this issue Mar 14, 2022 · 15 comments
Closed
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. cherry-pick-approved Label for approved cherrypick request merge-to-beta merge-to-stable web-dart2js

Comments

@sigmundch
Copy link
Member

commit(s) to merge: 5090974

merge instructions: needs special cherry-pick CL, but I'd like to discuss first whether we'd approve this hotfix before we create it. It merges cleanly on the beta channel, but not on stable due to some refactoring that occurred in between.

What is the issue: a broken invariant in the kernel representation when there are multiple redirecting factories, this can cause crashes in dart2js downstream.

What is the fix: CFE was updated to fix this invariant by picking the effective target and not the direct target of redirecting calls.

Why cherrypick: The pattern that triggers this bug is commonly generated by package:freezed, a very popular package in the flutter ecosystem. The broken representation silently works in DDC and the VM, but triggers a crash in dart2js for flutter web users.

Risk: low - fix is fairly localized, has been rolled internally.

Link to original issue(s): #47916 (multiple reports merged here, 69+ votes). This is related to flutter/flutter#100038 (a hotfix request on flutter).

/cc @mit-mit @whesse @athomas @vsmenon @devoncarew @johnniwinther

@sigmundch sigmundch added web-dart2js merge-to-stable area-front-end Use area-front-end for front end / CFE / kernel format related issues. area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. cherry-pick-review Issue that need cherry pick triage to approve labels Mar 14, 2022
@mit-mit
Copy link
Member

mit-mit commented Mar 15, 2022

cc @kevmoo for input on customer priority

@athomas
Copy link
Member

athomas commented Mar 15, 2022

Not included in the beta, so we also want to merge this to beta if approved.

/cc @itsjustkevin

@sigmundch
Copy link
Member Author

At our discussion we decided to proceed with including this in beta going forward, but to wait on the stable channel based on whether current users are in the beta/main channel or not.

For beta, it should be a clean merge.

@sigmundch
Copy link
Member Author

I talked offline with @kevmoo and we agreed we'd like to create a cherry-pick for this on both stable and beta.

@johnniwinther
Copy link
Member

CL for merging into stable: https://dart-review.googlesource.com/c/sdk/+/238041

@athomas athomas added cherry-pick-approved Label for approved cherrypick request and removed cherry-pick-review Issue that need cherry pick triage to approve labels Mar 22, 2022
@athomas
Copy link
Member

athomas commented Mar 22, 2022

@itsjustkevin I'm considering this one as approved for both stable and beta based on the offline conversation yesterday.

@whesse
Copy link
Contributor

whesse commented Mar 22, 2022

This CP has landed in 2.16.2 stable.

@sigmundch
Copy link
Member Author

thank you @whesse and @athomas !

@long1eu
Copy link

long1eu commented Mar 23, 2022

How much to we usually need to wait for this to land in Dart and then in Flutter?

@athomas
Copy link
Member

athomas commented Mar 23, 2022

Dart and Flutter releases get released on the same day. In this case that's planned for tomorrow.

@long1eu
Copy link

long1eu commented Mar 25, 2022

It landed in Dart but not Flutter

@athomas
Copy link
Member

athomas commented Mar 26, 2022

@itsjustkevin Did the Flutter release not go out?

@long1eu
Copy link

long1eu commented Mar 26, 2022

AEF951A7-DA2B-4C30-A582-E92C2AB859A4

Not on stable or on beta

@sigmundch
Copy link
Member Author

It appears there was a delay from the flutter side, but just checked this morning again and the release is out in 2.10.4 🎉

@mirland
Copy link

mirland commented Mar 28, 2022

I tested and it fixed my issue! so I think we can close the issue. Thanks for the hard work 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. cherry-pick-approved Label for approved cherrypick request merge-to-beta merge-to-stable web-dart2js
Projects
None yet
Development

No branches or pull requests

7 participants