Bug 184012 - Unable to use HashMap<WebCore::SecurityOriginData, Ref<WebSWServerToContextConnection>> type in StorageProcess.h
Summary: Unable to use HashMap<WebCore::SecurityOriginData, Ref<WebSWServerToContextCo...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-03-26 10:31 PDT by Chris Dumez
Modified: 2018-04-16 11:37 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.10 KB, patch)
2018-03-27 16:16 PDT, Chris Dumez
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2018-03-26 10:31:30 PDT
Unable to use HashMap<WebCore::SecurityOriginData, RefPtr<WebSWServerToContextConnection>> type in StorageProcess.h. It causes an assertion to be hit in debug:
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x00000001243b97b4 WTFCrash + 36 (Assertions.cpp:271)
1   com.apple.WebKit              	0x000000010f510de6 WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> >::leakRef() + 70 (Ref.h:133)
2   com.apple.WebKit              	0x000000010f510d51 WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> >::Ref(WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> >&&) + 33 (Ref.h:75)
3   com.apple.WebKit              	0x000000010f510d1d WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> >::Ref(WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> >&&) + 29 (Ref.h:78)
4   com.apple.WebKit              	0x000000010f510cec WTF::KeyValuePair<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > >::KeyValuePair<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > >(WebCore::SecurityOriginData&&, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> >&&) + 76 (KeyValuePair.h:46)
5   com.apple.WebKit              	0x000000010f510c25 WTF::KeyValuePair<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > >::KeyValuePair<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > >(WebCore::SecurityOriginData&&, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> >&&) + 37 (KeyValuePair.h:46)
6   com.apple.WebKit              	0x000000010f510bb9 WTF::KeyValuePairHashTraits<WTF::HashTraits<WebCore::SecurityOriginData>, WTF::HashTraits<WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > > >::emptyValue() + 57 (HashTraits.h:292)
7   com.apple.WebKit              	0x000000010f510b79 void WTF::HashTableBucketInitializer<false>::initialize<WTF::HashMap<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> >, WebCore::SecurityOriginDataHash, WTF::HashTraits<WebCore::SecurityOriginData>, WTF::HashTraits<WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > > >::KeyValuePairTraits, WTF::KeyValuePair<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > > >(WTF::KeyValuePair<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > >&) + 57 (HashTable.h:841)
8   com.apple.WebKit              	0x000000010f510b35 WTF::HashTable<WebCore::SecurityOriginData, WTF::KeyValuePair<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > > >, WebCore::SecurityOriginDataHash, WTF::HashMap<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> >, WebCore::SecurityOriginDataHash, WTF::HashTraits<WebCore::SecurityOriginData>, WTF::HashTraits<WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > > >::KeyValuePairTraits, WTF::HashTraits<WebCore::SecurityOriginData> >::initializeBucket(WTF::KeyValuePair<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > >&) + 21 (HashTable.h:858)
9   com.apple.WebKit              	0x000000010f51095b WTF::HashTable<WebCore::SecurityOriginData, WTF::KeyValuePair<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > > >, WebCore::SecurityOriginDataHash, WTF::HashMap<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> >, WebCore::SecurityOriginDataHash, WTF::HashTraits<WebCore::SecurityOriginData>, WTF::HashTraits<WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > > >::KeyValuePairTraits, WTF::HashTraits<WebCore::SecurityOriginData> >::allocateTable(unsigned int) + 75 (HashTable.h:1148)
10  com.apple.WebKit              	0x000000010f5106e8 WTF::HashTable<WebCore::SecurityOriginData, WTF::KeyValuePair<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > > >, WebCore::SecurityOriginDataHash, WTF::HashMap<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> >, WebCore::SecurityOriginDataHash, WTF::HashTraits<WebCore::SecurityOriginData>, WTF::HashTraits<WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > > >::KeyValuePairTraits, WTF::HashTraits<WebCore::SecurityOriginData> >::rehash(unsigned int, WTF::KeyValuePair<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > >*) + 72 (HashTable.h:1197)
11  com.apple.WebKit              	0x000000010f518385 WTF::HashTable<WebCore::SecurityOriginData, WTF::KeyValuePair<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > > >, WebCore::SecurityOriginDataHash, WTF::HashMap<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> >, WebCore::SecurityOriginDataHash, WTF::HashTraits<WebCore::SecurityOriginData>, WTF::HashTraits<WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > > >::KeyValuePairTraits, WTF::HashTraits<WebCore::SecurityOriginData> >::expand(WTF::KeyValuePair<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > >*) + 117 (HashTable.h:1174)
12  com.apple.WebKit              	0x000000010f51809c WTF::HashTableAddResult<WTF::HashTableIterator<WebCore::SecurityOriginData, WTF::KeyValuePair<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > > >, WebCore::SecurityOriginDataHash, WTF::HashMap<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> >, WebCore::SecurityOriginDataHash, WTF::HashTraits<WebCore::SecurityOriginData>, WTF::HashTraits<WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > > >::KeyValuePairTraits, WTF::HashTraits<WebCore::SecurityOriginData> > > WTF::HashTable<WebCore::SecurityOriginData, WTF::KeyValuePair<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > > >, WebCore::SecurityOriginDataHash, WTF::HashMap<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> >, WebCore::SecurityOriginDataHash, WTF::HashTraits<WebCore::SecurityOriginData>, WTF::HashTraits<WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > > >::KeyValuePairTraits, WTF::HashTraits<WebCore::SecurityOriginData> >::add<WTF::HashMapTranslator<WTF::HashMap<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> >, WebCore::SecurityOriginDataHash, WTF::HashTraits<WebCore::SecurityOriginData>, WTF::HashTraits<WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > > >::KeyValuePairTraits, WebCore::SecurityOriginDataHash>, WebCore::SecurityOriginData const&, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > >(WebCore::SecurityOriginData const&&&, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> >&&) + 108 (HashTable.h:869)
13  com.apple.WebKit              	0x000000010f51801c WTF::HashTableAddResult<WTF::HashTableIterator<WebCore::SecurityOriginData, WTF::KeyValuePair<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > > >, WebCore::SecurityOriginDataHash, WTF::HashMap<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> >, WebCore::SecurityOriginDataHash, WTF::HashTraits<WebCore::SecurityOriginData>, WTF::HashTraits<WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > > >::KeyValuePairTraits, WTF::HashTraits<WebCore::SecurityOriginData> > > WTF::HashMap<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> >, WebCore::SecurityOriginDataHash, WTF::HashTraits<WebCore::SecurityOriginData>, WTF::HashTraits<WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > > >::inlineAdd<WebCore::SecurityOriginData const&, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > >(WebCore::SecurityOriginData const&&&, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> >&&) + 60 (HashMap.h:346)
14  com.apple.WebKit              	0x000000010f505f34 WTF::HashTableAddResult<WTF::HashTableIterator<WebCore::SecurityOriginData, WTF::KeyValuePair<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > > >, WebCore::SecurityOriginDataHash, WTF::HashMap<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> >, WebCore::SecurityOriginDataHash, WTF::HashTraits<WebCore::SecurityOriginData>, WTF::HashTraits<WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > > >::KeyValuePairTraits, WTF::HashTraits<WebCore::SecurityOriginData> > > WTF::HashMap<WebCore::SecurityOriginData, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> >, WebCore::SecurityOriginDataHash, WTF::HashTraits<WebCore::SecurityOriginData>, WTF::HashTraits<WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > > >::add<WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> > >(WebCore::SecurityOriginData const&, WTF::Ref<WebKit::WebSWServerToContextConnection, WTF::DumbPtrTraits<WebKit::WebSWServerToContextConnection> >&&) + 52 (HashMap.h:381)
15  com.apple.WebKit              	0x000000010f505616 WebKit::StorageProcess::createStorageToWebProcessConnection(bool, WebCore::SecurityOriginData&&) + 726 (StorageProcess.cpp:281)
Comment 1 Chris Dumez 2018-03-26 10:32:01 PDT
HashMap<RefPtr<WebCore::SecurityOrigin>, RefPtr<WebSWServerToContextConnection>> type
Comment 2 Chris Dumez 2018-03-26 10:34:16 PDT
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.
Comment 3 Chris Dumez 2018-03-26 10:35:40 PDT
(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));
        }
    };
Comment 4 Chris Dumez 2018-03-26 10:41:33 PDT
(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?
Comment 5 Darin Adler 2018-03-27 09:55:43 PDT
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.
Comment 6 Chris Dumez 2018-03-27 10:07:59 PDT
(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.
Comment 7 Chris Dumez 2018-03-27 16:16:58 PDT
Created attachment 336628 [details]
Patch
Comment 8 Ryosuke Niwa 2018-04-16 11:37:17 PDT
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.