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

Introduce runtime check that it is root isolate that makes UI native calls. #18050

Merged
merged 5 commits into from
May 1, 2020

Conversation

aam
Copy link
Member

@aam aam commented Apr 30, 2020

With upcoming '--enable-isolate-groups' Dart VM option, flutter engine won't be able to register different native call handlers for same library in isolates in the same isolate group. The reason is that library objects are shared between all isolates in the same isolate group.

Proposed solution introduces runtime check into native calls.

aam added 2 commits April 29, 2020 15:30
…ates (flutter#6401)"

This reverts commit 69ae569 as it doesn't work when root and secondary isolates run in the same isolate group.
@aam aam changed the title Move check that it is root isolate that makes UI native calls. Introduce runtime check that it is root isolate that makes UI native calls. Apr 30, 2020
@auto-assign auto-assign bot requested a review from cbracken April 30, 2020 05:37
@aam
Copy link
Member Author

aam commented Apr 30, 2020

Hi @jason-simmons , please let me know what you think about this.

Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

I was attempting to find and validate that each check was necessary (and a couple were not) and realized that chasing all existing callback and adding the check is tedious and error prone. Instead of making it so the thread is checked in each and every single callback, why can't we just update DartLibraryNative::Register to forego the registration if made on the wrong thread. That way, the callback should be faster as well.

@@ -52,42 +52,52 @@ size_t Paragraph::GetAllocationSize() {
}

double Paragraph::width() {
UIDartState::ThrowIfUIOperationsProhited();
Copy link
Member

Choose a reason for hiding this comment

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

This seems redundant. Methods invocations on this C++ object should already go through FOR_EACH_BINDING to DART_NATIVE_CALLBACK which you have already guarded below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, true. Removed.

@aam
Copy link
Member Author

aam commented Apr 30, 2020

why can't we just update DartLibraryNative::Register to forego the registration if made on the wrong thread. That way, the callback should be faster as well.

Foregoing of registration won't work because registration is not per isolate. Registration is per-library and in case of enabled isolate groups where isolates share library objects, affects all isolates in a group. So this change effectively makes secondary isolates register same native handlers, which is harmless but redundant(currently secondary isolate registers reduced number of native handlers breaking root isolate native handler lookups associated with library objects shared between two isolates).
Secondary isolate spawned in existing isolate group don't really need to register native handlers. As a follow-up we could skip that registration for isolates spawned in existing isolate group. In other words, registration has to be done only when isolate group is created. And then, runtime check will ensure that UI-related native calls are only made from the root isolate.

@aam
Copy link
Member Author

aam commented May 1, 2020

Thanks Chinmay. cc @mkustermann

@aam aam merged commit 1c8ee98 into flutter:master May 1, 2020
@aam aam deleted the native-isolate-groups branch May 1, 2020 16:07
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 1, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 1, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 1, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 1, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 1, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 2, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants