Migrate Dart_WeakPersistentHandle uses and roll Dart #19843

dcharkes · 2020-07-17T16:32:15Z

Description

The PR is migrating the users of Dart_WeakPersistentHandle because the semantics of that is changing to be non-auto-deleting to address races between the GC and API use. This rolls Dart to dart-lang/sdk@5278383.

Most notably, this PR migrates DartWrappable to use the non-auto-deleting WeakPersistentHandles.

It introduces DartWeakPersistentValue to manage WeakPersistentHandles, analogous to how DartPersistentValue manages PersistentHandles.
It migrates DartWrappable to use DartWeakPersistentValue.
DartWrappable destructors can still be called on isolate shutdown, this PR adds DartState::is_shutting_down_ to cover this case.
DartWrappable destructors can no longer be called on arbitrary threads, because Dart_DeleteWeakPersistentHandle now has to be invoked manually and this must happen on the Dart mutator thread.
- This PR migrates the one use site where this showed up (src/flutter/lib/ui/painting/image_decoder.cc).

Related Issues

dart-lang/sdk#42312

Tests

I did not write new tests but ran the existing tests on HHH infrastructure. https://dart-review.googlesource.com/c/sdk/+/151525/27

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the contributor guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the C++, Objective-C, Java style guides for the engine.
I read the tree hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

https://dart-review.googlesource.com/c/sdk/+/151525 will break the Flutter build. This PR needs to be done with a Dart roll incorporating that CL.

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

fluttergithubbot · 2020-07-17T16:32:17Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

mkustermann · 2020-07-22T14:02:57Z

@dcharkes Could you split out the finalizable handle part into a different PR which we can land normally. That way we keep the manual roll addressing the breaking API change small.

third_party/tonic/dart_weak_persistent_value.h

third_party/tonic/dart_weak_persistent_value.cc

third_party/tonic/dart_weak_persistent_value.h

third_party/tonic/dart_weak_persistent_value.cc

dcharkes · 2020-07-29T10:20:28Z

@dcharkes Could you split out the finalizable handle part into a different PR which we can land normally. That way we keep the manual roll addressing the breaking API change small.

Done: #20107. Unfortunately, it's only a small part of the PR.

gaaclarke

I'm not familiar with this code but I gave it a look. Looks good, I just had a few questions.

So, this is adding no functionality, just a refactor? That's why we are fine just relying on the old tests? There is some new surface like DartWeakPersistentValue which could be tested FWIW.

shell/platform/fuchsia/dart_runner/dart_runner.cc

third_party/tonic/dart_state.h

third_party/tonic/dart_wrappable.h

runtime/dart_isolate.cc

dcharkes · 2020-07-31T13:52:21Z

Updating DEPS to point to a dart-lang/sdk branch does not seem to work out of the box. I'll check later if I can make that work to turn the CQ here green.

In the mean time: another build run on the DartCQ: https://ci.chromium.org/p/dart/builders/try/flutter-engine-linux-try/327? (All green.) Tests also locally succeeded on revisions:

    git_revisions.rev_engine = "e1c9673bbb63c68a840b8978f39bc0b059420e4d"
    git_revisions.rev_flutter = "30aef0a3b9611763f8e60985e7cca9cb30c1ea6a"
    git_revisions.rev_dart = "caaa2a752b85dd6c4afef5761651e0318dd2d49a"

dcharkes · 2020-09-01T16:25:11Z

@chinmaygarde Could you please have a look at this PR? (See the updated PR description.)

In particular:

Does this PR fit the overall architecture?
Are there any other places where DartWrappable destructors might be invoked on non-Dart-mutator threads? (Tests did not reveal any other instance besides the one migrated, but they could succeed based on timing.)

dcharkes · 2020-09-10T12:03:49Z

@gaaclarke

So, this is adding no functionality, just a refactor?

Yes, correct.

dcharkes · 2020-10-02T09:54:31Z

I updated the PR to use the DEPS file to point to the branch containing the Dart changes.

(Note that the licenses check is failing because I based the Dart branch on master instead of the commit that was in the DEPS file before.)

third_party/tonic/dart_weak_persistent_value.cc

third_party/tonic/dart_weak_persistent_value.h

zanderso · 2020-10-26T20:46:47Z

