Unreviewed, reverting 278977@main.

https://bugs.webkit.org/show_bug.cgi?id=274423

Seems to have introduced an assertion hit in StringImpl::setHash()

Reverted changeset:

"Optimize PublicSuffixStore"
https://bugs.webkit.org/show_bug.cgi?id=274358
https://commits.webkit.org/278977@main

Canonical link: https://commits.webkit.org/279018@main

Source/WTF/wtf/HashMap.h

@@ -168,9 +168,9 @@ class HashMap final {
     MappedTakeType take(iterator);
     MappedTakeType takeFirst();
 
-    // Alternate versions of find() / contains() / get() / remove() that find the object
-    // by hashing and comparing with some other type, to avoid the cost of type conversion.
-    // HashTranslator must have the following function members:
+    // An alternate version of find() that finds the object by hashing and comparing
+    // with some other type, to avoid the cost of type conversion. HashTranslator
+    // must have the following function members:
     //   static unsigned hash(const T&);
     //   static bool equal(const ValueType&, const T&);
     template<typename HashTranslator, typename T> iterator find(const T&);
@@ -180,14 +180,13 @@ class HashMap final {
     template<typename HashTranslator, typename T> MappedPeekType inlineGet(const T&) const;
     template<typename HashTranslator, typename T> bool remove(const T&);
 
-    // Alternate versions of add() / ensure() that find the object by hashing and comparing
+    // An alternate version of add() that finds the object by hashing and comparing
     // with some other type, to avoid the cost of type conversion if the object is already
     // in the table. HashTranslator must have the following function members:
     //   static unsigned hash(const T&);
     //   static bool equal(const ValueType&, const T&);
     //   static translate(ValueType&, const T&, unsigned hashCode);
     template<typename HashTranslator, typename K, typename V> AddResult add(K&&, V&&);
-    template<typename HashTranslator, typename K, typename Functor> AddResult ensure(K&&, Functor&&);
 
     // Overloads for smart pointer keys that take the raw pointer type as the parameter.
     template<typename K = KeyType> typename std::enable_if<IsSmartPtr<K>::value, iterator>::type find(std::add_const_t<typename GetPtrHelper<K>::UnderlyingType>*);
@@ -269,17 +268,6 @@ struct HashMapTranslatorAdapter {
     }
 };
 
-template<typename ValueTraits, typename Translator>
-struct HashMapEnsureTranslatorAdapter {
-    template<typename T> static unsigned hash(const T& key) { return Translator::hash(key); }
-    template<typename T, typename U> static bool equal(const T& a, const U& b) { return Translator::equal(a, b); }
-    template<typename T, typename U, typename Functor> static void translate(T& location, U&& key, Functor&& functor, unsigned hashCode)
-    {
-        Translator::translate(location.key, key, hashCode);
-        location.value = functor();
-    }
-};
-
 template<typename T, typename U, typename V, typename W, typename X, typename Y>
 inline void HashMap<T, U, V, W, X, Y>::swap(HashMap& other)
 {
@@ -451,13 +439,6 @@ auto HashMap<KeyArg, MappedArg, HashArg, KeyTraitsArg, MappedTraitsArg, TableTra
     return m_impl.template addPassingHashCode<HashMapTranslatorAdapter<KeyValuePairTraits, HashTranslator>>(std::forward<K>(key), std::forward<V>(value));
 }
 
-template<typename KeyArg, typename MappedArg, typename HashArg, typename KeyTraitsArg, typename MappedTraitsArg, typename TableTraitsArg>
-template<typename HashTranslator, typename K, typename Functor>
-auto HashMap<KeyArg, MappedArg, HashArg, KeyTraitsArg, MappedTraitsArg, TableTraitsArg>::ensure(K&& key, Functor&& functor) -> AddResult
-{
-    return m_impl.template addPassingHashCode<HashMapEnsureTranslatorAdapter<KeyValuePairTraits, HashTranslator>>(std::forward<K>(key), std::forward<Functor>(functor));
-}
-
 template<typename KeyArg, typename MappedArg, typename HashArg, typename KeyTraitsArg, typename MappedTraitsArg, typename TableTraitsArg>
 template<typename T>
 auto HashMap<KeyArg, MappedArg, HashArg, KeyTraitsArg, MappedTraitsArg, TableTraitsArg>::add(const KeyType& key, T&& mapped) -> AddResult

Source/WTF/wtf/URLHelpers.h

@@ -41,7 +41,7 @@ WTF_EXPORT_PRIVATE String userVisibleURL(const CString& URL);
 void loadIDNAllowedScriptList();
 void addScriptToIDNAllowedScriptList(const char* scriptName);
 void initializeDefaultIDNAllowedScriptList();
-WTF_EXPORT_PRIVATE std::optional<String> mapHostName(const String&, URLDecodeFunction);
+std::optional<String> mapHostName(const String&, URLDecodeFunction);
 String mapHostNames(const String&, URLDecodeFunction);
 
 } // namespace URLHelpers

