Cleanup lifetime management around NSData.bytes. #2994
Conversation
|
@swift-ci test |
|
Could you apply the patch in https://gist.github.com/spevans/c2c6a15b4cae1c376564ea434bda3eea |
Lukasa
force-pushed
the
cb-lifetime-management-in-nsstring
branch
from
9487a49
to
3633901
Compare
|
@swift-ci please test |
Lukasa
force-pushed
the
cb-lifetime-management-in-nsstring
branch
from
1a47cdd
to
b801a26
Compare
|
@swift-ci please test |
|
@spevans So it looks like we're down to simple pre-existing test failures in XML. I assume we don't have any particular reason to think those XML failures are related to this code. Are you happy to merge as-is or do we need to fix that failure too? |
|
Correct, the XML code does have long standing underlying memory issues. Could you apply this patch to yours to disable that XML test: https://gist.github.com/spevans/a7306641613a6531b1e977f321ea7bbb And then we can retest and merge. I testest your PR with that patch and it passes on Ubuntus16,18 & 20 I think we should also backport your fixes to 5.4 (without the XML test disable) |
|
I don't think this is a memory issue. Memory issues would be more unstable than this. It seems like a logical issue. |
|
Specifically, it seems like the CFXML function |
Several blocks of code in Foundation use the .bytes member on NSData to construct an interior pointer. However, NSData can potentially be freed very early, and in newer Swift releases is more likely to do so, leading to this pointer dangling. Similar problems exist around code that aggressively passes ARC-ed objects through unmanaged pointers. This patch attempts a minimal change by forcing the offending objects to live longer using withExtendedLifetime. This should resolve the misuse fairly effectively. It does not address cases where the code could potentially be entirely rewritten to be more efficient or more sensible, because I wasn't here to engage in a complete refactor.
Lukasa
force-pushed
the
cb-lifetime-management-in-nsstring
branch
from
b801a26
to
a8d300d
Compare
|
@swift-ci please test |
|
@swift-ci test |
|
@swift-ci test linux |
| element.removeAttribute(forName: "ns1:name") | ||
| XCTAssertNil(element.attribute(forLocalName: "name", uri: uriNs1), "ns1:name removed") | ||
|
|
||
| withExtendedLifetime(root) { |
There was a problem hiding this comment.
If you don't want to indent a bunch of code, you can do
defer { withExtendedLifetime(root) {} }
|
@swift-ci test linux |
1 similar comment
|
@swift-ci test linux |
|
I believe there are still some further fixes that will be required given the number of uses of |
Several blocks of code in Foundation use the .bytes member on NSData to
construct an interior pointer. However, NSData can potentially be freed
very early, and in newer Swift releases is more likely to do so, leading
to this pointer dangling.
This patch attempts a minimal change by forcing the NSData object to
live longer using withExtendedLifetime. This should resolve the misuse
fairly effectively. It does not address cases where the code could
potentially be entirely rewritten to be more efficient or more sensible,
because I wasn't here to engage in a complete refactor.