Bug 189044

Summary: Add specialized template declarations of HashTraits and DefaultHash to detect misuse
Product: WebKit Reporter: Fujii Hironori <Hironori.Fujii>
Component: Web Template FrameworkAssignee: Fujii Hironori <Hironori.Fujii>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, alecflett, beidson, benjamin, cdumez, cmarcelo, darin, dbates, ews, jsbell, ross.kirsling, sbarati, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch to land none

Description Fujii Hironori 2018-08-28 02:10:45 PDT
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)
Comment 1 Fujii Hironori 2018-08-28 02:24:52 PDT
Created attachment 348281 [details]
Patch
Comment 2 Fujii Hironori 2018-08-28 03:09:08 PDT
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>;
Comment 3 Fujii Hironori 2018-08-28 03:12:27 PDT
Created attachment 348286 [details]
Patch
Comment 4 Fujii Hironori 2018-08-28 03:56:12 PDT
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?
Comment 5 Ross Kirsling 2018-08-28 13:28:42 PDT
Created attachment 348330 [details]
Patch
Comment 6 Ross Kirsling 2018-08-28 13:29:47 PDT
(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 7 Darin Adler 2018-08-28 20:59:08 PDT
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 8 Fujii Hironori 2018-08-28 22:42:30 PDT
Comment on attachment 348330 [details]
Patch

Thank you very much, Darin and Ross. I will check and revise.
Comment 9 Fujii Hironori 2018-08-29 23:05:53 PDT
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.
Comment 10 Darin Adler 2018-08-30 09:06:48 PDT
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.
Comment 11 Fujii Hironori 2018-09-03 20:09:19 PDT
Yes. It will report compilation errors (comment 2 and comment 4).
Comment 12 Fujii Hironori 2018-09-03 21:27:43 PDT
Created attachment 348803 [details]
Patch
Comment 13 Fujii Hironori 2018-09-04 19:41:51 PDT
Addressed review feedbacks. Please review again.
Comment 14 Fujii Hironori 2018-09-05 18:25:27 PDT
review?
Comment 15 Fujii Hironori 2018-09-06 18:34:11 PDT
review?
Comment 16 Yusuke Suzuki 2018-09-07 09:53:15 PDT
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`.
Comment 17 Fujii Hironori 2018-09-09 19:15:12 PDT
Created attachment 349294 [details]
Patch to land

Thank you very much for the review.
Comment 18 Fujii Hironori 2018-09-09 22:21:46 PDT
Comment on attachment 349294 [details]
Patch to land

Clearing flags on attachment: 349294

Committed r235841: <https://trac.webkit.org/changeset/235841>
Comment 19 Fujii Hironori 2018-09-09 22:21:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Radar WebKit Bug Importer 2018-09-09 22:22:22 PDT
<rdar://problem/44291258>