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

[Foundation] Don't dispose tagged pointers. Fixes #21425. #21446

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rolfbjarne
Copy link
Member

@rolfbjarne rolfbjarne commented Oct 15, 2024

Objective-C has the concept of a tagged pointer, which is not really a pointer, but instead a pointer-like value that has encoded the constant value inside itself.

More information about tagged pointers can be found online:

The important part for us is that there's no need for reference counting tagged pointers - they're constant values, and there's nothing allocated, nor does anything need to be freed either. Another point is that these tagged pointers can pop up at any time from Objective-C, and that causes a problem:

  • A tagged pointer is surfaced, say for NSString, and we create a managed NSString instance around it.
  • We dispose the managed NSString instance, and disconnect the managed instance from the underlying (tagged) pointer value.
  • The tagged pointer pops up again, and we find the managed NSString instance in our dictionary that maps native pointers to managed instances.
  • An ObjectDisposedException is thrown when using the managed NSString instance.

This PR attempts to fix this by detecting a tagged pointer, and in that case we don't disconnect the managed instance from the native pointer, because the native pointer will never point to freed memory (it'll always be a valid object).

The unfortunate part is that detecting a tagged pointer is not possible with public API, we have to look at the actual bits, and this is considered an internal implementation detail by Apple.

To make this as safe as possible:

  • This feature is only enabled by default in .NET 10+.
  • It's only enabled by default if the SupportedOSPlatformVersion is iOS 14+ (because earlier versions of iOS used a different tagged pointer format - see previous wwdc video link).
  • It's possible to disable it by setting an MSBuild property.

TODO:

  • Detect SupportedOSPlatformVersion and disable if too early.

Fixes #21425.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@@ -573,6 +573,9 @@
<!-- Set default ValidateObjectPointers value -->
<_ValidateObjectPointers Condition="'$(_ValidateObjectPointers)' == ''">false</_ValidateObjectPointers>

<!-- Set default DisposeTaggedPointers value -->
<_DisposeTaggedPointers Condition="'$(_DisposeTaggedPointers)' == ''">true</_DisposeTaggedPointers>
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the default supposed to be false? How does the test pass? (Or, am I just misreading it?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this was just a quick test run on CI to see if doing it this way would break any test, the PR isn't complete at all.

Copy link
Contributor

⚠️ Your code has been reformatted. ⚠️

If this is not desired, add the actions-disable-autoformat label, and revert the reformatting commit.

If files unrelated to your change were modified, try reverting the reformatting commit + merging with the target branch (and push those changes).

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [CI Build] Windows Integration Tests passed 💻

All Windows Integration Tests passed.

Pipeline on Agent
Hash: 0430fcf54a5bb0299c875d62671566d89ad36737 [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

✅ API diff for current PR / commit

.NET (No breaking changes)

✅ API diff vs stable

.NET (No breaking changes)

ℹ️ Generator diff

Generator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes)

Pipeline on Agent
Hash: 0430fcf54a5bb0299c875d62671566d89ad36737 [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [CI Build] Tests on macOS X64 - Mac Sonoma (14) passed 💻

All tests on macOS X64 - Mac Sonoma (14) passed.

Pipeline on Agent
Hash: 0430fcf54a5bb0299c875d62671566d89ad36737 [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [CI Build] Tests on macOS M1 - Mac Monterey (12) passed 💻

All tests on macOS M1 - Mac Monterey (12) passed.

Pipeline on Agent
Hash: 0430fcf54a5bb0299c875d62671566d89ad36737 [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [CI Build] Tests on macOS M1 - Mac Ventura (13) passed 💻

All tests on macOS M1 - Mac Ventura (13) passed.

Pipeline on Agent
Hash: 0430fcf54a5bb0299c875d62671566d89ad36737 [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

🚀 [CI Build] Test results 🚀

Test results

✅ All tests passed on VSTS: test results.

🎉 All 99 tests passed 🎉

Tests counts

✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (iOS): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (MacCatalyst): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (macOS): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (Multiple platforms): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (tvOS): All 1 tests passed. Html Report (VSDrops) Download
✅ framework: All 2 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 4 tests passed. Html Report (VSDrops) Download
✅ generator: All 1 tests passed. Html Report (VSDrops) Download
✅ interdependent-binding-projects: All 4 tests passed. Html Report (VSDrops) Download
✅ introspection: All 4 tests passed. Html Report (VSDrops) Download
✅ linker: All 40 tests passed. Html Report (VSDrops) Download
✅ monotouch (iOS): All 7 tests passed. Html Report (VSDrops) Download
✅ monotouch (MacCatalyst): All 8 tests passed. Html Report (VSDrops) Download
✅ monotouch (macOS): All 9 tests passed. Html Report (VSDrops) Download
✅ monotouch (tvOS): All 7 tests passed. Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 4 tests passed. Html Report (VSDrops) Download
✅ xtro: All 1 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: 0430fcf54a5bb0299c875d62671566d89ad36737 [PR build]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disposing immutable objects can lead to unpredictable behavior when Objective-C runtime does object pooling
4 participants