For Bug 177893, we disabled -Wcast-qual in WebCore for Apple ports. This bug tracks re-enabling it by introducing a safe (and readable) way to cast const CFTypeRef pointers to non-const CFTypeRef pointers (sometimes of a different type).
<rdar://problem/34960830>
Created attachment 330270 [details] Patch v1
Attachment 330270 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaSampleAVFObjC.mm:32: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:59: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:106: dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:107: dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 4 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 330271 [details] Patch v2
Attachment 330271 [details] did not pass style-queue: ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:59: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:106: dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:107: dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 3 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 330273 [details] Patch v3
Attachment 330273 [details] did not pass style-queue: ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:59: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:106: dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:107: dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 3 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 330276 [details] Patch v4
Attachment 330276 [details] did not pass style-queue: ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:59: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:106: dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:107: dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 3 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 330282 [details] Patch v5
Attachment 330282 [details] did not pass style-queue: ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:59: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:106: dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:107: dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 3 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 330283 [details] Patch v6
Attachment 330283 [details] did not pass style-queue: ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:63: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:110: dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:111: dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 3 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 330284 [details] Patch v7
Attachment 330284 [details] did not pass style-queue: ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:63: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:110: dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:111: dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 3 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 330286 [details] Patch v8
Attachment 330286 [details] did not pass style-queue: ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:63: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:110: dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:111: dynamic_id_cast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 3 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 330286 [details] Patch v8 Attachment 330286 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5885909 New failing tests: fast/mediastream/MediaStream-MediaElement-setObject-null.html
Created attachment 330292 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 330304 [details] Patch v9
Attachment 330304 [details] did not pass style-queue: ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:59: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 330306 [details] Patch v10
Attachment 330306 [details] did not pass style-queue: ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:59: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 330309 [details] Patch v11
Attachment 330309 [details] did not pass style-queue: ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:59: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 330309 [details] Patch v11 View in context: https://bugs.webkit.org/attachment.cgi?id=330309&action=review > Source/WTF/ChangeLog:40 > + (WTF::checked_cf_const_cast): This behaves the same way as > + WTF::checked_cf_cast does now, but works with CFTypes that are > + not defined as const pointers, including CFMutable types and > + other CFTypes defined outside of CoreFoundation.framework. We > + also add built-in support for CFMutable types in the header by > + defining traits for them. Note that I added WTF::checked_cf_const_cast<> so that non-const CFTypes (the CFMutable* types) had a separate method to use. This makes it harder (in theory) to incorrectly cast to CFArray from CFMutableArray since you must use checked_cf_cast<CFArrayRef>() and checked_cf_const_cast<CFMutableArrayRef>(). In practice, I'm not sure how much this difference matters. I can just add the const_cast<CF_BRIDGED_TYPE(ID) void*>() to WTF::checked_cf_cast<> and the compiler wouldn't complain about the 'const' being removed and then re-added.
(In reply to David Kilzer (:ddkilzer) from comment #26) > Comment on attachment 330309 [details] > Patch v11 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=330309&action=review > > > Source/WTF/ChangeLog:40 > > + (WTF::checked_cf_const_cast): This behaves the same way as > > + WTF::checked_cf_cast does now, but works with CFTypes that are > > + not defined as const pointers, including CFMutable types and > > + other CFTypes defined outside of CoreFoundation.framework. We > > + also add built-in support for CFMutable types in the header by > > + defining traits for them. > > Note that I added WTF::checked_cf_const_cast<> so that non-const CFTypes > (the CFMutable* types) had a separate method to use. > > This makes it harder (in theory) to incorrectly cast to CFArray from > CFMutableArray since you must use checked_cf_cast<CFArrayRef>() and > checked_cf_const_cast<CFMutableArrayRef>(). > > In practice, I'm not sure how much this difference matters. I can just add > the const_cast<CF_BRIDGED_TYPE(ID) void*>() to WTF::checked_cf_cast<> and > the compiler wouldn't complain about the 'const' being removed and then > re-added. And I can check that CFMutable* objects are actually mutable using something like this (which would be a Debug assertion to start with since there is no security implication): inline bool CFArrayIsMutable(CFArrayRef array) { return [(id)array isMemberOfClass:[NSMutableArray class]]; }
Created attachment 330366 [details] Patch v12
Attachment 330366 [details] did not pass style-queue: ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:84: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to David Kilzer (:ddkilzer) from comment #27) > (In reply to David Kilzer (:ddkilzer) from comment #26) > > Comment on attachment 330309 [details] > > Patch v11 > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=330309&action=review > > > > > Source/WTF/ChangeLog:40 > > > + (WTF::checked_cf_const_cast): This behaves the same way as > > > + WTF::checked_cf_cast does now, but works with CFTypes that are > > > + not defined as const pointers, including CFMutable types and > > > + other CFTypes defined outside of CoreFoundation.framework. We > > > + also add built-in support for CFMutable types in the header by > > > + defining traits for them. > > > > Note that I added WTF::checked_cf_const_cast<> so that non-const CFTypes > > (the CFMutable* types) had a separate method to use. > > > > This makes it harder (in theory) to incorrectly cast to CFArray from > > CFMutableArray since you must use checked_cf_cast<CFArrayRef>() and > > checked_cf_const_cast<CFMutableArrayRef>(). > > > > In practice, I'm not sure how much this difference matters. I can just add > > the const_cast<CF_BRIDGED_TYPE(ID) void*>() to WTF::checked_cf_cast<> and > > the compiler wouldn't complain about the 'const' being removed and then > > re-added. > > And I can check that CFMutable* objects are actually mutable using something > like this (which would be a Debug assertion to start with since there is no > security implication): > > inline bool CFArrayIsMutable(CFArrayRef array) > { > return [(id)array isMemberOfClass:[NSMutableArray class]]; > } Actually this won't work, so will need to make more changes.
Created attachment 330394 [details] Patch v13
Attachment 330394 [details] did not pass style-queue: ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:70: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to David Kilzer (:ddkilzer) from comment #31) > Created attachment 330394 [details] > Patch v13 Okay, I'm done iterating on this. It's ready for review.
Comment on attachment 330394 [details] Patch v13 View in context: https://bugs.webkit.org/attachment.cgi?id=330394&action=review Looks good to me. I didn't really understand the RetainPtr changes though so I'll let someone else review. > Source/WTF/ChangeLog:30 > + Release buidls, as well as other "checked" functions. Typo: "buidls" => "builds" > Source/WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:47 > attributesDictionary = adoptCF(CFDictionaryCreateMutable(kCFAllocatorDefault, 4, &kCFCopyStringDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks)); > - CFMutableDictionaryRef mutableAttributes = (CFMutableDictionaryRef)attributesDictionary.get(); > + CFMutableDictionaryRef mutableAttributes = checked_cf_cast<CFMutableDictionaryRef>(attributesDictionary.get()); Here I'd probably just switch to using `attributesDictionary.get()` locally instead of `mutableAttributes`. This is a local we just created on line 46, we shouldn't need to check its type. > Source/WebCore/testing/cocoa/WebArchiveDumpSupport.mm:223 > - RetainPtr<CFMutableDictionaryRef> propertyList = adoptCF((CFMutableDictionaryRef)CFPropertyListCreateWithData(kCFAllocatorDefault, webArchiveData, kCFPropertyListMutableContainersAndLeaves, &format, &error)); > + RetainPtr<CFMutableDictionaryRef> propertyList = adoptCF(checked_cf_cast<CFMutableDictionaryRef>(CFPropertyListCreateWithData(kCFAllocatorDefault, webArchiveData, kCFPropertyListMutableContainersAndLeaves, &format, &error))); For these WebArchive cases, I think that if we got an unexpected type that we would want to bail with an error message instead of crashing (inside a cast check). That said the checked casts are certainly better then previous behavior.
Comment on attachment 330394 [details] Patch v13 View in context: https://bugs.webkit.org/attachment.cgi?id=330394&action=review >> Source/WTF/ChangeLog:30 >> + Release buidls, as well as other "checked" functions. > > Typo: "buidls" => "builds" Fixed. Thanks for spotting this! >> Source/WebCore/platform/graphics/mac/SimpleFontDataCoreText.cpp:47 >> + CFMutableDictionaryRef mutableAttributes = checked_cf_cast<CFMutableDictionaryRef>(attributesDictionary.get()); > > Here I'd probably just switch to using `attributesDictionary.get()` locally instead of `mutableAttributes`. This is a local we just created on line 46, we shouldn't need to check its type. Done! Had to change the type of m_kernedCFStringAttributes and m_nonKernedCFStringAttributes in Font.h from RetainPtr<CFDictionaryRef> to RetainPtr<CFMutableDictionaryRef>, but that's more truthy anyway. >> Source/WebCore/testing/cocoa/WebArchiveDumpSupport.mm:223 >> + RetainPtr<CFMutableDictionaryRef> propertyList = adoptCF(checked_cf_cast<CFMutableDictionaryRef>(CFPropertyListCreateWithData(kCFAllocatorDefault, webArchiveData, kCFPropertyListMutableContainersAndLeaves, &format, &error))); > > For these WebArchive cases, I think that if we got an unexpected type that we would want to bail with an error message instead of crashing (inside a cast check). That said the checked casts are certainly better then previous behavior. At best we could output the CFTypeID of the incorrectly typed object. If it's a CoreFoundation type, then we might be able to give the name of the object type in the log output, but for CFTypes like DDResultRef (as a random example that probably would never happen here), I think the CFTypeID is assigned at runtime, so there's no way we could exhaustively map a CFTypeID to a string name at runtime. So I'm not sure that the CFTypeID by itself would be all that useful in the crash output (again unless it's a native CoreFoundation type--although I didn't check if those had static CFTypeIDs or not).
(In reply to Joseph Pecoraro from comment #34) > Comment on attachment 330394 [details] > Patch v13 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=330394&action=review > > Looks good to me. I didn't really understand the RetainPtr changes though so > I'll let someone else review. Internally, RetainPtr<> stores a pointer to the object it references using CFTypeRef: typedef CFTypeRef StorageType; And CFTypeRef is defined as: typedef const CF_BRIDGED_TYPE(id) void * CFTypeRef; The reason the const_cast<> is needed here is that some CF objects (like CFMutable*Ref types and most CF types defined outside of CoreFoundation.framework) are defined as non-const pointers (compare CFArrayRef to CFMutableArrayRef): typedef const struct CF_BRIDGED_TYPE(NSArray) __CFArray * CFArrayRef; typedef struct CF_BRIDGED_MUTABLE_TYPE(NSMutableArray) __CFArray * CFMutableArrayRef; typedef struct DD_BRIDGED_TYPE(id) __DDResult * DDResultRef; So when we request the "raw" pointer from a RetainPtr<> (using get() or leakRef()) with a template type that doesn't have a 'const' modifier, we need the const_cast<> to make sure the compiler doesn't emit a -Wcast-qual warning when casting from the internal 'const' (CFTypeRef) pointer to 'non-const' (template type) pointer. The const_cast<> is not needed for CFTypes with 'const' modifiers, but it does no harm since casting from non-const back to const is okay.
Created attachment 330574 [details] Patch v14
Attachment 330574 [details] did not pass style-queue: ERROR: Source/WTF/wtf/cf/TypeCastsCF.h:70: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5] Total errors found: 1 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 330574 [details] Patch v14 r=me, thanks for the detailed explanation
Comment on attachment 330574 [details] Patch v14 Rejecting attachment 330574 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 330574, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: .webkit.org/git/WebKit e552102..c1c0711 master -> origin/master Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 226466 = e55210209101d2e7c86dddd6a26640e11db1ea9d r226467 = c1c07115837487ce093672773264ad8605be5ef6 Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Full output: http://webkit-queues.webkit.org/results/5947220
Comment on attachment 330574 [details] Patch v14 Clearing flags on attachment: 330574 Committed r226483: <https://trac.webkit.org/changeset/226483>
All reviewed patches have been landed. Closing bug.