Sorry for the long delay. My understanding is that an engine unit test would be able to make calls against the tonic API. That is, I don't think the test for this PR needs to be encapsulated entirely within tonic. Maybe I'm misunderstanding something? @chinmaygarde can correct me if I'm mistaken.

fluttergithubbot · 2020-11-02T14:08:03Z

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

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

zanderso

lgtm w/ license file fix

DEPS

Changes Dart_NewWeakPersistentHandle to no longer auto delete the weak persistent handle. Changes the signatures of WeakPersistentHandleFinalizers to no longer have access to the handle. Flutter PR: flutter/engine#19843 g3 presubmit: cl/318028238 Issue: #42312 TEST=runtime/vm/dart_api_impl_test.cc Change-Id: I3f77db9954d9486759f903b78c03a494f73c68ba Cq-Include-Trybots:dart/try:vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64-try,app-kernel-linux-debug-x64-try,vm-kernel-linux-debug-ia32-try,vm-kernel-win-debug-x64-try,vm-kernel-win-debug-ia32-try,vm-kernel-precomp-linux-debug-x64-try,vm-kernel-precomp-android-release-arm64-try,vm-kernel-asan-linux-release-x64-try,vm-kernel-msan-linux-release-x64-try,vm-kernel-precomp-msan-linux-release-x64-try,vm-kernel-linux-release-simarm-try,vm-kernel-linux-release-simarm64-try,vm-kernel-precomp-android-release-arm_x64-try,vm-kernel-precomp-obfuscate-linux-release-x64-try,dart-sdk-linux-try,analyzer-analysis-server-linux-try,analyzer-linux-release-try,front-end-linux-release-x64-try,vm-kernel-precomp-win-release-x64-try,vm-kernel-mac-debug-x64-try,vm-precomp-ffi-qemu-linux-release-arm-try,vm-kernel-nnbd-linux-debug-x64-try,analyzer-nnbd-linux-release-try,front-end-nnbd-linux-release-x64-try Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/151525 Commit-Queue: Daco Harkes <[email protected]> Reviewed-by: Ryan Macnak <[email protected]> Reviewed-by: Martin Kustermann <[email protected]>

dcharkes · 2020-11-03T12:05:03Z

Requested changes with DEPS & licences addressed, merging.