Source/WTF/wtf/text/StringHash.h

@@ -250,12 +250,6 @@ namespace WTF {
         {
             return equalIgnoringASCIICaseCommon(a, b);
         }
-
-        static void translate(String& location, StringView view, unsigned hash)
-        {
-            location = view.toString();
-            location.impl()->setHash(hash);
-        }
     };
 
     struct HashTranslatorASCIILiteral {

Source/WTF/wtf/text/StringImpl.h

@@ -66,7 +66,6 @@ namespace WTF {
 class SymbolImpl;
 class SymbolRegistry;
 
-struct ASCIICaseInsensitiveStringViewHashTranslator;
 struct HashedUTF8CharactersTranslator;
 struct HashTranslatorASCIILiteral;
 struct LCharBufferTranslator;
@@ -186,7 +185,6 @@ class StringImpl : private StringImplShape {
     friend class SymbolImpl;
     friend class ExternalStringImpl;
 
-    friend struct WTF::ASCIICaseInsensitiveStringViewHashTranslator;
     friend struct WTF::HashedUTF8CharactersTranslator;
     friend struct WTF::HashTranslatorASCIILiteral;
     friend struct WTF::LCharBufferTranslator;

Source/WebCore/dom/Document.cpp

@@ -4108,8 +4108,8 @@ const URL& Document::urlForBindings() const
 
         auto areSameSiteIgnoringPublicSuffix = [](StringView domain, StringView otherDomain) {
             auto& publicSuffixStore = PublicSuffixStore::singleton();
-            auto domainString = publicSuffixStore.topPrivatelyControlledDomain(domain);
-            auto otherDomainString = publicSuffixStore.topPrivatelyControlledDomain(otherDomain);
+            auto domainString = publicSuffixStore.topPrivatelyControlledDomain(domain.toStringWithoutCopying());
+            auto otherDomainString = publicSuffixStore.topPrivatelyControlledDomain(otherDomain.toStringWithoutCopying());
             auto substringToSeparator = [](const String& string) -> String {
                 auto indexOfFirstSeparator = string.find('.');
                 if (indexOfFirstSeparator == notFound)

Source/WebCore/page/Quirks.cpp

@@ -101,7 +101,7 @@ static HashMap<RegistrableDomain, String>& updatableStorageAccessUserAgentString
 static inline bool isYahooMail(Document& document)
 {
     auto host = document.topDocument().url().host();
-    return host.startsWith("mail."_s) && PublicSuffixStore::singleton().topPrivatelyControlledDomain(host).startsWith("yahoo."_s);
+    return host.startsWith("mail."_s) && PublicSuffixStore::singleton().topPrivatelyControlledDomain(host.toString()).startsWith("yahoo."_s);
 }
 #endif
 
@@ -242,7 +242,7 @@ bool Quirks::shouldHideSearchFieldResultsButton() const
     if (!needsQuirks())
         return false;
 
-    if (PublicSuffixStore::singleton().topPrivatelyControlledDomain(m_document->topDocument().url().host()).startsWith("google."_s))
+    if (PublicSuffixStore::singleton().topPrivatelyControlledDomain(m_document->topDocument().url().host().toString()).startsWith("google."_s))
         return true;
 #endif
     return false;
@@ -419,13 +419,13 @@ bool Quirks::shouldDisableElementFullscreenQuirk() const
 #if ENABLE(TOUCH_EVENTS)
 bool Quirks::isAmazon() const
 {
-    return PublicSuffixStore::singleton().topPrivatelyControlledDomain(m_document->topDocument().url().host()).startsWith("amazon."_s);
+    return PublicSuffixStore::singleton().topPrivatelyControlledDomain(m_document->topDocument().url().host().toString()).startsWith("amazon."_s);
 }
 
 bool Quirks::isGoogleMaps() const
 {
     auto& url = m_document->topDocument().url();
-    return PublicSuffixStore::singleton().topPrivatelyControlledDomain(url.host()).startsWith("google."_s) && startsWithLettersIgnoringASCIICase(url.path(), "/maps/"_s);
+    return PublicSuffixStore::singleton().topPrivatelyControlledDomain(url.host().toString()).startsWith("google."_s) && startsWithLettersIgnoringASCIICase(url.path(), "/maps/"_s);
 }
 
 // rdar://49124313

Source/WebCore/platform/PublicSuffixStore.cpp

@@ -28,12 +28,9 @@
 
 #include <wtf/CrossThreadCopier.h>
 #include <wtf/NeverDestroyed.h>
-#include <wtf/text/StringHash.h>
 
 namespace WebCore {
 
-constexpr auto maxHostTopPrivatelyControlledDomainCache = 128;
-
 PublicSuffixStore& PublicSuffixStore::singleton()
 {
     static LazyNeverDestroyed<PublicSuffixStore> store;
@@ -74,31 +71,30 @@ String PublicSuffixStore::publicSuffix(const URL& url) const
     return { };
 }
 
-String PublicSuffixStore::topPrivatelyControlledDomain(StringView host) const
+String PublicSuffixStore::topPrivatelyControlledDomain(const String& host) const
 {
     // FIXME: if host is a URL, we could drop these checks.
     if (host.isEmpty())
         return { };
 
     if (!host.containsOnlyASCII())
-        return host.toString();
+        return host;
 
     Locker locker { m_HostTopPrivatelyControlledDomainCacheLock };
-    auto addResult = m_hostTopPrivatelyControlledDomainCache.ensure<ASCIICaseInsensitiveStringViewHashTranslator>(host, [&] {
-        const auto lowercaseHost = host.convertToASCIILowercase();
+    auto hostCopy = crossThreadCopy(host);
+    auto result = m_hostTopPrivatelyControlledDomainCache.ensure(hostCopy, [&] {
+        const auto lowercaseHost = hostCopy.convertToASCIILowercase();
         if (lowercaseHost == "localhost"_s || URL::hostIsIPAddress(lowercaseHost))
             return lowercaseHost;
 
         return platformTopPrivatelyControlledDomain(lowercaseHost);
-    });
+    }).iterator->value.isolatedCopy();
 
-    auto domain = addResult.iterator->value.isolatedCopy();
-    if (!addResult.isNewEntry) {
-        if (m_hostTopPrivatelyControlledDomainCache.size() > maxHostTopPrivatelyControlledDomainCache)
-            m_hostTopPrivatelyControlledDomainCache.remove(m_hostTopPrivatelyControlledDomainCache.random());
-    }
+    constexpr auto maxHostTopPrivatelyControlledDomainCache = 128;
+    if (m_hostTopPrivatelyControlledDomainCache.size() > maxHostTopPrivatelyControlledDomainCache)
+        m_hostTopPrivatelyControlledDomainCache.remove(m_hostTopPrivatelyControlledDomainCache.random());
 
-    return domain;
+    return result;
 }
 
 } // namespace WebCore

Source/WebCore/platform/PublicSuffixStore.h

@@ -39,7 +39,7 @@ class PublicSuffixStore {
     // https://url.spec.whatwg.org/#host-public-suffix
     WEBCORE_EXPORT bool isPublicSuffix(StringView domain) const;
     WEBCORE_EXPORT String publicSuffix(const URL&) const;
-    WEBCORE_EXPORT String topPrivatelyControlledDomain(StringView host) const;
+    WEBCORE_EXPORT String topPrivatelyControlledDomain(const String& host) const;
     WEBCORE_EXPORT void clearHostTopPrivatelyControlledDomainCache();
 
 #if PLATFORM(COCOA)
@@ -53,14 +53,14 @@ class PublicSuffixStore {
     PublicSuffixStore() = default;
 
     bool platformIsPublicSuffix(StringView domain) const;
-    String platformTopPrivatelyControlledDomain(StringView host) const;
+    String platformTopPrivatelyControlledDomain(const String& host) const;
 
     mutable Lock m_HostTopPrivatelyControlledDomainCacheLock;
     mutable HashMap<String, String, ASCIICaseInsensitiveHash> m_hostTopPrivatelyControlledDomainCache WTF_GUARDED_BY_LOCK(m_HostTopPrivatelyControlledDomainCacheLock);
 #if PLATFORM(COCOA)
     mutable Lock m_publicSuffixCacheLock;
     std::optional<HashSet<String, ASCIICaseInsensitiveHash>> m_publicSuffixCache WTF_GUARDED_BY_LOCK(m_publicSuffixCacheLock);
-    bool m_canAcceptCustomPublicSuffix { false }; // Only used on the main thread.
+    bool m_canAcceptCustomPublicSuffix WTF_GUARDED_BY_LOCK(m_publicSuffixCacheLock) { false };
 #endif
 };
 

Source/WebCore/platform/cocoa/PublicSuffixStoreCocoa.mm

@@ -27,64 +27,63 @@
 #import "PublicSuffixStore.h"
 
 #import <pal/spi/cf/CFNetworkSPI.h>
-#import <wtf/URLHelpers.h>
 #import <wtf/cocoa/NSURLExtras.h>
-#import <wtf/text/StringHash.h>
 
 namespace WebCore {
 
-static bool isPublicSuffixCF(StringView domain)
+static bool isPublicSuffixCF(const String& domain)
 {
-    if (auto host = WTF::URLHelpers::mapHostName(domain.toStringWithoutCopying(), nullptr))
-        return !host->isNull() ? _CFHostIsDomainTopLevel(host->createCFString().get()) : _CFHostIsDomainTopLevel(domain.createCFStringWithoutCopying().get());
-    return false;
+    NSString *host = WTF::decodeHostName(domain);
+    return host && _CFHostIsDomainTopLevel((__bridge CFStringRef)host);
 }
 
 bool PublicSuffixStore::platformIsPublicSuffix(StringView domain) const
 {
+    auto domainString = domain.toStringWithoutCopying();
     {
         Locker locker { m_publicSuffixCacheLock };
-        if (m_publicSuffixCache && m_publicSuffixCache->contains<ASCIICaseInsensitiveStringViewHashTranslator>(domain))
+        if (m_publicSuffixCache && m_publicSuffixCache->contains(domainString))
             return true;
     }
 
-    return isPublicSuffixCF(domain);
+    return isPublicSuffixCF(domainString);
 }
 
-String PublicSuffixStore::platformTopPrivatelyControlledDomain(StringView host) const
+String PublicSuffixStore::platformTopPrivatelyControlledDomain(const String& host) const
 {
     size_t separatorPosition;
     for (unsigned labelStart = 0; (separatorPosition = host.find('.', labelStart)) != notFound; labelStart = separatorPosition + 1) {
         if (isPublicSuffix(host.substring(separatorPosition + 1)))
-            return host.substring(labelStart).toString();
+            return host.substring(labelStart);
     }
 
     return { };
 }
 
 void PublicSuffixStore::enablePublicSuffixCache(CanAcceptCustomPublicSuffix canAcceptCustomPublicSuffix)
 {
-    ASSERT(isMainThread());
-
-    m_canAcceptCustomPublicSuffix = canAcceptCustomPublicSuffix == CanAcceptCustomPublicSuffix::Yes;
-
     Locker locker { m_publicSuffixCacheLock };
     ASSERT(!m_publicSuffixCache);
+
+    m_canAcceptCustomPublicSuffix = (canAcceptCustomPublicSuffix == CanAcceptCustomPublicSuffix::Yes);
     m_publicSuffixCache = HashSet<String, ASCIICaseInsensitiveHash> { };
 }
 
 void PublicSuffixStore::addPublicSuffix(const String& publicSuffix)
 {
-    ASSERT(isMainThread());
     if (publicSuffix.isEmpty())
         return;
 
+    Locker locker { m_publicSuffixCacheLock };
+
+    if (!m_publicSuffixCache)
+        return;
+
     if (LIKELY(!m_canAcceptCustomPublicSuffix))
         RELEASE_ASSERT(isPublicSuffixCF(publicSuffix));
 
-    Locker locker { m_publicSuffixCacheLock };
-    ASSERT(m_publicSuffixCache);
-    m_publicSuffixCache->add(crossThreadCopy(publicSuffix));
+    if (m_publicSuffixCache)
+        m_publicSuffixCache->add(crossThreadCopy(publicSuffix));
 }
 
 } // namespace WebCore

Source/WebCore/platform/network/curl/PublicSuffixStoreCurl.cpp

@@ -50,7 +50,7 @@ static String topPrivatelyControlledDomainInternal(const psl_ctx_t* psl, const c
     return String();
 }
 
-String PublicSuffixStore::platformTopPrivatelyControlledDomain(StringView domain) const
+String PublicSuffixStore::platformTopPrivatelyControlledDomain(const String& domain) const
 {
     if (platformIsPublicSuffix(domain))
         return String();

Source/WebCore/platform/soup/PublicSuffixStoreSoup.cpp

@@ -39,7 +39,7 @@ bool PublicSuffixStore::platformIsPublicSuffix(StringView domain) const
     return soup_tld_domain_is_public_suffix(domain.convertToASCIILowercase().utf8().data());
 }
 
-static String permissiveTopPrivateDomain(StringView domain)
+static String permissiveTopPrivateDomain(const String& domain)
 {
     auto position = domain.length();
     bool foundDot = false;
@@ -50,15 +50,15 @@ static String permissiveTopPrivateDomain(StringView domain)
     while (position-- > 0) {
         if (domain[position] == '.') {
             if (foundDot)
-                return domain.substring(position + 1).toString();
+                return domain.substring(position + 1);
             foundDot = true;
         }
     }
 
-    return foundDot ? domain.toString() : String();
+    return foundDot ? domain : String();
 }
 
-String PublicSuffixStore::platformTopPrivatelyControlledDomain(StringView domain) const
+String PublicSuffixStore::platformTopPrivatelyControlledDomain(const String& domain) const
 {
     CString domainUTF8 = domain.utf8();
 
@@ -84,7 +84,7 @@ String PublicSuffixStore::platformTopPrivatelyControlledDomain(StringView domain
         return String();
 
     if (g_error_matches(error.get(), SOUP_TLD_ERROR, SOUP_TLD_ERROR_IS_IP_ADDRESS))
-        return domain.toString();
+        return domain;
 
     ASSERT_NOT_REACHED();
     return String();

Source/WebKit/UIProcess/WebPageProxy.cpp

@@ -9740,7 +9740,7 @@ void WebPageProxy::resetStateAfterProcessTermination(ProcessTerminationReason re
 
 #if PLATFORM(IOS_FAMILY)
     if (m_process->isUnderMemoryPressure()) {
-        auto domain = WebCore::PublicSuffixStore::singleton().topPrivatelyControlledDomain(URL({ }, currentURL()).host());
+        auto domain = WebCore::PublicSuffixStore::singleton().topPrivatelyControlledDomain(URL({ }, currentURL()).host().toString());
         if (!domain.isEmpty())
             logDiagnosticMessageWithEnhancedPrivacy(WebCore::DiagnosticLoggingKeys::domainCausingJetsamKey(), domain, WebCore::ShouldSample::No);
     }

Source/WebKit/UIProcess/WebProcessProxy.cpp

@@ -1212,7 +1212,7 @@ void WebProcessProxy::processDidTerminateOrFailedToLaunch(ProcessTerminationReas
     // FIXME: Perhaps this should consider ProcessTerminationReasons ExceededMemoryLimit, ExceededCPULimit, Unresponsive as well.
     if (pages.size() == 1 && reason == ProcessTerminationReason::Crash) {
         auto& page = pages[0];
-        auto domain = PublicSuffixStore::singleton().topPrivatelyControlledDomain(URL({ }, page->currentURL()).host());
+        auto domain = PublicSuffixStore::singleton().topPrivatelyControlledDomain(URL({ }, page->currentURL()).host().toString());
         if (!domain.isEmpty())
             page->logDiagnosticMessageWithEnhancedPrivacy(WebCore::DiagnosticLoggingKeys::domainCausingCrashKey(), domain, WebCore::ShouldSample::No);
     }
@@ -2015,7 +2015,7 @@ void WebProcessProxy::didExceedMemoryFootprintThreshold(size_t footprint)
     bool hasAllowedToRunInTheBackgroundActivity = false;
 
     for (auto& page : this->pages()) {
-        auto pageDomain = PublicSuffixStore::singleton().topPrivatelyControlledDomain(URL({ }, page->currentURL()).host());
+        auto pageDomain = PublicSuffixStore::singleton().topPrivatelyControlledDomain(URL({ }, page->currentURL()).host().toString());
         if (domain.isEmpty())
             domain = WTFMove(pageDomain);
         else if (domain != pageDomain)