Skip to content

Fix memory leak in NSAttributedString #5256

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix memory leak in NSAttributedString #5256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Sources/CoreFoundation/CFRunArray.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,14 @@ CFRunArrayRef CFRunArrayCreate(CFAllocatorRef allocator) {
return array;
}

CFRunArrayRef CFRunArrayRetain(CFRunArrayRef array) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is CFRunArray not a regular CF type? (CFRetain, CFRelease should work)

Copy link
Contributor

Choose a reason for hiding this comment

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

The COPY below is just a typedef to CFRetain:

#define COPY(obj) CFRetain(obj)

So yeah I think we can just call CFRetain/CFRelease from NSAttributedString.swift (unless there's an issue with how it's imported by Swift?) and that should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I try to use CFRetain, I get the following error:

'CFRetain' is unavailable: Core Foundation objects are automatically memory managed

Copy link
Contributor

Choose a reason for hiding this comment

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

So if it’s automatically memory managed, where is the leak coming from? We don’t have to do this with other CF types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My guess is that this is simply not true for non-Darwin platforms.
I’m not sure how this "automatically memory managed" behavior is supposed to work there.
Are there any tests for this?

return COPY(array);
}

void CFRunArrayRelease(CFRunArrayRef array) {
FREE(array);
}

CFIndex CFRunArrayGetCount(CFRunArrayRef array) {
return array->guts->length;
}
Expand Down
2 changes: 2 additions & 0 deletions Sources/CoreFoundation/include/CFRunArray.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ Returns the type identifier of all CFAttributedString instances.
CF_EXPORT CFTypeID CFRunArrayGetTypeID(void);

CF_EXPORT CFRunArrayRef CFRunArrayCreate(CFAllocatorRef allocator);
CF_EXPORT CFRunArrayRef CFRunArrayRetain(CFRunArrayRef array);
CF_EXPORT void CFRunArrayRelease(CFRunArrayRef array);

CF_EXPORT CFIndex CFRunArrayGetCount(CFRunArrayRef array);
CF_EXPORT CFTypeRef CFRunArrayGetValueAtIndex(CFRunArrayRef array, CFIndex loc, CFRange *effectiveRange, CFIndex *runArrayIndexPtr);
Expand Down
9 changes: 7 additions & 2 deletions Sources/Foundation/NSAttributedString.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ open class NSAttributedString: NSObject, NSCopying, NSMutableCopying, NSSecureCo

// use the resulting _string and _attributeArray to initialize a new instance, just like init
_string = mutableAttributedString._string
_attributeArray = mutableAttributedString._attributeArray
_attributeArray = CFRunArrayRetain(mutableAttributedString._attributeArray)
}

open func encode(with aCoder: NSCoder) {
Expand Down Expand Up @@ -223,7 +223,12 @@ open class NSAttributedString: NSObject, NSCopying, NSMutableCopying, NSSecureCo

// use the resulting _string and _attributeArray to initialize a new instance
_string = mutableAttributedString._string
_attributeArray = mutableAttributedString._attributeArray
_attributeArray = CFRunArrayRetain(mutableAttributedString._attributeArray)
}

deinit {
// Release the CFRunArray created in init methods
CFRunArrayRelease(_attributeArray)
}

/// Executes the block for each attribute in the range.
Expand Down
36 changes: 36 additions & 0 deletions Tests/Foundation/TestNSAttributedString.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,42 @@ class TestNSAttributedString : XCTestCase {
XCTAssertEqual(string, unarchived, "Object loaded from \(variant) didn't match fixture.")
}
}

func test_attributeValueDeallocation() throws {

class AttributeValueTracker {
let id: UUID
let deinitExpectation: XCTestExpectation

init(expectation: XCTestExpectation) {
self.id = UUID()
self.deinitExpectation = expectation
}

deinit {
deinitExpectation.fulfill()
}
}

let deinitExpectation = self.expectation(description: "AttributeValueTracker should be deallocated")
var trackedObject: AttributeValueTracker? = AttributeValueTracker(expectation: deinitExpectation)
weak var weakTrackedObject = trackedObject

// Use a 'do' block for scoping instead of 'autoreleasepool'
// This ensures 'attributedString' goes out of scope at the end of the block
do {
let strongTrackedObject = try XCTUnwrap(trackedObject)
let attributes: [NSAttributedString.Key: Any] = [
NSAttributedString.Key(rawValue: "KEY"): strongTrackedObject
]
let attributedString = NSAttributedString(string: "Test string for lifecycle", attributes: attributes)
_ = attributedString.length
} // <-- attributedString scope ends here, ARC should release it

trackedObject = nil
wait(for: [deinitExpectation], timeout: 1.0)
XCTAssertNil(weakTrackedObject, "Weak reference should be nil after object deallocation.")
}
}

fileprivate extension TestNSAttributedString {
Expand Down