Remove mutex locks in SchemeRegistry

third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp

Index: third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp
diff --git a/third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp b/third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp
index 202b35019696c530528b1f38f515eedaac1fb819..b3c6d8699266caa8053745301ed35306360a1f02 100644
--- a/third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp
+++ b/third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp
@@ -27,27 +27,14 @@
 #include "platform/weborigin/SchemeRegistry.h"
 
 #include "wtf/ThreadSpecific.h"
+#include "wtf/Threading.h"
 #include "wtf/ThreadingPrimitives.h"
 #include "wtf/text/StringBuilder.h"
 
 namespace blink {
 
-static Mutex& mutex() {
-  // The first call to this should be made before or during blink
-  // initialization to avoid racy static local initialization.
-  DEFINE_STATIC_LOCAL(Mutex, m, ());
-  return m;
-}
-
-// Defines static local variable after making sure that a lock is held.
-// (We can't use DEFINE_STATIC_LOCAL for this because it asserts thread
-// safety, which is externally guaranteed by the local mutex() lock)
-#define DEFINE_STATIC_LOCAL_WITH_LOCK(type, name, arguments) \
-  ASSERT(mutex().locked());                                  \
-  static type& name = *new type arguments
-
 static URLSchemesSet& localURLSchemes() {
-  DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesSet, localSchemes, ());
+  DEFINE_STATIC_LOCAL(URLSchemesSet, localSchemes, ());
 
   if (localSchemes.isEmpty())
     localSchemes.add("file");
@@ -56,66 +43,63 @@ static URLSchemesSet& localURLSchemes() {
 }
 
 static URLSchemesSet& displayIsolatedURLSchemes() {
-  DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesSet, displayIsolatedSchemes, ());
+  DEFINE_STATIC_LOCAL(URLSchemesSet, displayIsolatedSchemes, ());
   return displayIsolatedSchemes;
 }
 
 static URLSchemesSet& mixedContentRestrictingSchemes() {
-  DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesSet, mixedContentRestrictingSchemes,
-                                ({
-                                    "https",
-                                }));
+  DEFINE_STATIC_LOCAL(URLSchemesSet, mixedContentRestrictingSchemes,
+                      ({
+                          "https",
+                      }));
   return mixedContentRestrictingSchemes;
 }
 
 static URLSchemesSet& secureSchemes() {
-  DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesSet, secureSchemes,
-                                ({
-                                    "https", "about", "data", "wss",
-                                }));
+  DEFINE_STATIC_LOCAL(URLSchemesSet, secureSchemes,
+                      ({
+                          "https", "about", "data", "wss",
+                      }));
   return secureSchemes;
 }
 
 static URLSchemesSet& schemesWithUniqueOrigins() {
-  DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesSet, schemesWithUniqueOrigins,
-                                ({
-                                    "about", "javascript", "data",
-                                }));
+  DEFINE_STATIC_LOCAL(URLSchemesSet, schemesWithUniqueOrigins,
+                      ({
+                          "about", "javascript", "data",
+                      }));
   return schemesWithUniqueOrigins;
 }
 
 static URLSchemesSet& emptyDocumentSchemes() {
-  DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesSet, emptyDocumentSchemes,
-                                ({
-                                    "about",
-                                }));
+  DEFINE_STATIC_LOCAL(URLSchemesSet, emptyDocumentSchemes, ({
+                                                               "about",
+                                                           }));
   return emptyDocumentSchemes;
 }
 
 static HashSet<String>& schemesForbiddenFromDomainRelaxation() {
-  DEFINE_STATIC_LOCAL_WITH_LOCK(HashSet<String>, schemes, ());
+  DEFINE_STATIC_LOCAL(HashSet<String>, schemes, ());
   return schemes;
 }
 
 static URLSchemesSet& notAllowingJavascriptURLsSchemes() {
-  DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesSet, notAllowingJavascriptURLsSchemes,
-                                ());
+  DEFINE_STATIC_LOCAL(URLSchemesSet, notAllowingJavascriptURLsSchemes, ());
   return notAllowingJavascriptURLsSchemes;
 }
 
 void SchemeRegistry::registerURLSchemeAsLocal(const String& scheme) {
+  DCHECK(WTF::isBeforeThreadCreated());

[kinuko, 2016/11/06 11:14:25]

Given that we're deprecating ASSERT could we change WTF::isBeforeThreadCreated()
to be defined if ENABLE(ASSERT) || DCHECK_IS_ON()?

(Or only when DCHECK_IS_ON and change callsites to only use DCHECK)

[Charlie Harrison, 2016/11/07 17:57:48]

Unfortunately, it seems like DCHECK requires its arguments to exist during
compilation. Your suggestions hit the same problems :/ We could just stop gating
isBeforeThreadCreated() and friends. It's only a few methods and a single global
bool... WDYT?

   DCHECK_EQ(scheme, scheme.lower());
-  MutexLocker locker(mutex());
   localURLSchemes().add(scheme);
 }
 
 const URLSchemesSet& SchemeRegistry::localSchemes() {
-  MutexLocker locker(mutex());
   return localURLSchemes();
 }
 
 static URLSchemesSet& CORSEnabledSchemes() {
-  DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesSet, CORSEnabledSchemes, ());
+  DEFINE_STATIC_LOCAL(URLSchemesSet, CORSEnabledSchemes, ());
 
   if (CORSEnabledSchemes.isEmpty()) {
     CORSEnabledSchemes.add("http");
@@ -127,7 +111,7 @@ static URLSchemesSet& CORSEnabledSchemes() {
 }
 
 static URLSchemesSet& serviceWorkerSchemes() {
-  DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesSet, serviceWorkerSchemes, ());
+  DEFINE_STATIC_LOCAL(URLSchemesSet, serviceWorkerSchemes, ());
 
   if (serviceWorkerSchemes.isEmpty()) {
     // HTTP is required because http://localhost is considered secure.
@@ -141,7 +125,7 @@ static URLSchemesSet& serviceWorkerSchemes() {
 }
 
 static URLSchemesSet& fetchAPISchemes() {
-  DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesSet, fetchAPISchemes, ());
+  DEFINE_STATIC_LOCAL(URLSchemesSet, fetchAPISchemes, ());
 
   if (fetchAPISchemes.isEmpty()) {
     fetchAPISchemes.add("http");
@@ -152,26 +136,23 @@ static URLSchemesSet& fetchAPISchemes() {
 }
 
 static URLSchemesSet& firstPartyWhenTopLevelSchemes() {
-  DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesSet, firstPartyWhenTopLevelSchemes,
-                                ());
+  DEFINE_STATIC_LOCAL(URLSchemesSet, firstPartyWhenTopLevelSchemes, ());
   return firstPartyWhenTopLevelSchemes;
 }
 
 static URLSchemesMap<SchemeRegistry::PolicyAreas>&
 ContentSecurityPolicyBypassingSchemes() {
-  DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesMap<SchemeRegistry::PolicyAreas>,
-                                schemes, ());
+  DEFINE_STATIC_LOCAL(URLSchemesMap<SchemeRegistry::PolicyAreas>, schemes, ());
   return schemes;
 }
 
 static URLSchemesSet& secureContextBypassingSchemes() {
-  DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesSet, secureContextBypassingSchemes,
-                                ());
+  DEFINE_STATIC_LOCAL(URLSchemesSet, secureContextBypassingSchemes, ());
   return secureContextBypassingSchemes;
 }
 
 static URLSchemesSet& allowedInReferrerSchemes() {
-  DEFINE_STATIC_LOCAL_WITH_LOCK(URLSchemesSet, allowedInReferrerSchemes, ());
+  DEFINE_STATIC_LOCAL(URLSchemesSet, allowedInReferrerSchemes, ());
 
   if (allowedInReferrerSchemes.isEmpty()) {
     allowedInReferrerSchemes.add("http");
@@ -182,21 +163,33 @@ static URLSchemesSet& allowedInReferrerSchemes() {
 }
 
 void SchemeRegistry::initialize() {

[kinuko, 2016/11/06 11:14:25]

There're a lot :(  Should we quickly check if it doesn't add much initialization
cost?

I feel like cleaning up these a little further, but for now let's focus on one
thing in this patch...

[Charlie Harrison, 2016/11/07 17:30:31]

I locally added a SCOPED_BLINK_UMA_HISTOGRAM_TIMER to time this method on a
Nexus 7. It takes on average 40-50 us on renderer startup. Is that acceptable?

-  // Instantiate the mutex object.
-  mutex();
+  localURLSchemes();
+  displayIsolatedURLSchemes();
+  mixedContentRestrictingSchemes();
+  secureSchemes();
+  schemesWithUniqueOrigins();
+  emptyDocumentSchemes();
+  schemesForbiddenFromDomainRelaxation();
+  notAllowingJavascriptURLsSchemes();
+  CORSEnabledSchemes();
+  serviceWorkerSchemes();
+  fetchAPISchemes();
+  firstPartyWhenTopLevelSchemes();
+  ContentSecurityPolicyBypassingSchemes();
+  secureContextBypassingSchemes();
+  allowedInReferrerSchemes();
 }
 
 bool SchemeRegistry::shouldTreatURLSchemeAsLocal(const String& scheme) {
   DCHECK_EQ(scheme, scheme.lower());
   if (scheme.isEmpty())
     return false;
-  MutexLocker locker(mutex());
   return localURLSchemes().contains(scheme);
 }
 
 void SchemeRegistry::registerURLSchemeAsNoAccess(const String& scheme) {
+  DCHECK(WTF::isBeforeThreadCreated());
   DCHECK_EQ(scheme, scheme.lower());
-  MutexLocker locker(mutex());
   schemesWithUniqueOrigins().add(scheme);
 }
 
@@ -204,13 +197,12 @@ bool SchemeRegistry::shouldTreatURLSchemeAsNoAccess(const String& scheme) {
   DCHECK_EQ(scheme, scheme.lower());
   if (scheme.isEmpty())
     return false;
-  MutexLocker locker(mutex());
   return schemesWithUniqueOrigins().contains(scheme);
 }
 
 void SchemeRegistry::registerURLSchemeAsDisplayIsolated(const String& scheme) {
+  DCHECK(WTF::isBeforeThreadCreated());
   DCHECK_EQ(scheme, scheme.lower());
-  MutexLocker locker(mutex());
   displayIsolatedURLSchemes().add(scheme);
 }
 
@@ -219,14 +211,13 @@ bool SchemeRegistry::shouldTreatURLSchemeAsDisplayIsolated(
   DCHECK_EQ(scheme, scheme.lower());
   if (scheme.isEmpty())
     return false;
-  MutexLocker locker(mutex());
   return displayIsolatedURLSchemes().contains(scheme);
 }
 
 void SchemeRegistry::registerURLSchemeAsRestrictingMixedContent(
     const String& scheme) {
+  DCHECK(WTF::isBeforeThreadCreated());
   DCHECK_EQ(scheme, scheme.lower());
-  MutexLocker locker(mutex());
   mixedContentRestrictingSchemes().add(scheme);
 }
 
@@ -235,13 +226,12 @@ bool SchemeRegistry::shouldTreatURLSchemeAsRestrictingMixedContent(
   DCHECK_EQ(scheme, scheme.lower());
   if (scheme.isEmpty())
     return false;
-  MutexLocker locker(mutex());
   return mixedContentRestrictingSchemes().contains(scheme);
 }
 
 void SchemeRegistry::registerURLSchemeAsSecure(const String& scheme) {
+  DCHECK(WTF::isBeforeThreadCreated());
   DCHECK_EQ(scheme, scheme.lower());
-  MutexLocker locker(mutex());
   secureSchemes().add(scheme);
 }
 
@@ -249,13 +239,12 @@ bool SchemeRegistry::shouldTreatURLSchemeAsSecure(const String& scheme) {
   DCHECK_EQ(scheme, scheme.lower());
   if (scheme.isEmpty())
     return false;
-  MutexLocker locker(mutex());
   return secureSchemes().contains(scheme);
 }
 
 void SchemeRegistry::registerURLSchemeAsEmptyDocument(const String& scheme) {
+  DCHECK(WTF::isBeforeThreadCreated());
   DCHECK_EQ(scheme, scheme.lower());
-  MutexLocker locker(mutex());
   emptyDocumentSchemes().add(scheme);
 }
 
@@ -263,18 +252,17 @@ bool SchemeRegistry::shouldLoadURLSchemeAsEmptyDocument(const String& scheme) {
   DCHECK_EQ(scheme, scheme.lower());
   if (scheme.isEmpty())
     return false;
-  MutexLocker locker(mutex());
   return emptyDocumentSchemes().contains(scheme);
 }
 
 void SchemeRegistry::setDomainRelaxationForbiddenForURLScheme(
     bool forbidden,
     const String& scheme) {
+  DCHECK(WTF::isBeforeThreadCreated());
   DCHECK_EQ(scheme, scheme.lower());
   if (scheme.isEmpty())
     return;
 
-  MutexLocker locker(mutex());
   if (forbidden)
     schemesForbiddenFromDomainRelaxation().add(scheme);
   else
@@ -286,7 +274,6 @@ bool SchemeRegistry::isDomainRelaxationForbiddenForURLScheme(
   DCHECK_EQ(scheme, scheme.lower());
   if (scheme.isEmpty())
     return false;
-  MutexLocker locker(mutex());
   return schemesForbiddenFromDomainRelaxation().contains(scheme);
 }
 
@@ -297,8 +284,8 @@ bool SchemeRegistry::canDisplayOnlyIfCanRequest(const String& scheme) {
 
 void SchemeRegistry::registerURLSchemeAsNotAllowingJavascriptURLs(
     const String& scheme) {
+  DCHECK(WTF::isBeforeThreadCreated());
   DCHECK_EQ(scheme, scheme.lower());
-  MutexLocker locker(mutex());
   notAllowingJavascriptURLsSchemes().add(scheme);
 }
 
@@ -307,13 +294,12 @@ bool SchemeRegistry::shouldTreatURLSchemeAsNotAllowingJavascriptURLs(
   DCHECK_EQ(scheme, scheme.lower());
   if (scheme.isEmpty())
     return false;
-  MutexLocker locker(mutex());
   return notAllowingJavascriptURLsSchemes().contains(scheme);
 }
 
 void SchemeRegistry::registerURLSchemeAsCORSEnabled(const String& scheme) {
+  DCHECK(WTF::isBeforeThreadCreated());
   DCHECK_EQ(scheme, scheme.lower());
-  MutexLocker locker(mutex());
   CORSEnabledSchemes().add(scheme);
 }
 
@@ -321,19 +307,13 @@ bool SchemeRegistry::shouldTreatURLSchemeAsCORSEnabled(const String& scheme) {
   DCHECK_EQ(scheme, scheme.lower());
   if (scheme.isEmpty())
     return false;
-  MutexLocker locker(mutex());
   return CORSEnabledSchemes().contains(scheme);
 }
 
 String SchemeRegistry::listOfCORSEnabledURLSchemes() {
   StringBuilder builder;
   bool addSeparator = false;
-  URLSchemesSet schemes;
-  {
-    MutexLocker locker(mutex());
-    schemes = CORSEnabledSchemes();
-  }
-  for (const auto& scheme : schemes) {
+  for (const auto& scheme : CORSEnabledSchemes()) {
     if (addSeparator)
       builder.append(", ");
     else
@@ -350,8 +330,8 @@ bool SchemeRegistry::shouldTreatURLSchemeAsLegacy(const String& scheme) {
 
 void SchemeRegistry::registerURLSchemeAsAllowingServiceWorkers(
     const String& scheme) {
+  DCHECK(WTF::isBeforeThreadCreated());
   DCHECK_EQ(scheme, scheme.lower());
-  MutexLocker locker(mutex());
   serviceWorkerSchemes().add(scheme);
 }
 
@@ -360,14 +340,13 @@ bool SchemeRegistry::shouldTreatURLSchemeAsAllowingServiceWorkers(
   DCHECK_EQ(scheme, scheme.lower());
   if (scheme.isEmpty())
     return false;
-  MutexLocker locker(mutex());
   return serviceWorkerSchemes().contains(scheme);
 }
 
 void SchemeRegistry::registerURLSchemeAsSupportingFetchAPI(
     const String& scheme) {
+  DCHECK(WTF::isBeforeThreadCreated());
   DCHECK_EQ(scheme, scheme.lower());
-  MutexLocker locker(mutex());
   fetchAPISchemes().add(scheme);
 }
 
@@ -376,21 +355,20 @@ bool SchemeRegistry::shouldTreatURLSchemeAsSupportingFetchAPI(
   DCHECK_EQ(scheme, scheme.lower());
   if (scheme.isEmpty())
     return false;
-  MutexLocker locker(mutex());
   return fetchAPISchemes().contains(scheme);
 }
 
 void SchemeRegistry::registerURLSchemeAsFirstPartyWhenTopLevel(
     const String& scheme) {
+  DCHECK(WTF::isBeforeThreadCreated());
   DCHECK_EQ(scheme, scheme.lower());
-  MutexLocker locker(mutex());
   firstPartyWhenTopLevelSchemes().add(scheme);
 }
 
 void SchemeRegistry::removeURLSchemeAsFirstPartyWhenTopLevel(
     const String& scheme) {
+  DCHECK(WTF::isBeforeThreadCreated());
   DCHECK_EQ(scheme, scheme.lower());
-  MutexLocker locker(mutex());
   firstPartyWhenTopLevelSchemes().remove(scheme);
 }
 
@@ -399,19 +377,18 @@ bool SchemeRegistry::shouldTreatURLSchemeAsFirstPartyWhenTopLevel(
   DCHECK_EQ(scheme, scheme.lower());
   if (scheme.isEmpty())
     return false;
-  MutexLocker locker(mutex());
   return firstPartyWhenTopLevelSchemes().contains(scheme);
 }
 
 void SchemeRegistry::registerURLSchemeAsAllowedForReferrer(
     const String& scheme) {
+  DCHECK(WTF::isBeforeThreadCreated());
   DCHECK_EQ(scheme, scheme.lower());
-  MutexLocker locker(mutex());
   allowedInReferrerSchemes().add(scheme);
 }
 
 void SchemeRegistry::removeURLSchemeAsAllowedForReferrer(const String& scheme) {
-  MutexLocker locker(mutex());
+  DCHECK(WTF::isBeforeThreadCreated());
   allowedInReferrerSchemes().remove(scheme);
 }
 
@@ -420,22 +397,21 @@ bool SchemeRegistry::shouldTreatURLSchemeAsAllowedForReferrer(
   DCHECK_EQ(scheme, scheme.lower());
   if (scheme.isEmpty())
     return false;
-  MutexLocker locker(mutex());
   return allowedInReferrerSchemes().contains(scheme);
 }
 
 void SchemeRegistry::registerURLSchemeAsBypassingContentSecurityPolicy(
     const String& scheme,
     PolicyAreas policyAreas) {
+  DCHECK(WTF::isBeforeThreadCreated());
   DCHECK_EQ(scheme, scheme.lower());
-  MutexLocker locker(mutex());
   ContentSecurityPolicyBypassingSchemes().add(scheme, policyAreas);
 }
 
 void SchemeRegistry::removeURLSchemeRegisteredAsBypassingContentSecurityPolicy(
     const String& scheme) {
+  DCHECK(WTF::isBeforeThreadCreated());
   DCHECK_EQ(scheme, scheme.lower());
-  MutexLocker locker(mutex());
   ContentSecurityPolicyBypassingSchemes().remove(scheme);
 }
 
@@ -448,15 +424,14 @@ bool SchemeRegistry::schemeShouldBypassContentSecurityPolicy(
 
   // get() returns 0 (PolicyAreaNone) if there is no entry in the map.
   // Thus by default, schemes do not bypass CSP.
-  MutexLocker locker(mutex());
   return (ContentSecurityPolicyBypassingSchemes().get(scheme) & policyAreas) ==
          policyAreas;
 }
 
 void SchemeRegistry::registerURLSchemeBypassingSecureContextCheck(
     const String& scheme) {
+  DCHECK(WTF::isBeforeThreadCreated());
   DCHECK_EQ(scheme, scheme.lower());
-  MutexLocker locker(mutex());
   secureContextBypassingSchemes().add(scheme.lower());
 }
 
@@ -464,7 +439,6 @@ bool SchemeRegistry::schemeShouldBypassSecureContextCheck(
     const String& scheme) {
   if (scheme.isEmpty())
     return false;
-  MutexLocker locker(mutex());
   return secureContextBypassingSchemes().contains(scheme.lower());
 }