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

[AppKit] Try to make NSWindow.ReleasedWhenClosed work. Fixes #8606. #17105

Merged
merged 3 commits into from
Dec 22, 2022
Merged

[AppKit] Try to make NSWindow.ReleasedWhenClosed work. Fixes #8606. #17105

merged 3 commits into from
Dec 22, 2022

Conversation

rolfbjarne
Copy link
Member

@rolfbjarne rolfbjarne commented Dec 21, 2022 •
edited

NSWindow.ReleasedWhenClosed is a rather annoying API, because it interferes with
reference counting. In effect it behaves kind of like autoreleasing does, except
that the object (window) is released at a different time (when the window is closed,
as opposed to when the autorelease pool drains).

We've tried to fix this in the past in several ways:

  • Forcefully disable ReleasedWhenClosed in all the constructors, and if it's set
    in the Close method, then add an extra Retain call to offset the imminent extra
    release. The unfortunate side-effect is that we also call Dispose, which might
    be too early (see IsKeyWindow called on disposed window & Releasing window w/wo default delegate #8606).

  • Rewrite the code to correctly override the native 'close' method, so we get the
    supposedly correct semantics (our special Close code is called) when the window
    is closed by Objective-C (for instance when the user hits the red X to close the
    window). This doesn't really solve the previous problem (we're calling Dispose
    too early), and it doesn't work for non-subclassed NSWindows (see [appkit] Allow ObjC calls into NSWindow.Close #8717).

So I'm trying another approach: track the value of ReleasedWhenClosed, and call Retain/Release
when the value switches between true/false. This way we don't need any special logic
in the Close method.

I've also:

  • Marked the ReleasedWhenClosed property as obsolete, and added a DangerousReleasedWhenClosed
    property, to match how we bind the other reference counting methods (retain ->
    DangerousRetain, release -> DangerousRelease, etc.).

  • Added a ReleaseWhenClosed(bool) method, to be called instead of the ReleasedWhenClosed
    property, and this method will call DangerousRetain/DangerousRelease as described
    above when the ReleasedWhenClosed property changes value.

This new tracking behavior is opt-in for now, but will become opt-out in .NET 9,
and hopefully we'll be able to make it the only behavior at some point (in .NET 10
maybe?).

Fixes #8606.
Fixes #8607.

Ref: #8717.

@rolfbjarne
[AppKit] Try to make NSWindow.ReleasedWhenClosed work. Fixes xamarin#…
72a7f59 
…8606.

NSWindow.ReleasedWhenClosed is a rather annoying API, because it interferes with
reference counting. In effect it behaves kind of like autoreleasing does, except
that the object (window) is released at a different time (when the window is closed,
as opposed to when the autorelease pool drains).

We've tried to fix this in the past in several ways:

* Forcefully disable ReleasedWhenClosed in all the constructors, and if it's set
  in the Close method, then add an extra Retain call to offset the imminent extra
  release. The unfortunate side-effect is that we also call Dispose, which might
  be too early (see xamarin#8606).

* Rewrite the code to correctly override the native 'close' method, so we get the
  supposedly correct semantics (our special Close code is called) when the window
  is closed by Objective-C (for instance when the user hits the red X to close the
  window). This doesn't really solve the previous problem (we're calling Dispose
  too early), and it doesn't work for non-subclassed NSWindows (see xamarin#8717).

So I'm trying another approach: track the value of ReleasedWhenClosed, and call Retain/Release
when the value switches between true/false. This way we don't need any special logic
in the Close method.

I've also:

* Marked the ReleasedWhenClosed property as obsolete, and added a DangerousReleasedWhenClosed
  property, to match how we bind the other reference counting methods (retain ->
  DangerousRetain, release -> DangerousRelease, etc.).

* Added a ReleaseWhenClosed(bool) method, to be called instead of the ReleasedWhenClosed
  property, and this method will call DangerousRetain/DangerousRelease as described
  above when the ReleasedWhenClosed property changes value.

