| Summary: | Add tests for WTF::OSObjectPtr/adoptOSObject in Cocoa with and without ARC | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||
| Component: | Web Template Framework | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, darin, ews-watchlist, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=231251 https://bugs.webkit.org/show_bug.cgi?id=231817 |
||||||||
| Attachments: |
|
||||||||
|
Description
David Kilzer (:ddkilzer)
2021-10-15 10:53:39 PDT
(In reply to David Kilzer (:ddkilzer) from comment #0) > I suspect that WTF::OSObjectPtr/adoptOSObject probably leaks objects under > ARC (and there is at least one using of it in an Objective-C++ source file > in WTF), but I can't quite nail down a failing test. (I think the test may > be flakey based on whether MRR or ARC code is kept during linking.) Actually, because WTF::OSObjectPtr uses the correct type to store the object (and doesn't type-pun to CFTypeRef), we might not leak. The only issue may be some ref-churn when using adoptOSObject(). And doing this: #define adoptOSObject adoptOSObjectArc Created attachment 441418 [details]
Patch v1
Comment on attachment 441418 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=441418&action=review NOTE: Need to test Debug builds before marking cq+. > Tools/TestWebKitAPI/Tests/WTF/darwin/OSObjectPtr.cpp:37 > +#ifdef __OBJC__ > +#define bridge_cf_cast(object) ((__bridge CFTypeRef)object) > +#else > +#define bridge_cf_cast(object) ((CFTypeRef)object) > +#endif Need to check whether this is strictly necessary below. (In reply to David Kilzer (:ddkilzer) from comment #1) > because WTF::OSObjectPtr uses the correct type to store the object > (and doesn't type-pun to CFTypeRef), we might not leak. Need to test all 3 cases: Objective-C++ with ARC on Objective-C++ with ARC off C++ where we can do things like copy, move, or destroy the RetainPtr but can’t actually use the pointer beyond that (In reply to Darin Adler from comment #5) > (In reply to David Kilzer (:ddkilzer) from comment #1) > > because WTF::OSObjectPtr uses the correct type to store the object > > (and doesn't type-pun to CFTypeRef), we might not leak. > > Need to test all 3 cases: > > Objective-C++ with ARC on > Objective-C++ with ARC off > C++ where we can do things like copy, move, or destroy the RetainPtr but > can’t actually use the pointer beyond that Right. This patch does that. Am I missing something? Tools/TestWebKitAPI/Tests/WTF/darwin/OSObjectPtr.cpp Tools/TestWebKitAPI/Tests/WTF/darwin/OSObjectPtrCocoa.mm Tools/TestWebKitAPI/Tests/WTF/darwin/OSObjectPtrCocoaARC.mm (In reply to David Kilzer (:ddkilzer) from comment #6) > Right. This patch does that. Great; hadn’t looked at the patch yet. Another concern I had (that's more of a thought experiment than a writing-a-test experiment) is that the code for WTF::OSObjectPtr<T> in C/C++ and non-ARC Cocoa will behave _differently_ (but have the same signatures as) WTF::OSObjectPtr<T> in ARC-enabled Cocoa. I'm not sure how to fix that, unless we ban the use of WTF::OSObjectPtr<T> in Cocoa (or in ARC-enabled Cocoa) and use RetainPtr<> instead (which should work perfectly fine). (In reply to David Kilzer (:ddkilzer) from comment #8) > Another concern I had (that's more of a thought experiment than a > writing-a-test experiment) .... And I say this because the bad behavior occurs at link time, but the linker is non-deterministic in that it can choose either the ARC implementation or a non-ARC implementation to keep, so these would show up as flakey tests. So I think it's better to think through whether this is an issue or not. Comment on attachment 441418 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=441418&action=review > Source/WTF/wtf/OSObjectPtr.h:42 > +template<typename T> OSObjectPtr<T> adoptOSObject(T) WARN_UNUSED_RETURN; Not sure how I feel about adding this. Typically we use WARN_UNUSED_RETURN when it’s a programming error to not use the return value. Especially when it’s a particularly dangerous one. We haven’t done this for adoptRef, although we have done for adoptCF and adoptNS. A little funny to use it on the much less used functions and not on the nearly-ubiquitous adoptRef. And to be super-pedantic, I don’t think it is guaranteed to be an error. For example, if a function has a side effect and also returns an allocated object, you might need to call adoptOSObject on it and it would be OK to not use the result, and let it get destroyed. Another way to write the release. Anyway, I suppose I am OK with it for now. > Tools/TestWebKitAPI/Tests/WTF/darwin/OSObjectPtr.cpp:66 > + EXPECT_EQ(1, CFGetRetainCount((CFTypeRef)fooPtr)); In non-ARC cases, doesn’t this test leak a dispatch queue? Doesn’t seem like a new issue, but also doesn't seem great. Seems like we’d fix it by adding another WTF::releaseOSObject(foo). Comment on attachment 441418 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=441418&action=review >> Source/WTF/wtf/OSObjectPtr.h:42 >> +template<typename T> OSObjectPtr<T> adoptOSObject(T) WARN_UNUSED_RETURN; > > Not sure how I feel about adding this. > > Typically we use WARN_UNUSED_RETURN when it’s a programming error to not use the return value. Especially when it’s a particularly dangerous one. We haven’t done this for adoptRef, although we have done for adoptCF and adoptNS. A little funny to use it on the much less used functions and not on the nearly-ubiquitous adoptRef. > > And to be super-pedantic, I don’t think it is guaranteed to be an error. For example, if a function has a side effect and also returns an allocated object, you might need to call adoptOSObject on it and it would be OK to not use the result, and let it get destroyed. Another way to write the release. > > Anyway, I suppose I am OK with it for now. > you might need to call adoptOSObject on it and it would be OK to not use the result, and let it get destroyed. I'll keep this in mind, but this pattern doesn't seem to be used in WebKit at the moment. >> Tools/TestWebKitAPI/Tests/WTF/darwin/OSObjectPtr.cpp:37 >> +#endif > > Need to check whether this is strictly necessary below. This wasn't needed so I removed it, but for Debug builds, some @autoreleasepool { } blocks are needed, just like TypeCastsCocoa.mm. I'll post a follow-up patch. >> Tools/TestWebKitAPI/Tests/WTF/darwin/OSObjectPtr.cpp:66 >> + EXPECT_EQ(1, CFGetRetainCount((CFTypeRef)fooPtr)); > > In non-ARC cases, doesn’t this test leak a dispatch queue? Doesn’t seem like a new issue, but also doesn't seem great. > > Seems like we’d fix it by adding another WTF::releaseOSObject(foo). Good catch! Will fix. Created attachment 441493 [details]
Patch for landing
Comment on attachment 441493 [details]
Patch for landing
Adding cq+ since jsc-armv7-tests are not impacted (and aren't done yet).
Committed r284340 (243134@main): <https://commits.webkit.org/243134@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 441493 [details]. |