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.

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

[release/8.0-staging] [mono][interp] Resolve virtual method on delegates created by compiled code #101290

Merged

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Apr 19, 2024

Backport of #101168 to release/8.0-staging

/cc @simonrozsival @BrzVlad

Customer Impact

  • Customer reported
  • Found internally

This issue caused customer's MacCatalyst app to crash. Because of this issue, delegates are potentially broken when mixing aot and interpreted code. Delegates created in aot-code that contain a virtual method on a target object are not invoked properly in the interpreter because we don't resolve the virtual method on the object.

Regression

  • Yes
  • No

Testing

Manual testing. This change only affects calls from code that is compiled with Mono AOT and code that is using the Mono Interpreter. This is a configuration that is sometimes used in iOS and Mac Catalyst apps and it is hard to test with automated tests. The fix was verified in the context of a Mac Catalyst repro app (#101074).

Risk

Low

…d code

Creating a delegate would normally end up calling into the runtime via ves_icall_mono_delegate_ctor. However, jit/aot backand have a fastpath where the delegate is not fully initialized (relying on the delegate trampoline to resolve the actual method to be called when the delegate is first called). Interp delegate initialization therefore doesn't take place. If this is the case and the delegate method is virtual, we would need to resolve it based on the target object.
Copy link
Contributor

Tagging subscribers to this area: @BrzVlad, @kotlarmilos
See info in area-owners.md if you want to be subscribed.

@vitek-karas
Copy link
Member

@simonrozsival could you please fill in the PR description?
@BrzVlad please review the changes (just to make sure they got ported over correctly)

@carlossanlop
Copy link
Member

@vitek-karas the template was filled out and Vlad reviewed it. If this is ready, please send the email to Tactics requesting approval and add the servicing-consider label.

@simonrozsival simonrozsival added the Servicing-consider Issue for next servicing release review label Apr 30, 2024
@vitek-karas vitek-karas added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Apr 30, 2024
@vitek-karas
Copy link
Member

Approved over email.

@simonrozsival simonrozsival merged commit a0411df into release/8.0-staging May 2, 2024
108 of 116 checks passed
@jkotas jkotas deleted the backport/pr-101168-to-release/8.0-staging branch May 6, 2024 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants