Bug 217712 – WeakObjCPtr.h is not safe to include in C++ source files

• Component: Web Template Framework
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

WeakObjCPtr.h is not safe to include in C++ source files.

Found by compiling WebKit with upstream clang.  Here's the include path:

In file included from WebKitBuild/Release/DerivedSources/WebKit2/NetworkResourceLoaderMessageReceiver.cpp:27:
In file included from Source/WebKit/NetworkProcess/NetworkResourceLoader.h:31:
In file included from Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:39:
In file included from Source/WebKit/Shared/ApplePay/WebPaymentCoordinatorProxy.h:37:
WebKitBuild/Release/usr/local/include/wtf/WeakObjCPtr.h:89:16: error: use of undeclared identifier 'adoptNS'
        return adoptNS(objc_loadWeakRetained(&m_weakReference));
               ^

<rdar://problem/70250667>

[David Kilzer (:ddkilzer)]

Created attachment 411337 [details]
Patch v1

[David Kilzer (:ddkilzer)]

Comment on attachment 411337 [details]
Patch v1

Well, this is trickier than I thought!

In file included from /Volumes/Data/worker/tvOS-14-Build-EWS/build/WebKitBuild/Release-appletvos/DerivedSources/WebKit2/RemoteObjectRegistryMessageReceiver.cpp:27:
/Volumes/Data/worker/tvOS-14-Build-EWS/build/Source/WebKit/Shared/API/Cocoa/RemoteObjectRegistry.h:71:5: error: no template named 'WeakObjCPtr'

<https://ews-build.webkit.org/#/builders/48/builds/753/steps/7/logs/errors>

[David Kilzer (:ddkilzer)]

Created attachment 411339 [details]
Patch v2

[Alex Christensen]

Why do you want to do this?
I noticed if I just use adoptCF everywhere the ObjC-only static_assert in it fails.  That makes me suspicious of this.  Also, objc_loadWeakRetained is only available in objc.

[David Kilzer (:ddkilzer)]

(In reply to Alex Christensen from comment #4)
> Why do you want to do this?

It makes WeakObjCPtr.h safe to include in C++ source files.  That seems to be the rule we want to use for headers--make them safe to include anywhere so you don't have to create special header break-out sections like this:

#ifdef __OBJC__
#include <wtf/WeakObjCPtr.h>
#endif

> I noticed if I just use adoptCF everywhere the ObjC-only static_assert in it
> fails.  That makes me suspicious of this.

What do you mean by "if I just use adoptCF everywhere"?  Do you mean everywhere in WeakObjCPtr.h (one place), or globally on all source files?  (Maybe this isn't important, though.)

> Also, objc_loadWeakRetained is
> only available in objc.

This seemed like a good idea initially, but you're right, it doesn't really hold up.

A couple other ideas here:

1. Add back the #ifdef __OBJC__/#endif guards to WeakObjCPtr.h, then change the C++ source files to Objective-C++.

2. Move the implementation of ::get() to a source file so adoptNS() doesn't prevent WeakObjCPtr from being used in plain C++ sources, although that doesn't address the use of objc_loadWeakRetained() only being available in the ObjC runtime.  (I guess I'm not sure of a scenario where this would ever actually matter on Apple platforms, though.)

[Alex Christensen]

(In reply to David Kilzer (:ddkilzer) from comment #5)
> (In reply to Alex Christensen from comment #4)
> > Why do you want to do this?
> 
> It makes WeakObjCPtr.h safe to include in C++ source files.  That seems to
> be the rule we want to use for headers--make them safe to include anywhere
> so you don't have to create special header break-out sections like this:
> 
> #ifdef __OBJC__
> #include <wtf/WeakObjCPtr.h>
> #endif
> 
> > I noticed if I just use adoptCF everywhere the ObjC-only static_assert in it
> > fails.  That makes me suspicious of this.
> 
> What do you mean by "if I just use adoptCF everywhere"?  Do you mean
> everywhere in WeakObjCPtr.h (one place), or globally on all source files? 
> (Maybe this isn't important, though.)
I tried just replacing the adoptNS with adoptCF.  It didn't compile.

> 2. Move the implementation of ::get() to a source file so adoptNS() doesn't
> prevent WeakObjCPtr from being used in plain C++ sources, although that
> doesn't address the use of objc_loadWeakRetained() only being available in
> the ObjC runtime.  (I guess I'm not sure of a scenario where this would ever
> actually matter on Apple platforms, though.)
Or we could just make WeakObjCPtr::get only available in ObjC.

[Darin Adler]

I think the trick might be to move function bodies out of the template, and then leave various functions unimplemented in C++, but still defined. We’ll get linker errors if we accidentally use them.

[Darin Adler]

There are two differences between adoptNS and adoptCF:

1) adoptNS expects Objective-C pointers and not CF types, and tries to check that at runtime

2) adoptNS does the right thing under ARC for Objective-C objects, which is *not* the same thing that adoptCF should do under ARC. In non-ARC they are doing the same thing (or at least an equivalent thing), just on different types.

[Darin Adler]

(In reply to David Kilzer (:ddkilzer) from comment #5)
> 2. Move the implementation of ::get() to a source file so adoptNS() doesn't
> prevent WeakObjCPtr from being used in plain C++ sources, although that
> doesn't address the use of objc_loadWeakRetained() only being available in
> the ObjC runtime.  (I guess I'm not sure of a scenario where this would ever
> actually matter on Apple platforms, though.)

Seems that something like this would work. Our goal should be to still have this inlined in Objective-C++ files, but in C++ it would probably be OK to call a non-inline function each time.

There’s no real issue with the function being "not available"; I suppose it’s just that it can’t be correctly *compiled* in a C++ file, but can be called by C++ indirectly through an Objective-C++ file.

Also likely would be OK if you got a linker error if you tried to call it in a C++ file. I don’t think that would be a practical problem.

[David Kilzer (:ddkilzer)]

Created attachment 411463 [details]
Patch v3

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #10)
> Created attachment 411463 [details]
> Patch v3

Took Alex+Darin's suggestions and moved the implementation of ::get() out of the class (leaving the definition intact), but then protecting the implementation of ::get() with #ifdef __OBJC__/#endif.

It occurs to me that we might be able to make a WeakRetainPtr<> class that would work for CFTypes by casting them to `id` behind the scenes, but there's no need for that now, and it's out of scope for this (hopefully simple) fix.

[EWS]

Committed r268543: <https://trac.webkit.org/changeset/268543>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411463 [details].