Changeset 230044 in webkit

Timestamp:

Mar 28, 2018 1:36:03 PM (6 years ago)

Author:

Chris Dumez

Message:

Thread safety issue in IDBFactory' shouldThrowSecurityException()
​https://bugs.webkit.org/show_bug.cgi?id=184064

Reviewed by Ryosuke Niwa.

shouldThrowSecurityException() gets called on a non-main thread but
it ended up using the SchemeRegistry via SecurityOrigin::canAccessDatabase()
which calls SecurityOrigin::isLocal().

Since using the SchemeRegistry from the background thread is not safe
(we recently added locks which we're trying to remove), and since SecurityOrigin
methods are often called from background threads, this patch make SecurityOrigin::isLocal()
safe to call from a background thread. To achieve this, we now query the SchemeRegistry
in the SecurityOrigin constructor instead as SecurityOrigin objects are expected to be
constructed on the main thread.

• page/SecurityOrigin.cpp:

(WebCore::SecurityOrigin::SecurityOrigin):
(WebCore::SecurityOrigin::isLocal const): Deleted.

• page/SecurityOrigin.h:

(WebCore::SecurityOrigin::isLocal const):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• page/SecurityOrigin.cpp (modified) (3 diffs)
• page/SecurityOrigin.h (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2018-03-28  Chris Dumez  <cdumez@apple.com>
+
+        Thread safety issue in IDBFactory' shouldThrowSecurityException()
+        https://bugs.webkit.org/show_bug.cgi?id=184064
+
+        Reviewed by Ryosuke Niwa.
+
+        shouldThrowSecurityException() gets called on a non-main thread but
+        it ended up using the SchemeRegistry via SecurityOrigin::canAccessDatabase()
+        which calls SecurityOrigin::isLocal().
+
+        Since using the SchemeRegistry from the background thread is not safe
+        (we recently added locks which we're trying to remove), and since SecurityOrigin
+        methods are often called from background threads, this patch make SecurityOrigin::isLocal()
+        safe to call from a background thread. To achieve this, we now query the SchemeRegistry
+        in the SecurityOrigin constructor instead as SecurityOrigin objects are expected to be
+        constructed on the main thread.
+
+        * page/SecurityOrigin.cpp:
+        (WebCore::SecurityOrigin::SecurityOrigin):
+        (WebCore::SecurityOrigin::isLocal const): Deleted.
+        * page/SecurityOrigin.h:
+        (WebCore::SecurityOrigin::isLocal const):
+
 2018-03-28  Ryan Haddad  <ryanhaddad@apple.com>
 

trunk/Source/WebCore/page/SecurityOrigin.cpp

@@ -147 +147 @@
 SecurityOrigin::SecurityOrigin(const URL& url)
     : m_data(SecurityOriginData::fromURL(url))
+    , m_isLocal(SchemeRegistry::shouldTreatURLSchemeAsLocal(m_data.protocol))
 {
     // document.domain starts as m_data.host, but can be set by the DOM.
@@ -183 +184 @@
     , m_needsStorageAccessFromFileURLsQuirk { other->m_needsStorageAccessFromFileURLsQuirk }
     , m_isPotentiallyTrustworthy { other->m_isPotentiallyTrustworthy }
+    , m_isLocal { other->m_isLocal }
 {
 }
@@ -459 +461 @@
 }
 
-bool SecurityOrigin::isLocal() const
-{
-    return SchemeRegistry::shouldTreatURLSchemeAsLocal(m_data.protocol);
-}
-
 String SecurityOrigin::toString() const
 {

trunk/Source/WebCore/page/SecurityOrigin.h

@@ -154 +154 @@
     // The local SecurityOrigin can script any document, navigate to local
     // resources, and can set arbitrary headers on XMLHttpRequests.
-    WEBCORE_EXPORT bool isLocal() const;
+    bool isLocal() const { return m_isLocal; }
 
     // The origin is a globally unique identifier assigned when the Document is
@@ -235 +235 @@
     bool m_needsStorageAccessFromFileURLsQuirk { false };
     bool m_isPotentiallyTrustworthy { false };
+    bool m_isLocal { false };
 };