Bug 189044 – Add specialized template declarations of HashTraits and DefaultHash to detect misuse

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Fujii Hironori]

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)

[Fujii Hironori]

Created attachment 348281 [details]
Patch

[Fujii Hironori]

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>;

[Fujii Hironori]

Created attachment 348286 [details]
Patch

[Fujii Hironori]

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?

[Ross Kirsling]

Created attachment 348330 [details]
Patch

[Ross Kirsling]

(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!

[Darin Adler]

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.

[Fujii Hironori]

Comment on attachment 348330 [details]
Patch

Thank you very much, Darin and Ross. I will check and revise.

[Fujii Hironori]

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.

[Darin Adler]

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.

[Fujii Hironori]

Yes. It will report compilation errors (comment 2 and comment 4).

[Fujii Hironori]

Created attachment 348803 [details]
Patch

[Fujii Hironori]

Addressed review feedbacks. Please review again.

[Fujii Hironori]

review?

[Fujii Hironori]

review?

[Yusuke Suzuki]

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`.

[Fujii Hironori]

Created attachment 349294 [details]
Patch to land

Thank you very much for the review.

[Fujii Hironori]

Comment on attachment 349294 [details]
Patch to land

Clearing flags on attachment: 349294

Committed r235841: <https://trac.webkit.org/changeset/235841>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44291258>