WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
Patch v2
(1.54 KB, patch)
2020-10-14 09:57 PDT
,
David Kilzer (:ddkilzer)
no flags
Details
Formatted Diff
Diff
Patch v3
(2.13 KB, patch)
2020-10-15 11:08 PDT
,
David Kilzer (:ddkilzer)
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug