Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Made sure not to delete handles of dart objects if the isolate has been deleted #25506

Merged
merged 7 commits into from
Apr 14, 2021
Merged

Made sure not to delete handles of dart objects if the isolate has been deleted #25506

merged 7 commits into from
Apr 14, 2021

Conversation

@gaaclarke
Copy link
Member

@gaaclarke gaaclarke commented Apr 9, 2021

This fixes a crash but can result in memory living longer than expected. The memory fix will require a larger engineering effort. I think we should land this in the meantime. See the attached issue for a detailed explanation about what is happening here.

fixes flutter/flutter#79335

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.
  • The reviewer has submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

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

@google-cla google-cla bot added the cla: yes label Apr 9, 2021
gaaclarke
gaaclarke commented Apr 9, 2021
View reviewed changes
third_party/tonic/dart_persistent_value.cc
/// may cause memory to stick around until the isolate group is destroyed.
/// Currently if DartState::IsShuttingDown == true, this code will crash when
/// binding the isolate.
if (!dart_state->IsShuttingDown()) {
Copy link
Member Author

@gaaclarke gaaclarke Apr 9, 2021

Choose a reason for hiding this comment

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

This matches the logic in DartWeakPersistentValue.

@gaaclarke
Copy link
Member Author

gaaclarke commented Apr 9, 2021

@xster @mkustermann what do you think about this approach for now? I don't know what our policy is for editing tonic is, I'll come up with a unit test for the DartIsolate stuff, too.

@mkustermann
Copy link
Member

mkustermann commented Apr 9, 2021 •
edited
Loading

@xster @mkustermann what do you think about this approach for now? I don't know what our policy is for editing tonic is, I'll
come up with a unit test for the DartIsolate stuff, too.

This LGTM. Checking for shutdown condition is solving the exact same issue as we already do in weak persistent handles, so it should have the same solution. This fix is independent of lightweight isolates.

(The original commit that added this condition to weak handles had some simple tests in it - maybe you can add a test case there, see ccdb681)

The fact that we might leak those handles is specific to --enable-lightweight-isolates and can be solved separately.

@gaaclarke Since this is low-risk, so we should cherry-pick this to the stable branch if possible.

@gaaclarke gaaclarke force-pushed the dont-delete-handle-after-isolate-deleted branch from 96eb4cc to cbe2785 Compare April 9, 2021 17:14
@gaaclarke gaaclarke marked this pull request as ready for review April 9, 2021 21:32
@gaaclarke gaaclarke requested a review from xster April 9, 2021 21:32
@gaaclarke gaaclarke mentioned this pull request Apr 9, 2021
Open
xster
xster reviewed Apr 12, 2021
View reviewed changes
third_party/tonic/dart_persistent_value.cc Outdated
Dart_DeletePersistentHandle(value_);
/// TODO(80155): Remove the handle even if the isolate is shutting down. This
/// may cause memory to stick around until the isolate group is destroyed.
/// Currently if DartState::IsShuttingDown == true, this code will crash when
Copy link
Member

@xster xster Apr 12, 2021

Choose a reason for hiding this comment

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

From the perspective of someone reading this in the future, this should read "Previously" right?

Copy link
Member Author

@gaaclarke gaaclarke Apr 12, 2021

Choose a reason for hiding this comment

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

I was referring to the code inside of the branch, that will crash. Reworded it.

@gaaclarke gaaclarke added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 12, 2021
@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Apr 12, 2021

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite build_and_test_linux_unopt_debug has failed. Please fix the issues identified (or deflake) before re-applying this label.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 12, 2021
@sroddy
Copy link

sroddy commented Apr 14, 2021

@gaaclarke is it possible to merge this?

@gaaclarke
Copy link
Member Author

gaaclarke commented Apr 14, 2021

Yea, the bot was suppose to do it automatically, sorry.

@gaaclarke gaaclarke merged commit 4f6462b into flutter:master Apr 14, 2021
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 14, 2021
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 14, 2021
@engine-flutter-autoroll
4f6462b Made sure not to delete handles of dart objects if the isolat…
6168dd7 
…e has been deleted (flutter/engine#25506)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 14, 2021
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 14, 2021
@engine-flutter-autoroll
4f6462b Made sure not to delete handles of dart objects if the isolat…
163332a 
…e has been deleted (flutter/engine#25506)
christopherfujino added a commit that referenced this pull request Apr 14, 2021
@christopherfujino @gaaclarke
[flutter_releases] Flutter Dev 2.2.0-10.0.pre Engine Cherrypicks (#25595
15ee76c 
)

* Made sure not to delete handles of dart objects if the isolate has been deleted (#25506)

* Roll Dart SDK to 2.13.0-211.6.beta

* update licenses golden

Co-authored-by: gaaclarke <[email protected]>
@sroddy sroddy mentioned this pull request Apr 16, 2021
Merged
@sroddy
Copy link

sroddy commented Apr 16, 2021

@gaaclarke is it possible to have this cherry-picked to the upcoming stable hot fix as proposed by @mkustermann?

@xster
Copy link
Member

xster commented Apr 16, 2021

It already is cherry picked in #25595

duanqz pushed a commit to duanqz/engine that referenced this pull request Apr 16, 2021
@gaaclarke @duanqz
Made sure not to delete handles of dart objects if the isolate has be…
29ad97f 
…en deleted (flutter#25506)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@xster xster xster approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Crash on iOS when using multiple engines spawned from FlutterEngineGroup

5 participants

@gaaclarke @mkustermann @fluttergithubbot @sroddy @xster