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] [vm] Flush thread store buffer block after processing finalizers #48964

Closed
dcharkes opened this issue May 5, 2022 · 2 comments
Closed
Assignees
Labels
cherry-pick-approved Label for approved cherrypick request merge-to-stable

Comments

@dcharkes
Copy link
Contributor

dcharkes commented May 5, 2022

Commit(s) to merge

78f218e

Target

stable

Issue Description

When Finalizers and NativeFinalizers are used in Flutter (and dart with --use-compactor), the GC can crash.

What is the fix

Flush the store buffer during GC to account for write to the store buffer during the GC.

Why cherry-pick

When Finalizers or NativeFinalizers are used in Flutter apps, those apps can crash.

Without the fix, developers have to work around this by allocating a new Finalizer or NativeFinalizer for every attachment. But we have no way of communicating that to all developers trying out finalizers.

Risk

low

Issue link(s)

#48843

Extra Info

This is a 4 line fix:

https://dart-review.googlesource.com/c/sdk/+/243262/12/runtime/vm/heap/marker.cc
https://dart-review.googlesource.com/c/sdk/+/243262/12/runtime/vm/thread.cc

The rest is tests/logging.

@mraleph @rmacnak-google Would you consider this a low risk cherry pick?

cc @mkustermann

@dcharkes dcharkes added the cherry-pick-review Issue that need cherry pick triage to approve label May 5, 2022
@athomas
Copy link
Member

athomas commented May 5, 2022

Based on an offline discussion with @mit-mit and @mraleph we're treating this as approved and will prepare a beta build with this CP. We can still decide not to publish this.

@itsjustkevin

@athomas athomas added cherry-pick-approved Label for approved cherrypick request merge-to-stable and removed cherry-pick-review Issue that need cherry pick triage to approve labels May 5, 2022
@sortie
Copy link
Contributor

sortie commented May 5, 2022

I cherry-picked this change as 2.17.0-266.10.beta (fc290d1).

@athomas athomas closed this as completed May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-approved Label for approved cherrypick request merge-to-stable
Projects
None yet
Development

No branches or pull requests

7 participants