| Summary: | Unable to use HashMap<WebCore::SecurityOriginData, Ref<WebSWServerToContextConnection>> type in StorageProcess.h | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||
| Component: | Web Template Framework | Assignee: | Chris Dumez <cdumez> | ||||
| Status: | NEW --- | ||||||
| Severity: | Normal | CC: | ap, beidson, darin, rniwa, sam, youennf | ||||
| Priority: | P2 | ||||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=183969 | ||||||
| Attachments: |
|
||||||
|
Description
Chris Dumez
2018-03-26 10:31:30 PDT
HashMap<RefPtr<WebCore::SecurityOrigin>, RefPtr<WebSWServerToContextConnection>> type Using HashMap<RefPtr<WebCore::SecurityOrigin>, Ref<WebSWServerToContextConnection>> works. I believe the different is that emptyValueIsZero for both the Key and the Value. emptyValueIsZero is false when the key is a SecurityOriginData. So KeyValuePairHashTraits::emptyValueIsZero goes from true to false and it causes failures. (In reply to Chris Dumez from comment #2) > Using HashMap<RefPtr<WebCore::SecurityOrigin>, > Ref<WebSWServerToContextConnection>> works. I believe the different is that > emptyValueIsZero for both the Key and the Value. > > emptyValueIsZero is false when the key is a SecurityOriginData. So > KeyValuePairHashTraits::emptyValueIsZero goes from true to false and it > causes failures. As a result, it ends up using a different template specialization for HashTableBucketInitializer: template<bool emptyValueIsZero> struct HashTableBucketInitializer; template<> struct HashTableBucketInitializer<false> { template<typename Traits, typename Value> static void initialize(Value& bucket) { new (NotNull, std::addressof(bucket)) Value(Traits::emptyValue()); } }; template<> struct HashTableBucketInitializer<true> { template<typename Traits, typename Value> static void initialize(Value& bucket) { // This initializes the bucket without copying the empty value. // That makes it possible to use this with types that don't support copying. // The memset to 0 looks like a slow operation but is optimized by the compilers. memset(std::addressof(bucket), 0, sizeof(bucket)); } }; (In reply to Chris Dumez from comment #3) > (In reply to Chris Dumez from comment #2) > > Using HashMap<RefPtr<WebCore::SecurityOrigin>, > > Ref<WebSWServerToContextConnection>> works. I believe the different is that > > emptyValueIsZero for both the Key and the Value. > > > > emptyValueIsZero is false when the key is a SecurityOriginData. So > > KeyValuePairHashTraits::emptyValueIsZero goes from true to false and it > > causes failures. > > As a result, it ends up using a different template specialization for > HashTableBucketInitializer: > template<bool emptyValueIsZero> struct HashTableBucketInitializer; > > template<> struct HashTableBucketInitializer<false> { > template<typename Traits, typename Value> static void > initialize(Value& bucket) > { > new (NotNull, std::addressof(bucket)) > Value(Traits::emptyValue()); > } > }; > > template<> struct HashTableBucketInitializer<true> { > template<typename Traits, typename Value> static void > initialize(Value& bucket) > { > // This initializes the bucket without copying the empty value. > // That makes it possible to use this with types that don't > support copying. > // The memset to 0 looks like a slow operation but is optimized > by the compilers. > memset(std::addressof(bucket), 0, sizeof(bucket)); > } > }; So instead of doing a memset zero, we now rely on Traits::emptyValue(), which for Ref<T> is static Ref<P> emptyValue() { return HashTableEmptyValue; } This is implemented as: Ref(HashTableEmptyValueType) : m_ptr(hashTableEmptyValue()) { } hashTableDeletedValue() returns nullptr. So when we construct a KeyValuePair: template<typename K, typename V> KeyValuePair(K&& key, V&& value) : key(std::forward<K>(key)) , value(std::forward<V>(value)) { } And V is HashTraits<Ref<T>>::emptyValue(), then it calls the Ref<T>(Ref<t>&&) move constructor, which ends up calling leakRef() on the Ref. This is implemented as: T& leakRef() WARN_UNUSED_RETURN { ASSERT(m_ptr); / ... } We hit this assertion since m_ptr is nullptr when the Ref<> is an empty HashTable value :/ Advice on how to best address this would be appreciated: 1. Dropping this assertion would be unfortunate since it is useful to catch bugs 2. Using another value than nullptr as empty HashTable value maybe? So we consider it a programming error to move from a Ref that is null. But we want to use null values in the hash table. I think the best way to work around this is in the hash table machinery rather than in Ref; I don’t think we should use a value other than nullptr for empty since I think it would hurt efficiency. For this specific case, I think we could figure out how to make emptyValueIsZero be true for SecurityOriginData. I am pretty sure that works even for std::optional. (In reply to Darin Adler from comment #5) > So we consider it a programming error to move from a Ref that is null. But > we want to use null values in the hash table. I think the best way to work > around this is in the hash table machinery rather than in Ref; I don’t think > we should use a value other than nullptr for empty since I think it would > hurt efficiency. > > For this specific case, I think we could figure out how to make > emptyValueIsZero be true for SecurityOriginData. I am pretty sure that works > even for std::optional. Fixing it only for SecurityOriginData would not be optimal IMO. Someone else is going to get caught by this in the future for another key type's whose emptyValueIsZero is false. Created attachment 336628 [details]
Patch
Comment on attachment 336628 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336628&action=review > Source/WebCore/ChangeLog:9 > + Set SecurityOriginDataHashTraits::emptyValueIsZero to true. This is OK because > + the data members of SecurityOriginData are of type String or std::optional<uint16_t>. This seems a bit fragile. Can we add an assertion somewhere to make sure this assumption holds? e.g. memset SecurityOriginData and make sure isEmpty returns true there. |