Add specialize template declarations of HashTraits and DefaultHash to detect misuse Some classes have a separate header for specializations of WTF::HashTraits and WTF::DefaultHash to reduce the number of header files included. For example, ColorHash.h and Color.h. If someone is mistakenly using hash without including the specialization header, the linker would silently produce an broken executable. (Bug 188893)
Created attachment 348281 [details] Patch
All Mac EWS bots are red. It catches the misuse! > CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/AVCaptureDeviceManager.o platform/mediastream/mac/AVCaptureDeviceManager.mm normal x86_64 objective-c++ com.apple.compilers.llvm.clang.1_0.compiler > cd /Volumes/Data/EWS/WebKit/Source/WebCore > export LANG=en_US.US-ASCII > /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang -x objective-c++ -arch x86_64 -fmessage-length=0 -fdiagnostics-show-note-include-stack -fmacro-backtrace-limit=0 -std=gnu++1y -stdlib=libc++ -fobjc-weak -gmodules -Wno-trigraphs -fno-exceptions -fno-rtti -fpascal-strings -O2 -fno-common -Werror -Wno-missing-field-initializers -Wmissing-prototypes -Wunreachable-code -Wno-implicit-atomic-properties -Wno-arc-repeated-use-of-weak -Wnon-virtual-dtor -Wno-overloaded-virtual -Wno-exit-time-destructors -Wduplicate-method-match -Wno-missing-braces -Wparentheses -Wswitch -Wunused-function -Wno-unused-label -Wno-unused-parameter -Wunused-variable -Wunused-value -Wempty-body -Wuninitialized -Wno-unknown-pragmas -Wno-shadow -Wno-four-char-constants -Wno-conversion -Wconstant-conversion -Wint-conversion -Wbool-conversion -Wenum-conversion -Wsign-compare -Wno-shorten-64-to-32 -Wnewline-eof -Wno-selector -Wno-strict-selector-match -Wundeclared-selector -Wno-deprecated-implementations -Wno-c++11-extensions -DNDEBUG -DENABLE_3D_TRANSFORMS -DENABLE_APPLE_PAY -DENABLE_APPLICATION_MANIFEST -DENABLE_ATTACHMENT_ELEMENT -DENABLE_AVF_CAPTIONS -DENABLE_CACHE_PARTITIONING -DENABLE_CHANNEL_MESSAGING -DENABLE_CONTENT_FILTERING -DENABLE_CSS_ANIMATIONS_LEVEL_2 -DENABLE_CSS_BOX_DECORATION_BREAK -DENABLE_CSS_COMPOSITING -DENABLE_CSS_SCROLL_SNAP -DENABLE_CSS_SELECTORS_LEVEL4 -DENABLE_CSS_TRAILING_WORD -DENABLE_CURSOR_VISIBILITY -DENABLE_DASHBOARD_SUPPORT -DENABLE_DATACUE_VALUE -DENABLE_EXPERIMENTAL_FEATURES -DENABLE_FILTERS_LEVEL_2 -DENABLE_FTL_JIT -DENABLE_FULLSCREEN_API -DENABLE_GAMEPAD -DENABLE_GEOLOCATION -DENABLE_ICONDATABASE -DENABLE_INDEXED_DATABASE -DENABLE_INDEXED_DATABASE_IN_WORKERS -DENABLE_INTERSECTION_OBSERVER -DENABLE_INTL -DENABLE_JS_ASYNC_ITERATION -DENABLE_KEYBOARD_CODE_ATTRIBUTE -DENABLE_KEYBOARD_KEY_ATTRIBUTE -DENABLE_LEGACY_CSS_VENDOR_PREFIXES -DENABLE_LEGACY_CUSTOM_PROTOCOL_MANAGER -DENABLE_LEGACY_ENCRYPTED_MEDIA -DENABLE_MATHML -DENABLE_MEDIA_CONTROLS_SCRIPT -DENABLE_MEDIA_SOURCE -DENABLE_MEDIA_STREAM -DENABLE_METER_ELEMENT -DENABLE_MOUSE_CURSOR_SCALE -DENABLE_NOTIFICATIONS -DENABLE_PAYMENT_REQUEST -DENABLE_PDFKIT_PLUGIN -DENABLE_POINTER_LOCK -DENABLE_PUBLIC_SUFFIX_LIST -DENABLE_REMOTE_INSPECTOR -DENABLE_RESOURCE_USAGE -DENABLE_RUBBER_BANDING -DENABLE_SERVICE_CONTROLS -DENABLE_SERVICE_WORKER -DENABLE_SPEECH_SYNTHESIS -DENABLE_STREAMS_API -DENABLE_SUBTLE_CRYPTO -DENABLE_SVG_FONTS -DENABLE_TELEPHONE_NUMBER_DETECTION -DENABLE_TEXT_AUTOSIZING -DENABLE_USER_MESSAGE_HANDLERS -DENABLE_USERSELECT_ALL -DENABLE_VIDEO -DENABLE_VIDEO_PRESENTATION_MODE -DENABLE_VIDEO_TRACK -DENABLE_VIDEO_USES_ELEMENT_FULLSCREEN -DENABLE_WEB_AUDIO -DENABLE_WEB_AUTHN -DENABLE_WEB_RTC -DENABLE_WEBGL -DENABLE_WEBGL2 -DENABLE_WEBGPU -DENABLE_WIRELESS_PLAYBACK_TARGET -DENABLE_XSLT -DBUILDING_WEBKIT -DU_DISABLE_RENAMING=1 -DU_SHOW_CPLUSPLUS_API=0 -DGL_SILENCE_DEPRECATION=1 -DGLES_SILENCE_DEPRECATION=1 -DOBJC_OLD_DISPATCH_PROTOTYPES=0 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk -fasm-blocks -fstrict-aliasing -Wprotocol -Wdeprecated-declarations -Winvalid-offsetof -mmacosx-version-min=10.12 -g -fvisibility=hidden -fvisibility-inlines-hidden -fno-threadsafe-statics -Wno-sign-conversion -Winfinite-recursion -Wmove -iquote /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/WebCore-generated-files.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/WebCore-own-target-headers.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/WebCore-all-target-headers.hmap -iquote /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/WebCore-project-headers.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/include -IPAL -IForwardingHeaders -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/usr/include/libxslt -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/usr/include/libxml2 -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/WebKitAdditions -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/usr/local/include/WebKitAdditions -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/webrtc -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/usr/local/include/webrtc -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/webrtc/sdk/objc/Framework/Headers -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/usr/local/include/webrtc/sdk/objc/Framework/Headers -I/Volumes/Data/EWS/WebKit/Source/WebCore -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/DerivedSources/x86_64 -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/DerivedSources -Wall -Wextra -Wcast-qual -Wchar-subscripts -Wextra-tokens -Wformat=2 -Winit-self -Wmissing-format-attribute -Wmissing-noreturn -Wpacked -Wpointer-arith -Wredundant-decls -Wundef -Wwrite-strings -Wexit-time-destructors -Wglobal-constructors -Wtautological-compare -Wimplicit-fallthrough -Wno-unknown-warning-option -F/Volumes/Data/EWS/WebKit/WebKitBuild/Release -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/PrivateFrameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks -isystem /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/System.framework/PrivateHeaders -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/Carbon.framework/Frameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/CoreServices.framework/Frameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/Quartz.framework/Frameworks -include /Volumes/Data/EWS/WebKit/WebKitBuild/PrecompiledHeaders/WebCorePrefix-desqhkomlzqricciuchocluavzcj/WebCorePrefix.h -MMD -MT dependencies -MF /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/AVCaptureDeviceManager.d --serialize-diagnostics /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/AVCaptureDeviceManager.dia -c /Volumes/Data/EWS/WebKit/Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/AVCaptureDeviceManager.o > In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:27: > In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.h:31: > In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/platform/mediastream/CaptureDeviceManager.h:31: > In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/platform/mediastream/RealtimeMediaSource.h:41: > In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/platform/MediaSample.h:30: > In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/JavaScriptCore.framework/PrivateHeaders/TypedArrays.h:29: > In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/JavaScriptCore.framework/PrivateHeaders/TypedArrayAdaptors.h:28: > In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/JavaScriptCore.framework/PrivateHeaders/JSCJSValue.h:32: > /Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/wtf/HashMap.h:52:33: error: incomplete type 'WTF::HashTraits<WebCore::IntSize>' named in nested name specifier > using MappedType = typename MappedTraits::TraitType; > ^~~~~~~~~~~~~~ > In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:32: > /Volumes/Data/EWS/WebKit/Source/WebCore/platform/mediastream/mac/AVVideoCaptureSource.h:103:20: note: in instantiation of template class 'WTF::HashMap<WTF::String, WebCore::IntSize, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WebCore::IntSize> >' requested here > VideoPresetMap m_supportedPresets; > ^ > In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.mm:27: > In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/platform/mediastream/mac/AVCaptureDeviceManager.h:31: > In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/platform/mediastream/CaptureDeviceManager.h:31: > In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/platform/mediastream/RealtimeMediaSource.h:39: > In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/platform/graphics/Image.h:30: > In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/platform/graphics/DecodingOptions.h:28: > /Volumes/Data/EWS/WebKit/Source/WebCore/platform/graphics/IntSize.h:225:19: note: forward declaration of 'WTF::HashTraits<WebCore::IntSize>' > template<> struct HashTraits<WebCore::IntSize>;
Created attachment 348286 [details] Patch
Oh, no. iOS EWS is failing again. > In file included from /Volumes/Data/EWS/WebKit/Source/WebKit/UIProcess/ProcessThrottler.h:33: > In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphoneos/usr/local/include/wtf/RunLoop.h:35: > /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphoneos/usr/local/include/wtf/HashMap.h:52:33: error: incomplete type 'WTF::HashTraits<WebCore::IntPoint>' named in nested name specifier > using MappedType = typename MappedTraits::TraitType; > ^~~~~~~~~~~~~~ > In file included from /Volumes/Data/EWS/WebKit/Source/WebKit/Shared/API/Cocoa/RemoteObjectRegistry.mm:34: > /Volumes/Data/EWS/WebKit/Source/WebKit/WebProcess/WebPage/WebPage.h:1662:69: note: in instantiation of template class 'WTF::HashMap<std::__1::pair<WebCore::IntSize, double>, WebCore::IntPoint, WTF::PairHash<WebCore::IntSize, double>, WTF::HashTraits<std::__1::pair<WebCore::IntSize, double> >, WTF::HashTraits<WebCore::IntPoint> >' requested here > HashMap<std::pair<WebCore::IntSize, double>, WebCore::IntPoint> m_dynamicSizeUpdateHistory; It seems that WebPage.h need to include IntPointHash.h. Could any iOS port falks volunteer to make it green?
Created attachment 348330 [details] Patch
(In reply to Fujii Hironori from comment #4) > It seems that WebPage.h need to include IntPointHash.h. > Could any iOS port falks volunteer to make it green? Done!
Comment on attachment 348330 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348330&action=review Not saying review+ because I don’t yet understand why there are no new tests or test progressions with this fix. > Source/WTF/ChangeLog:10 > + * wtf/HashIterators.h: Added #include <wtf/KeyValuePair.h>. Why? > Source/WTF/ChangeLog:14 > + Removed Removed unnecessarily HashTraits declaration. Should remove the second "Removed". Should say "unnecessary" instead of "unnecessarily". > Source/WebCore/ChangeLog:14 > + If someone is mistakenly using hash without including the > + specialization header, the linker would silently produce an > + broken executable. (Bug 188893) If that’s true, why aren’t all the cases where we added the needed includes here silently producing a broken executable already? Given the explanation here, it seems strange that none of these changes are producing test progressions. Do we need to add new tests? Or are these not actually fixing problems? > Source/WebCore/ChangeLog:28 > + * platform/network/ProtectionSpaceHash.h: Removed unnecessarily DefaultHash declaration. Should say "unnecessary" instead of "unnecessarily". > Source/WebCore/platform/URLHash.h:55 > + typedef WebCore::URLHash Hash; In new code we use "using" instead of typedef for this sort of thing. Since we moved this code, we could modernize it. > Source/WebCore/platform/graphics/FloatSize.h:31 > +#include <wtf/Forward.h> This is unneeded, since FloatSize.h includes IntPoint.h, which already takes care of this. > Source/WebCore/platform/graphics/IntPoint.h:30 > +#include <wtf/Forward.h> This is unneeded, since IntPoint.h includes IntSize.h, which already takes care of this.
Comment on attachment 348330 [details] Patch Thank you very much, Darin and Ross. I will check and revise.
This change found three misuses. DebugPageOverlays.cpp is using HashMap<String, Color> without including "ColorHash.h". AVVideoCaptureSource.h is using HashMap<String, IntSize> without including "IntSizeHash.h". WebPage.h is using HashMap<std::pair<WebCore::IntSize, double>, WebCore::IntPoint> without including "IntPointHash.h". All three are HashTraits for the second template arugment of HashMap. Here is HashMap declaration: > template<typename KeyArg, typename MappedArg, typename = typename DefaultHash<KeyArg>::Hash, typename = HashTraits<KeyArg>, typename = HashTraits<MappedArg>> class HashMap; The defaut fifth template arguemnt is HashTraits of the second argument. All HashTraits<WebCore::IntSize>, HashTraits<WebCore::IntPoint> and HashTraits<WebCore::Color> inherit GenericHashTraits of each and override following class variables and class methods: * emptyValueIsZero * emptyValue() * needsDestruction * constructDeletedValue() * isDeletedValue() needsDestruction has been obsoleted in Bug 121983, not used at all. (I will remove them in follow-up patch.) constructDeletedValue() and isDeletedValue() are not used in these misuse cases because the missing traits are MappedTraits of HashMap, not KeyTraits of it. HashMap is using HashTable with KeyValuePairHashTraits as Traits. KeyValuePairHashTraits::constructDeletedValue and KeyValuePairHashTraits::isDeletedValue are using only one of KeyTraits. emptyValueIsZero is only for speed. HashTable fills slot with zero if it is enabled. emptyValue() can cause a problem. HashTraits<WebCore::IntPoint> and HashTraits<WebCore::Color> overrides it. emptyValue() is used in following two cases: 1. HashTable initializes the buffer by filling with KeyValuePairHashTraits::emptyValue() 2. HashMap::get() and HashMap::take() returns emptyValue() if key is not found For (1), it can't be a problem if consistently misused because KeyValuePairHashTraits::emptyValue() is using both KeyTraits::emptyValue() and ValueTraits::emptyValue(). KeyTraits::emptyValue() solely can make it unique. For (2), WebPageIOS.mm doesn't use get() and take(). DebugPageOverlays.cpp is using get(), but calls get() only with existing keys. I conclude the misuses doesn't introduce any bugs at the moment, and they are not testable.
OK, thanks for the analysis. Now I understand why this change doesn’t fix bugs, does enable a tiny bit more optimization, and introduces warnings/errors (not sure which) that could prevent future subtle bugs.
Yes. It will report compilation errors (comment 2 and comment 4).
Created attachment 348803 [details] Patch
Addressed review feedbacks. Please review again.
review?
Comment on attachment 348803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348803&action=review r=me with a small nit, interesting :) > Source/WebCore/platform/network/ProtectionSpaceHash.h:63 > typedef WebCore::ProtectionSpaceHash Hash; Let's use `using Hash = WebCore::ProtectionSpaceHash`.
Created attachment 349294 [details] Patch to land Thank you very much for the review.
Comment on attachment 349294 [details] Patch to land Clearing flags on attachment: 349294 Committed r235841: <https://trac.webkit.org/changeset/235841>
All reviewed patches have been landed. Closing bug.
<rdar://problem/44291258>