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

Support cleaner Dart isolate shutdown handling.#4096

Merged
ianloic merged 1 commit intoflutter:masterfrom
ianloic:isolate-shutdown
Sep 13, 2017
Merged

Support cleaner Dart isolate shutdown handling.#4096
ianloic merged 1 commit intoflutter:masterfrom
ianloic:isolate-shutdown

Conversation

@ianloic
Copy link
Contributor

@ianloic ianloic commented Sep 12, 2017

If an isolate shuts down (for example if an app calls
Isolate.current.kill()), the UIDartState* on DartController will refer
to a freed object. This wires through notification that the is shutting
down through to the DartController so it can clean up appropriately.

@ianloic
Support cleaner Dart isolate shutdown handling.
d2e51c0 
If an isolate shuts down (for example if an app calls
Isolate.current.kill()), the UIDartState* on DartController will refer
to a freed object. This wires through notification that the is shutting
down through to the DartController so it can clean up appropriately.
@ianloic ianloic requested review from abarth and zanderso September 12, 2017 22:32
@zanderso
Copy link
Member

zanderso commented Sep 13, 2017

This lgtm, but I'm curious if there was a crash because something was touching the dangling state. Also, I think someone more familiar with the engine should double-check this.

@ianloic
Copy link
Contributor Author

ianloic commented Sep 13, 2017

So the problem is that DartController has a pointer to UIDartState*, but the UIDartState is freed by isolate shutdown. If the isolate shuts down because it has an uncaught exception or it's killed with Isolate.kill() or whatever DartController ends up with a dangling pointer to the freed object.

This makes sure that doesn't happen.

@ianloic ianloic merged commit 05751f7 into flutter:master Sep 13, 2017
@goderbauer goderbauer mentioned this pull request Sep 13, 2017
Merged
@xster
Copy link
Member

xster commented Sep 13, 2017

Seems like flutter test no longer works with engine versions past this one. A lot of tests fail with something like

00:02 +21 -12: loading ../flutter/packages/flutter/test/material/colors_test.dart [E]
  Failed to load "../flutter/packages/flutter/test/material/colors_test.dart":
  Shell subprocess crashed with segmentation fault before connecting to test harness.
  Test: ../flutter/packages/flutter/test/material/colors_test.dart
  Shell: ../flutter/bin/cache/artifacts/engine/darwin-x64/flutter_tester

@aam
Copy link
Member

aam commented Sep 14, 2017

@ianloic , Issue @xster identified above prevents getting updated engine into flutter.
If there is no clear solution for the segfault crash right this moment, can this be rolled back?

@aam aam mentioned this pull request Sep 14, 2017
Closed
aam added a commit to aam/engine that referenced this pull request Sep 14, 2017
@aam
Revert "Support cleaner Dart isolate shutdown handling. (flutter#4096)"
c708515 
This reverts commit 05751f7.
aam added a commit that referenced this pull request Sep 14, 2017
@aam
Reverting to unblock engine roll (#4109)
0e8ffa2 
* Revert "Make Travis happy again (#4101)"

This reverts commit c99b255.

* Revert "Support cleaner Dart isolate shutdown handling. (#4096)"

This reverts commit 05751f7.
@ianloic ianloic mentioned this pull request Sep 18, 2017
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@abarth abarth Awaiting requested review from abarth

@zanderso zanderso Awaiting requested review from zanderso

1 more reviewer

@jason-simmons jason-simmons jason-simmons 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.

6 participants

@ianloic @zanderso @xster @aam @jason-simmons @googlebot