* c597333 Roll Skia from f548a028ce70 to c21902c0d3cc (46 revisions) (flutter/engine#22224) * 37d766c Fix includes to start with shell (flutter/engine#22227) * 1ad6765 [web] Fixes canvas pixelation and overallocation due to transforms. (flutter/engine#22160) * 9f9fc1f Roll Skia from c21902c0d3cc to 9615bcf71f2a (1 revision) (flutter/engine#22226) * 0faa72e Roll Dart SDK from fed66f60a3bc to 25ef5dc559cf (1 revision) (flutter/engine#22225) * 172a393 Report image diff status for iOS scenario golden tests (flutter/engine#22230) * fddabca updating integration tests version. (flutter/engine#22235) * a36bcdc Roll Fuchsia Mac SDK from mhak7e_o6... to 8SkbMXJJ9... (flutter/engine#22231) * 6a331d3 Roll Skia from 9615bcf71f2a to d5e6368fffd0 (8 revisions) (flutter/engine#22234) * 9b34207 Fixing semantics borders on mobile web (flutter/engine#21856) * 4e9459e Refactored the FlutterEngine to make it easier to implement spawn functionality (flutter/engine#21890) * fa77e68 disable AppLifecycleTests (flutter/engine#22236) * 153775b update golden (flutter/engine#22247) * 50dbe85 [web] fix hot restart type error (flutter/engine#22248) * c8cf09a Roll Skia from d5e6368fffd0 to 7585a65ac709 (7 revisions) (flutter/engine#22237) * 68e2e46 Roll Fuchsia Mac SDK from 8SkbMXJJ9... to Pz4ZHZrUp... (flutter/engine#22246) * d3182bc Roll Dart SDK from 25ef5dc559cf to 5acb5fcf84cb (4 revisions) (flutter/engine#22243) * 9b4bb20 makes android semanticsnode to ignore hittest if it is not focusable (flutter/engine#22205) * 3c7a54e Roll Fuchsia Linux SDK from sNx8qabBn... to QqGvMWaYk... (flutter/engine#22244) * eea98b2 Roll Skia from 7585a65ac709 to dffd20efe95c (14 revisions) (flutter/engine#22250) * 46e3bba Defer Windows arrow key and delete handling (flutter/engine#22207) * 14437d6 fix _getArrayBuffer signature (flutter/engine#22251) * 8defec6 Fix nullability issue with Image.network (flutter/engine#22252) * 67d55ed Roll Dart SDK from 5acb5fcf84cb to a9d583383410 (4 revisions) (flutter/engine#22255) * 7c8f57c Report error when instantiating CanvasKit network image (flutter/engine#22159) * 9945db3 Remove the metrics task from cirrus. (flutter/engine#22240) * bd19181 Add braces on if statements to match linter style (flutter/engine#22130) * e9c62e7 do not print in _computePixelDensity (flutter/engine#22257) * 2617101 Roll Dart SDK from a9d583383410 to d2577410a501 (1 revision) (flutter/engine#22258) * 3d194fa Switch macOS embedding to proc table embedder API (flutter/engine#21811) * 31b6f0b Roll Fuchsia Mac SDK from Pz4ZHZrUp... to 6yEx5GNGG... (flutter/engine#22262) * 78a0181 Roll Fuchsia Linux SDK from QqGvMWaYk... to oLF1FW-gC... (flutter/engine#22264) * ccdb681 WeakPersistentHandle migration (flutter/engine#19843) * ce0a30c Roll Dart SDK from 52783837369d to b43baaaa477d (723 revisions) (flutter/engine#22265) * 59b01e0 [web] Fix repaint logic for cullrect,transform changes (flutter/engine#22273)

and roll Dart to 5278383.

…that cross thread-boundaries. In flutter#19843, a regression was introduced where an image descriptor object that has a Dart peer could be collected on a concurrent task runner thread. Collection of such object needs to enter isolate scope which might still be active on the UI thread. [A comment in the original commit explicitly mentions][1] this and attempts to achieve the collection of the Dart wrappable on the UI task runner. However, the author missed that the object was present in the captures of a copyable closure. These captures may be released on any of the participating threads. To avoid the the indeterminism of the smart pointer being dropped on the concurrent task runner thread, this patch resorts to manually reference counting the descriptor. Fixes flutter/flutter#72144 [1]: https://github.com/flutter/engine/pull/19843/files#diff-a3034d4a06133381b6b636d841ec98cb7cfce640f316dda9e71eda4d9e7872f6R227

googlebot added the cla: yes label Jul 17, 2020

auto-assign bot requested a review from gaaclarke July 17, 2020 16:32

dcharkes requested review from chinmaygarde, cbracken and mkustermann and removed request for gaaclarke July 17, 2020 16:33

mkustermann reviewed Jul 22, 2020

View reviewed changes

dcharkes mentioned this pull request Jul 29, 2020

Migrate some Dart_WeakPersistentHandle uses to Dart_FinalizableHandle #20107

Merged

8 tasks

gaaclarke reviewed Jul 30, 2020

View reviewed changes

shell/platform/fuchsia/dart_runner/dart_runner.cc Show resolved Hide resolved

third_party/tonic/dart_state.h Show resolved Hide resolved

third_party/tonic/dart_wrappable.h Show resolved Hide resolved

runtime/dart_isolate.cc Show resolved Hide resolved

dcharkes force-pushed the weak-persistent-handle-migration branch 2 times, most recently from 1ae07ab to a4cf7cd Compare July 31, 2020 13:38

dcharkes force-pushed the weak-persistent-handle-migration branch from a4cf7cd to a6c09b8 Compare August 13, 2020 09:39

dcharkes force-pushed the weak-persistent-handle-migration branch from a6c09b8 to 61c9ac7 Compare September 1, 2020 16:00

dcharkes requested a review from gaaclarke September 10, 2020 12:02

chinmaygarde requested review from zanderso and jason-simmons September 24, 2020 21:56

dcharkes force-pushed the weak-persistent-handle-migration branch 2 times, most recently from a8338b1 to 0638046 Compare October 2, 2020 09:00

zanderso reviewed Oct 5, 2020

View reviewed changes

third_party/tonic/dart_weak_persistent_value.cc Show resolved Hide resolved

third_party/tonic/dart_weak_persistent_value.h Show resolved Hide resolved

dcharkes force-pushed the weak-persistent-handle-migration branch 2 times, most recently from ffa089e to 52d1d8f Compare October 30, 2020 18:16

chinmaygarde approved these changes Oct 30, 2020

View reviewed changes

dcharkes force-pushed the weak-persistent-handle-migration branch 3 times, most recently from 07bbce3 to 0d7952a Compare October 30, 2020 22:00

dcharkes 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 Nov 2, 2020

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 Nov 2, 2020

zanderso approved these changes Nov 2, 2020

View reviewed changes

zanderso suggested changes Nov 2, 2020

View reviewed changes

DEPS Outdated Show resolved Hide resolved

dcharkes force-pushed the weak-persistent-handle-migration branch from 0d7952a to 1628bbd Compare November 3, 2020 10:29

dcharkes requested a review from zanderso November 3, 2020 10:31

WeakPersistentHandle migration

a70f5ee

dcharkes force-pushed the weak-persistent-handle-migration branch from 1628bbd to a70f5ee Compare November 3, 2020 10:58

dcharkes changed the title ~~Migrate Dart_WeakPersistentHandle uses~~ Migrate Dart_WeakPersistentHandle uses and roll Dart Nov 3, 2020

dcharkes merged commit ccdb681 into flutter:master Nov 3, 2020

engine-flutter-autoroll mentioned this pull request Nov 3, 2020

Roll Engine from 346051939814 to ccdb6819ce53 (33 revisions) flutter/flutter#69672

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 3, 2020

ccdb681 WeakPersistentHandle migration (flutter/engine#19843)

ef3c147

engine-flutter-autoroll mentioned this pull request Nov 3, 2020

Roll Engine from 346051939814 to ccdb6819ce53 (33 revisions) flutter/flutter#69692

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 3, 2020

ccdb681 WeakPersistentHandle migration (flutter/engine#19843)

9ab87dd

a-siva mentioned this pull request Nov 3, 2020

Roll Dart SDK from 52783837369d to b43baaaa477d (723 revisions) #22265

Merged

engine-flutter-autoroll mentioned this pull request Nov 3, 2020

Roll Engine from 346051939814 to 59b01e0e5a3f (35 revisions) flutter/flutter#69709

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 3, 2020

ccdb681 WeakPersistentHandle migration (flutter/engine#19843)

15c6e2f

dcharkes deleted the weak-persistent-handle-migration branch November 6, 2020 08:18

chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020

WeakPersistentHandle migration (flutter#19843)

3cff227

and roll Dart to 5278383.

chinmaygarde mentioned this pull request Dec 14, 2020

Enter Isolate : Multiple mutators entering an isolate / Dart VM is shutting down flutter/flutter#72144

Closed

mkustermann mentioned this pull request Dec 18, 2020

Android: Unable to Enter Isolate : Multiple mutators entering an isolate / Dart VM is shutting down dart-lang/sdk#44447

Closed

chinmaygarde mentioned this pull request Jan 7, 2021

During image decoding, avoid using smart pointers for DartWrappables that cross thread-boundaries. #23503

Merged

Migrate Dart_WeakPersistentHandle uses and roll Dart #19843

Migrate Dart_WeakPersistentHandle uses and roll Dart #19843

Uh oh!

Conversation

dcharkes commented Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

fluttergithubbot commented Jul 17, 2020

Uh oh!

mkustermann commented Jul 22, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dcharkes commented Jul 29, 2020

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dcharkes commented Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcharkes commented Sep 1, 2020

Uh oh!

dcharkes commented Sep 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcharkes commented Oct 2, 2020

Uh oh!

Uh oh!

Uh oh!

zanderso commented Oct 26, 2020

Uh oh!

fluttergithubbot commented Nov 2, 2020

Uh oh!

zanderso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dcharkes commented Nov 3, 2020

Uh oh!

Uh oh!

dcharkes commented Jul 17, 2020 •

edited

Loading

dcharkes commented Jul 31, 2020 •

edited

Loading

dcharkes commented Sep 10, 2020 •

edited

Loading