RESOLVED FIXED 217712
WeakObjCPtr.h is not safe to include in C++ source files
https://bugs.webkit.org/show_bug.cgi?id=217712
Summary WeakObjCPtr.h is not safe to include in C++ source files
David Kilzer (:ddkilzer)
Reported 2020-10-14 09:37:16 PDT
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>
Attachments
Patch v1 (1.40 KB, patch)
2020-10-14 09:47 PDT, David Kilzer (:ddkilzer)
ews-feeder: commit-queue-
Patch v2 (1.54 KB, patch)
2020-10-14 09:57 PDT, David Kilzer (:ddkilzer)
no flags
Patch v3 (2.13 KB, patch)
2020-10-15 11:08 PDT, David Kilzer (:ddkilzer)
no flags
David Kilzer (:ddkilzer)
Comment 1 2020-10-14 09:47:35 PDT
Created attachment 411337 [details] Patch v1
David Kilzer (:ddkilzer)
Comment 2 2020-10-14 09:52:17 PDT
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)
Comment 3 2020-10-14 09:57:07 PDT
Created attachment 411339 [details] Patch v2
Alex Christensen
Comment 4 2020-10-14 15:06:33 PDT
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)
Comment 5 2020-10-14 16:41:41 PDT
(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
Comment 6 2020-10-14 16:44:07 PDT
(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
Comment 7 2020-10-14 18:02:01 PDT
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
Comment 8 2020-10-14 18:03:44 PDT
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
Comment 9 2020-10-14 18:11:57 PDT
(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)
Comment 10 2020-10-15 11:08:40 PDT
Created attachment 411463 [details] Patch v3
David Kilzer (:ddkilzer)
Comment 11 2020-10-15 11:12:14 PDT
(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
Comment 12 2020-10-15 12:12:07 PDT
Committed r268543: <https://trac.webkit.org/changeset/268543> All reviewed patches have been landed. Closing bug and clearing flags on attachment 411463 [details].
Note You need to log in before you can comment on or make changes to this bug.