Skip to content

Commit

Permalink
fix memory leaks (#123)
Browse files Browse the repository at this point in the history
* new comptr stuff is building!

* all building...but crashing

* all gucci

* add comments describing ComPtrs design

* remove IUnknown2

* undo unnecessary add

* fixing ref count of comptr

* Revert "undo unnecessary add"

This reverts commit cbabceb.

* working for weak ref, but this might not actually work

* fix event subscriptions

* fix ref count for aggregated objects

* cache interfaces as lazy vars on the classes

* properly hide _inner

* Update swiftwinrt/Resources/Support/ComPtr.swift

Co-authored-by: Jeff <[email protected]>

* Update swiftwinrt/Resources/Support/ComPtr.swift

Co-authored-by: Jeff <[email protected]>

* return identity and don't use ComObject since classes require strong ref to identity

* fix build break

* use spi and some other cleanup so i don't need dubious _getRetainCount call

* add extra test cases

* add deinitializers to ensure proper cleanup

* Update swiftwinrt/Resources/Support/Aggregation.swift

Co-authored-by: Jeff <[email protected]>

---------

Co-authored-by: Jeff <[email protected]>
  • Loading branch information
stevenbrix and jeffdav authored Nov 14, 2023
1 parent 5bd2bfa commit fe63cb9
Show file tree
Hide file tree
Showing 43 changed files with 1,862 additions and 1,539 deletions.
4 changes: 2 additions & 2 deletions .vscode/launch.json
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
{
"configurations": [
{
"type": "lldb",
"type": "swift-lldb",
"request": "launch",
"name": "test_app",
"program": "${workspaceFolder:swiftwinrt}\\out\\${command:cmake.activeBuildPresetName}\\bin\\test_app",
"program": "${workspaceFolder:swiftwinrt}\\out\\${command:cmake.activeBuildPresetName}\\bin\\test_app.exe",
"args": [],
"cwd": "${workspaceFolder:swiftwinrt}/tests",
"preLaunchTask": "Install"
Expand Down
5 changes: 4 additions & 1 deletion .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@
"tuple": "cpp",
"unordered_map": "cpp",
"variant": "cpp",
"xiosbase": "cpp"
"xiosbase": "cpp",
"coroutine": "cpp",
"resumable": "cpp",
"string": "cpp"
}
}
2 changes: 0 additions & 2 deletions CMakePresets.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@
"strategy": "external"
},
"cacheVariables": {
"CMAKE_C_COMPILER": "cl",
"CMAKE_CXX_COMPILER": "cl",
"CMAKE_SYSTEM_VERSION": "10.0.17763.0",
"CMAKE_MODULE_PATH": "${sourceDir}/cmake",
"CMAKE_SKIP_INSTALL_ALL_DEPENDENCY": true
Expand Down
7 changes: 5 additions & 2 deletions swiftwinrt/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
project(swiftwinrt)

set(CMAKE_C_COMPILER cl)
set(CMAKE_CXX_COMPILER cl)

set(SWIFTWINRT_VERSION_STRING "0.0.1")
set(MicrosoftWindowsWinMD_INCLUDE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/winmd/src)

Expand Down Expand Up @@ -30,7 +33,7 @@ string(APPEND CMAKE_SHARED_LINKER_FLAGS_RELEASE " /DEBUG /OPT:REF /OPT:ICF /MAP"
string(APPEND CMAKE_EXE_LINKER_FLAGS_RELEASE " /DEBUG /OPT:REF /OPT:ICF /MAP")

if (CMAKE_CXX_COMPILER MATCHES "clang-cl")
add_compile_options(-Wno-delete-non-virtual-dtor -mcx16 -fno-delayed-template-parsing -Xclang -fcoroutines-ts)
add_compile_options(-Wno-delete-non-virtual-dtor -mcx16 -fno-delayed-template-parsing)
else()
add_compile_options(/permissive- /await)
endif()
Expand All @@ -47,7 +50,7 @@ endif()
add_executable(swiftwinrt "")
target_sources(swiftwinrt PUBLIC
main.cpp
pch.cpp
pch.cpp
metadata_cache.cpp
types.cpp
metadata_filter.cpp
Expand Down
92 changes: 71 additions & 21 deletions swiftwinrt/Resources/Support/Aggregation.swift
Original file line number Diff line number Diff line change
@@ -1,12 +1,52 @@
import C_BINDINGS_MODULE
import Foundation

public protocol ComposableImpl : AbiInterfaceBridge where SwiftABI: IInspectable, SwiftProjection: WinRTClass {
// The WinRTClassWeakReference class is a proxy for properly managing the reference count of
// the WinRTClass that is being aggregated. The aggregated object holds a weak reference to
// the outer IInspectable (swift) that is passed in during construction. In general, the
// swift wrappers we create hold strong references to the objects they are wrapping, as we
// expect an AddRef from WinRT to keep the object alive. Since this doesn't happen for aggregated
// objects, we need a proxy which sits in the middle. The WinRTClassWeakReference object doesn't
// keep a strong ref to the swift object, but it forwards all AddRef/Release calls from WinRT
// to the swift object, to ensure it doesn't get cleaned up. The Swift object in turn holds a strong
// reference to this object so that it stays alive.
@_spi(WinRTInternal)
public final class WinRTClassWeakReference<Class: WinRTClass> {
fileprivate weak var instance: Class?
public init(_ instance: Class){
self.instance = instance
}
}

extension WinRTClassWeakReference: CustomQueryInterface {
@_spi(WinRTImplements)
public func queryInterface(_ iid: SUPPORT_MODULE.IID) -> IUnknownRef? {
guard let instance else { return nil }
return instance.queryInterface(iid)
}
}

extension WinRTClassWeakReference: CustomAddRef {
func addRef() {
guard let instance else { return }
let unmanaged = Unmanaged.passUnretained(instance)
_ = unmanaged.retain()
}

func release() {
guard let instance else { return }
let unmanaged = Unmanaged.passUnretained(instance)
unmanaged.release()
}
}

@_spi(WinRTInternal)
public protocol ComposableImpl<Class> : AbiInterfaceBridge where SwiftABI: IInspectable, SwiftProjection: WinRTClassWeakReference<Class> {
associatedtype Class: WinRTClass
associatedtype Default : AbiInterface where Default.SwiftABI: SUPPORT_MODULE.IInspectable
static func makeAbi() -> CABI
}


// At a high level, aggregation simply requires the WinRT object to have a pointer back to the Swift world, so that it can call
// overridable methods on the class. This Swift pointer is given to the WinRT object during construction. The construction of the
// WinRT object returns us two different pointers:
Expand All @@ -24,11 +64,12 @@ public protocol ComposableImpl : AbiInterfaceBridge where SwiftABI: IInspectable
// |---------------|---------------------------|-------------------------|--------------------------|
// | Yes | self | stored on swift object | ignored or stored |
// | No | nil | ignored | stored on swift object |
@_spi(WinRTInternal)
public func MakeComposed<Composable: ComposableImpl>(
composing: Composable.Type,
_ this: Composable.SwiftProjection,
_ createCallback: (UnsealedWinRTClassWrapper<Composable>?, inout SUPPORT_MODULE.IInspectable?) -> Composable.Default.SwiftABI) -> SUPPORT_MODULE.IInspectable {
let aggregated = type(of: this) != Composable.SwiftProjection.self
_ this: Composable.Class,
_ createCallback: (UnsealedWinRTClassWrapper<Composable>?, inout SUPPORT_MODULE.IInspectable?) -> Composable.Default.SwiftABI) {
let aggregated = type(of: this) != Composable.Class.self
let wrapper:UnsealedWinRTClassWrapper<Composable>? = .init(aggregated ? this : nil)

var innerInsp: SUPPORT_MODULE.IInspectable? = nil
Expand All @@ -37,21 +78,36 @@ public func MakeComposed<Composable: ComposableImpl>(
fatalError("Unexpected nil returned after successful creation")
}

return aggregated ? innerInsp : base
if let wrapper {
this.identity = ComPtr(wrapper.toIInspectableABI { $0 })
// Storing a strong ref to the wrapper adds a ref to ourselves, remove the
// reference
wrapper.swiftObj.release()
}
this._inner = aggregated ? innerInsp : base
}

@_spi(WinRTInternal)
public class UnsealedWinRTClassWrapper<Composable: ComposableImpl> : WinRTAbiBridgeWrapper<Composable> {
override public class var IID: SUPPORT_MODULE.IID { Composable.SwiftABI.IID }
public init?(_ impl: Composable.SwiftProjection?) {
public init?(_ impl: Composable.Class?) {
guard let impl = impl else { return nil }
let abi = Composable.makeAbi()
super.init(abi, impl)
super.init(abi, Composable.SwiftProjection(impl))
}

public static func unwrapFrom(base: ComPtr<Composable.Default.CABI>) -> Composable.Class? {
let overrides: Composable.SwiftABI = try! base.queryInterface()
if let weakRef = tryUnwrapFrom(abi: RawPointer(overrides)) { return weakRef.instance }
guard let instance = makeFrom(abi: overrides) else {
// the derived class doesn't exist, which is fine, just return the type the API specifies.
return make(type: Composable.Class.self, from: overrides)
}
return instance as? Composable.Class
}
public static func unwrapFrom(base: UnsafeMutablePointer<Composable.Default.CABI>) -> Composable.SwiftProjection? {
let baseInsp = SUPPORT_MODULE.IInspectable(consuming: base)
let overrides: Composable.SwiftABI = try! baseInsp.QueryInterface()
return unwrapFrom(abi: RawPointer(overrides))

public static func tryUnwrapFrom(raw pUnk: UnsafeMutableRawPointer?) -> Composable.Class? {
tryUnwrapFromBase(raw: pUnk)?.instance
}

public func toIInspectableABI<ResultType>(_ body: (UnsafeMutablePointer<C_IInspectable>) throws -> ResultType)
Expand All @@ -62,13 +118,7 @@ public class UnsealedWinRTClassWrapper<Composable: ComposableImpl> : WinRTAbiBri
}

public extension ComposableImpl {
static func from(abi: UnsafeMutablePointer<CABI>?) -> SwiftProjection? {
guard let abi else { return nil }
let baseInsp = SUPPORT_MODULE.IInspectable(abi)
guard let instance = makeFrom(abi: baseInsp) else {
// the derived class doesn't exist, which is fine, just return the type the API specifies.
return make(type: Self.SwiftProjection.self, from: baseInsp)
}
return instance as? Self.SwiftProjection
static func from(abi: ComPtr<CABI>?) -> SwiftProjection? {
return nil
}
}
}
126 changes: 126 additions & 0 deletions swiftwinrt/Resources/Support/ComPtr.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
// Copyright © 2023 The Browser Company
// SPDX-License-Identifier: BSD-3
import C_BINDINGS_MODULE

// ComPtr is a smart pointer for COM interfaces. It holds on to the underlying pointer

// and the semantics of it are meant to mirror that of the ComPtr class in WRL. The
// design of ComPtr and ComPtrs.intialize is that there should be no use of UnsafeMutablePointer
// anywhere else in the code base. The only place where UnsafeMutablePointer should be used is

// where it's required at the ABI boundary.
public class ComPtr<CInterface> {
fileprivate var pUnk: UnsafeMutablePointer<CInterface>?

public init(_ ptr: UnsafeMutablePointer<CInterface>) {
self.pUnk = ptr
asIUnknown {
_ = $0.pointee.lpVtbl.pointee.AddRef($0)
}
}

public convenience init?(_ ptr: UnsafeMutablePointer<CInterface>?) {
guard let ptr else { return nil }
self.init(ptr)
}

fileprivate init?(takingOwnership ptr: UnsafeMutablePointer<CInterface>?) {
guard let ptr else { return nil }
self.pUnk = ptr
}

// Release ownership of the underlying pointer and return it. This is
// useful when assigning to an out parameter and avoids an extra Add/Ref
// release call.
public func detach() -> UnsafeMutableRawPointer? {
let result = pUnk
pUnk = nil
return UnsafeMutableRawPointer(result)
}

public func get() -> UnsafeMutablePointer<CInterface> {
guard let pUnk else { preconditionFailure("get() called on nil pointer") }
return pUnk
}

deinit {
release()
}

private func release() {
guard pUnk != nil else { return }
asIUnknown {
_ = $0.pointee.lpVtbl.pointee.Release($0)
}
}

func asIUnknown<ResultType>(_ body: (UnsafeMutablePointer<C_IUnknown>) throws -> ResultType) rethrows -> ResultType {
guard let pUnk else { preconditionFailure("asIUnknown called on nil pointer") }
return try pUnk.withMemoryRebound(to: C_IUnknown.self, capacity: 1) { try body($0) }
}
}

public extension ComPtr {
func queryInterface<Interface: IUnknown>() throws -> Interface {
let ptr = try self.asIUnknown { pUnk in
var iid = Interface.IID
return try ComPtrs.initialize(to: C_IUnknown.self) { result in
try CHECKED(pUnk.pointee.lpVtbl.pointee.QueryInterface(pUnk, &iid, &result))
}
}
return .init(ptr!)
}
}

// ComPtrs properly initializes pointers who have ownership of the underlying raw pointers. This is used at the ABI boundary layer, for example:
// let (return1, return2) = try ComPtrs.initialize { return1Abi, return2Abi in
// try CHECKED(pThis.pointee.lpVtbl.pointee.Method(pThis, &return1Abi, &return2Abi))
// }
public struct ComPtrs {
// Note: The single initialization methods still return a tuple for ease of code generation
public static func initialize<I>(to: I.Type, _ body: (inout UnsafeMutableRawPointer?) throws -> ()) rethrows -> (ComPtr<I>?) {
var ptrRaw: UnsafeMutableRawPointer?
try body(&ptrRaw)
return (ComPtr(takingOwnership: ptrRaw?.assumingMemoryBound(to: I.self)))
}

public static func initialize<I>(_ body: (inout UnsafeMutablePointer<I>?) throws -> ()) rethrows -> (ComPtr<I>?) {
var ptr: UnsafeMutablePointer<I>?
try body(&ptr)
return (ComPtr(takingOwnership: ptr))
}

public static func initialize<I1, I2>(_ body: (inout UnsafeMutablePointer<I1>?, inout UnsafeMutablePointer<I2>?) throws -> ()) rethrows -> (ComPtr<I1>?, ComPtr<I2>?) {
var ptr1: UnsafeMutablePointer<I1>?
var ptr2: UnsafeMutablePointer<I2>?
try body(&ptr1, &ptr2)
return (ComPtr(takingOwnership: ptr1), ComPtr(takingOwnership: ptr2))
}

public static func initialize<I1, I2, I3>(_ body: (inout UnsafeMutablePointer<I1>?, inout UnsafeMutablePointer<I2>?, inout UnsafeMutablePointer<I3>?) throws -> ()) rethrows -> (ComPtr<I1>?, ComPtr<I2>?, ComPtr<I3>?) {
var ptr1: UnsafeMutablePointer<I1>?
var ptr2: UnsafeMutablePointer<I2>?
var ptr3: UnsafeMutablePointer<I3>?
try body(&ptr1, &ptr2, &ptr3)
return (ComPtr(takingOwnership: ptr1), ComPtr(takingOwnership: ptr2), ComPtr(takingOwnership: ptr3))
}

public static func initialize<I1, I2, I3, I4>(_ body: (inout UnsafeMutablePointer<I1>?, inout UnsafeMutablePointer<I2>?, inout UnsafeMutablePointer<I3>?, inout UnsafeMutablePointer<I4>?) throws -> ()) rethrows -> (ComPtr<I1>?, ComPtr<I2>?, ComPtr<I3>?, ComPtr<I4>?) {
var ptr1: UnsafeMutablePointer<I1>?
var ptr2: UnsafeMutablePointer<I2>?
var ptr3: UnsafeMutablePointer<I3>?
var ptr4: UnsafeMutablePointer<I4>?
try body(&ptr1, &ptr2, &ptr3, &ptr4)
return (ComPtr(takingOwnership: ptr1), ComPtr(takingOwnership: ptr2), ComPtr(takingOwnership: ptr3), ComPtr(takingOwnership: ptr4))
}

public static func initialize<I1, I2, I3, I4, I5>(_ body: (inout UnsafeMutablePointer<I1>?, inout UnsafeMutablePointer<I2>?, inout UnsafeMutablePointer<I3>?, inout UnsafeMutablePointer<I4>?, inout UnsafeMutablePointer<I5>?) throws -> ()) rethrows -> (ComPtr<I1>?, ComPtr<I2>?, ComPtr<I3>?, ComPtr<I4>?, ComPtr<I5>?) {
var ptr1: UnsafeMutablePointer<I1>?
var ptr2: UnsafeMutablePointer<I2>?
var ptr3: UnsafeMutablePointer<I3>?
var ptr4: UnsafeMutablePointer<I4>?
var ptr5: UnsafeMutablePointer<I5>?
try body(&ptr1, &ptr2, &ptr3, &ptr4, &ptr5)
return (ComPtr(takingOwnership: ptr1), ComPtr(takingOwnership: ptr2), ComPtr(takingOwnership: ptr3), ComPtr(takingOwnership: ptr4), ComPtr(takingOwnership: ptr5))
}
}
17 changes: 6 additions & 11 deletions swiftwinrt/Resources/Support/CustomQueryInterface.swift
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,15 @@ public protocol CustomQueryInterface {
extension IUnknownRef {
func queryInterface(_ iid: SUPPORT_MODULE.IID) -> IUnknownRef? {
var iid = iid
var result: UnsafeMutableRawPointer?
guard borrow.pointee.lpVtbl.pointee.QueryInterface(borrow, &iid, &result) == S_OK, let result else { return nil }
return IUnknownRef(consuming: result)
let (ptr) = try? ComPtrs.initialize(to: C_IUnknown.self) { result in
try CHECKED(borrow.pointee.lpVtbl.pointee.QueryInterface(borrow, &iid, &result))
}
guard let ptr else { return nil}
return IUnknownRef(ptr)
}
}

@_spi(WinRTInternal)
public func queryInterface(_ obj: AnyWinRTClass, _ iid: SUPPORT_MODULE.IID) -> IUnknownRef? {
public func queryInterface(_ obj: WinRTClass, _ iid: SUPPORT_MODULE.IID) -> IUnknownRef? {
obj._inner.pUnk.queryInterface(iid)
}

extension WinRTClass {
@_spi(WinRTInternal)
public func queryInterface(_ iid: SUPPORT_MODULE.IID) -> IUnknownRef? {
SUPPORT_MODULE.queryInterface(self, iid)
}
}
11 changes: 5 additions & 6 deletions swiftwinrt/Resources/Support/ErrorHandling.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,13 @@

import WinSDK

@_alwaysEmitIntoClient @inline(__always) @discardableResult
public func CHECKED(_ body: () -> HRESULT) throws -> HRESULT {
@_alwaysEmitIntoClient @inline(__always)
public func CHECKED(_ body: () -> HRESULT) throws {
let hr: HRESULT = body()
guard hr >= 0 else { throw Error(hr: hr) }
return hr
}

@_alwaysEmitIntoClient @inline(__always) @discardableResult
public func CHECKED(_ body: @autoclosure () -> HRESULT) throws -> HRESULT {
return try CHECKED(body)
@_alwaysEmitIntoClient @inline(__always)
public func CHECKED(_ body: @autoclosure () -> HRESULT) throws {
try CHECKED(body)
}
Loading

0 comments on commit fe63cb9

Please sign in to comment.