Bug 231819 – Add tests for WTF::OSObjectPtr/adoptOSObject in Cocoa with and without ARC

• Component: Web Template Framework
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Add tests for WTF::OSObjectPtr/adoptOSObject in Cocoa with and without ARC.

While fixing Bug 231817, I decided to add tests for WTF::OSObjectPtr/adoptOSObject in Cocoa sources with and without ARC.

The interesting thing about the xpc_object_t type is that it behaves like a CFTypeRef in C/C++ code, like a CFTypeRef/NSObject in Cocoa without ARC (MRR), and like an NSObject in Cocoa with ARC (since ARC manages the lifetime of all xpc_object_t objects).

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.)

[David Kilzer (:ddkilzer)]

(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

[Radar WebKit Bug Importer]

<rdar://problem/84312692>

[David Kilzer (:ddkilzer)]

Created attachment 441418 [details]
Patch v1

[David Kilzer (:ddkilzer)]

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.

[Darin Adler]

(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

[David Kilzer (:ddkilzer)]

(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

[Darin Adler]

(In reply to David Kilzer (:ddkilzer) from comment #6)
> Right.  This patch does that.

Great; hadn’t looked at the patch yet.

[David Kilzer (:ddkilzer)]

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).

[David Kilzer (:ddkilzer)]

(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.

[Darin Adler]

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).

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Created attachment 441493 [details]
Patch for landing

[David Kilzer (:ddkilzer)]

Comment on attachment 441493 [details]
Patch for landing

Adding cq+ since jsc-armv7-tests are not impacted (and aren't done yet).

[EWS]

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].

[David Kilzer (:ddkilzer)]

Checking API test results:

<https://results.webkit.org/?suite=api-tests&suite=api-tests&suite=api-tests&suite=api-tests&suite=api-tests&suite=api-tests&suite=api-tests&suite=api-tests&suite=api-tests&test=TestWTF.OSObjectPtr.AdoptOSObject&test=TestWTF.OSObjectPtr.LeakRef&test=TestWTF.OSObjectPtr.RetainRelease&test=TestWTF.OSObjectPtrCocoa.AdoptOSObject&test=TestWTF.OSObjectPtrCocoa.LeakRef&test=TestWTF.OSObjectPtrCocoa.RetainRelease&test=TestWTF.OSObjectPtrCocoaARC.AdoptOSObject&test=TestWTF.OSObjectPtrCocoaARC.LeakRef&test=TestWTF.OSObjectPtrCocoaARC.RetainRelease>