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.

Sign up for GitHub

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

Jump to bottom

[breaking change][vm] Add class modifiers to dart:nativewrappers #51896

Closed
dcharkes opened this issue Mar 30, 2023 · 10 comments
Closed

[breaking change][vm] Add class modifiers to dart:nativewrappers #51896

dcharkes opened this issue Mar 30, 2023 · 10 comments
Assignees
@dcharkes
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. breaking-change-approved breaking-change-request This tracks requests for feedback on breaking changes vm-native
Milestone
Dart 3 cleanup

Comments

@dcharkes
Copy link
Contributor

dcharkes commented Mar 30, 2023 •
edited

The NativeWrapperClasses should be marked base so that none of their subtypes can be implemented.

Implementing subtypes can lead to issues when passing such native wrapper to a native call, as it will try to unwrap a native field that doesn't exist (likely leading to a segfault).

CL: https://dart-review.googlesource.com/c/sdk/+/291761

Flutter has classes which extend NativeWrapperClasses and are mocked in tests.
And Flutter has already opted in to 3.0, so adding the class modifiers to dart:nativewrappers is a breaking change for Flutter.

log
@dcharkes dcharkes added area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. vm-native labels Mar 30, 2023
@dcharkes dcharkes self-assigned this Mar 30, 2023
@dcharkes dcharkes added this to the Dart 3 beta 3 milestone Mar 30, 2023
@dcharkes dcharkes mentioned this issue Mar 30, 2023
Closed
@dcharkes
Copy link
Contributor Author

This has been punted from 3.0, and will be done as a breaking change later.

@dcharkes dcharkes removed this from the Dart 3 beta 3 milestone Mar 30, 2023
copybara-service bot pushed a commit that referenced this issue Mar 31, 2023
@dcharkes
[vm] Class modifiers for NativeFieldWrapper subtypes
3de5d25 
Split off https://dart-review.googlesource.com/c/sdk/+/291761.
Landing separately, so that the breaking change itself is a smaller
CL.

TEST=ci build (see touched _test files)

bug: #51896
Change-Id: Ic8d218845ccb6a277689e911b2c34c44639e13cf
Cq-Include-Trybots: luci.dart.try:flutter-linux-try,vm-aot-linux-debug-x64c-try,vm-ffi-android-debug-arm-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/292000
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Daco Harkes <dacoharkes@google.com>
copybara-service bot pushed a commit that referenced this issue Apr 3, 2023
@mkustermann
[vm] Allow sending objects across isolates that implement (but not ex…
6e252d9 
…tend) [NativeFieldWrapperClass]

There are currently users that implement classes (e.g. for mocking
purposes) that implement/extend NativeFieldWrappers and send such
objects across isolates.

This CL ensures such mocks can be sent across isolates again (which was
disabled in breaking change in [0]) as long as mocks do not extend NFW
(and therefore have actual native fields).

In the future the [NativeFieldWrapperClass]s will be marked as `base`
and therefore prevent implementing them, see issue [1].
=> At this point this CL can be reverted again.

[0] https://dart-review.googlesource.com/c/sdk/+/289027
[1] #51896

TEST=adjusted isolate tests

Change-Id: I5fe1110e7046761e3606927063d3ad5e2b4bf4e0
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/292320
Reviewed-by: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Alexander Aprelev <aam@google.com>
@dcharkes dcharkes mentioned this issue Apr 5, 2023
Closed
@itsjustkevin itsjustkevin added this to the Dart 3 cleanup milestone Apr 5, 2023
@dcharkes
Copy link
Contributor Author

Now that Flutter has migrated flutter/flutter#123756, we should land the breaking change https://dart-review.googlesource.com/c/sdk/+/291761.

cc @itsjustkevin for breaking change process.

@dcharkes dcharkes added the breaking-change-request This tracks requests for feedback on breaking changes label Oct 12, 2023
@itsjustkevin
Copy link
Contributor

CC: @MaryaBelanger as this did not use the breaking change template and may need more information.

@vsmenon @grouma @Hixie for breaking change approval.

@dcharkes
Copy link
Contributor Author

dcharkes commented Oct 12, 2023 •
edited

Intended change in behavior

Mark NativeWrapperClasses as base.

This was punted from 3.0, because Flutter couldn't migrate in time.

Justification for making the change

Implementing subtypes can lead to issues when passing such native wrapper to a native call, as it will try to unwrap a native field that doesn't exist (likely leading to a segfault).

Expected impact

This change is likely to impact a very small amount of code. (Flutter is already migrated.)

Steps for mitigating the change

Users need to go and mark subclasses of NativeWrapperClasses as base.
Implementing is not supported and would have crashed at runtime if the native field would have been dereferenced.

@dcharkes dcharkes changed the title [vm] Add class modifiers to dart:nativewrappers [breaking change][vm] Add class modifiers to dart:nativewrappers Oct 12, 2023
@dcharkes
Copy link
Contributor Author

@vsmenon @grouma @Hixie for breaking change approval.

Friendly ping!

@grouma
Copy link
Member

grouma commented Oct 19, 2023

I don't see any ACX usage so SGTM.

@dcharkes
Copy link
Contributor Author

@vsmenon @Hixie for breaking change approval.

Friendly ping!

@vsmenon
Copy link
Member

vsmenon commented Oct 26, 2023

lgtm

@dcharkes
Copy link
Contributor Author

@Hixie for breaking change approval.

Friendly ping!

Friendly ping.

@itsjustkevin
Copy link
Contributor

Marking this as approved for now. @Hixie if you disagree, please remove the breaking-change-approved label and provide reasoning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dcharkes dcharkes

Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. breaking-change-approved breaking-change-request This tracks requests for feedback on breaking changes vm-native
Projects
None yet
Milestone
Dart 3 cleanup
Development

No branches or pull requests
4 participants
@grouma @vsmenon @dcharkes @itsjustkevin