This new tracking behavior is opt-in for now, but will become opt-out in .NET 9,
and hopefully we'll be able to make it the only behavior at some point (in .NET 10
maybe?).

Fixes xamarin#8606.

Ref: xamarin#8717.
@rolfbjarne rolfbjarne added the bug If an issue is a bug or a pull request a bug fix label Dec 21, 2022
This was referenced Dec 21, 2022
Closed
Closed
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
spouliot
spouliot approved these changes Dec 21, 2022
View reviewed changes
src/appkit.cs Outdated Show resolved Hide resolved
@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@rolfbjarne rolfbjarne mentioned this pull request Dec 21, 2022
Closed
@vs-mobiletools-engineering-service2
Copy link
Collaborator

✅ API diff for current PR / commit

Legacy Xamarin (No breaking changes)
  • iOS (no change detected)
  • tvOS (no change detected)
  • watchOS (no change detected)
  • macOS: vsdrops gist (No breaking changes)
.NET (No breaking changes)
  • iOS: (empty diff detected)
  • tvOS: (empty diff detected)
  • MacCatalyst: (empty diff detected)
  • macOS: vsdrops gist (No breaking changes)

✅ API diff vs stable

Legacy Xamarin (No breaking changes)
.NET (No breaking changes)
Legacy Xamarin (stable) vs .NET

ℹ️ Generator diff

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

Pipeline on Agent
Hash: 8dae944457c997c28012bd5446b7292522c11efd [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

📚 [PR Build] Artifacts 📚

Artifacts were not provided.

Pipeline on Agent XAMBOT-1172.Monterey'
Hash: 8dae944457c997c28012bd5446b7292522c11efd [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻

✅ All tests on macOS M1 - Mac Big Sur (11.5) passed.

Pipeline on Agent
Hash: 8dae944457c997c28012bd5446b7292522c11efd [PR build]

@vs-mobiletools-engineering-service2

This comment has been minimized.

Sign in to view
@vs-mobiletools-engineering-service2
Copy link
Collaborator

🚀 [CI Build] Test results 🚀

Test results

✅ All tests passed on VSTS: simulator tests.

🎉 All 223 tests passed 🎉

Tests counts

✅ bcl: All 69 tests passed. Html Report (VSDrops) Download
✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests: All 1 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 7 tests passed. [attempt 2] Html Report (VSDrops) Download
✅ framework: All 8 tests passed. Html Report (VSDrops) Download
✅ generator: All 2 tests passed. Html Report (VSDrops) Download
✅ interdependent_binding_projects: All 7 tests passed. Html Report (VSDrops) Download
✅ install_source: All 1 tests passed. Html Report (VSDrops) Download
✅ introspection: All 8 tests passed. [attempt 2] Html Report (VSDrops) Download
✅ linker: All 65 tests passed. Html Report (VSDrops) Download
✅ mac_binding_project: All 1 tests passed. Html Report (VSDrops) Download
✅ mmp: All 2 tests passed. Html Report (VSDrops) Download
✅ mononative: All 12 tests passed. Html Report (VSDrops) Download
✅ monotouch: All 23 tests passed. Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
✅ mtouch: All 1 tests passed. Html Report (VSDrops) Download
✅ xammac: All 3 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 8 tests passed. Html Report (VSDrops) Download
✅ xtro: All 2 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: 8dae944457c997c28012bd5446b7292522c11efd [PR build]

@rolfbjarne rolfbjarne merged commit 2268a81 into xamarin:main Dec 22, 2022
@rolfbjarne rolfbjarne deleted the releasedwhenclosed branch December 22, 2022 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dalexsoto dalexsoto dalexsoto approved these changes

@spouliot spouliot spouliot approved these changes

Assignees
No one assigned
Labels
bug If an issue is a bug or a pull request a bug fix
Projects
None yet
Milestone
No milestone
4 participants
@rolfbjarne @vs-mobiletools-engineering-service2 @dalexsoto @spouliot