Changeset 247250 in webkit

Timestamp:

Jul 8, 2019 8:06:12 PM (5 years ago)

Author:

Chris Dumez

Message:

Speculative fix for crashes under LocalStorageDatabaseTracker::databasePath()
​https://bugs.webkit.org/show_bug.cgi?id=199599
<rdar://problem/31169686>

Reviewed by Ryosuke Niwa.

Speculative fix for crashes under LocalStorageDatabaseTracker::databasePath():

• Add new localStorageDirectory() getter to LocalStorageDatabaseTracker which calls isolatedCopy() on m_localStorageDirectory before returning it. Use it everywhere instead of m_localStorageDirectory since it is not safe to use the same String from various threads like it was done.
• Move localStorageDirectory when constructing the LocalStorageDatabaseTracker instead of copying it.
• Make sure that LocalStorageDatabaseTracker and StorageManager are both constructed and destroyed on the main thread.

• NetworkProcess/NetworkSession.cpp:

(WebKit::NetworkSession::NetworkSession):

• NetworkProcess/NetworkSession.h:
• NetworkProcess/WebStorage/LocalStorageDatabaseTracker.cpp:

(WebKit::LocalStorageDatabaseTracker::create):
(WebKit::LocalStorageDatabaseTracker::LocalStorageDatabaseTracker):
(WebKit::LocalStorageDatabaseTracker::localStorageDirectory const):
(WebKit::LocalStorageDatabaseTracker::~LocalStorageDatabaseTracker):
(WebKit::LocalStorageDatabaseTracker::deleteAllDatabases):
(WebKit::LocalStorageDatabaseTracker::origins const):
(WebKit::LocalStorageDatabaseTracker::databasePath const):

• NetworkProcess/WebStorage/LocalStorageDatabaseTracker.h:
• NetworkProcess/WebStorage/StorageManager.cpp:

(WebKit::StorageManager::create):
(WebKit::StorageManager::StorageManager):
(WebKit::StorageManager::~StorageManager):

• NetworkProcess/WebStorage/StorageManager.h:
• NetworkProcess/WebStorage/ios/LocalStorageDatabaseTrackerIOS.mm:

(WebKit::LocalStorageDatabaseTracker::platformMaybeExcludeFromBackup const):

• NetworkProcess/cocoa/NetworkSessionCocoa.mm:

(WebKit::NetworkSessionCocoa::NetworkSessionCocoa):

• NetworkProcess/curl/NetworkSessionCurl.cpp:

(WebKit::NetworkSessionCurl::NetworkSessionCurl):

• NetworkProcess/soup/NetworkSessionSoup.cpp:

(WebKit::NetworkSessionSoup::NetworkSessionSoup):

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• NetworkProcess/NetworkSession.cpp (modified) (1 diff)
• NetworkProcess/NetworkSession.h (modified) (1 diff)
• NetworkProcess/WebStorage/LocalStorageDatabaseTracker.cpp (modified) (7 diffs)
• NetworkProcess/WebStorage/LocalStorageDatabaseTracker.h (modified) (3 diffs)
• NetworkProcess/WebStorage/StorageManager.cpp (modified) (1 diff)
• NetworkProcess/WebStorage/StorageManager.h (modified) (2 diffs)
• NetworkProcess/WebStorage/ios/LocalStorageDatabaseTrackerIOS.mm (modified) (1 diff)
• NetworkProcess/cocoa/NetworkSessionCocoa.mm (modified) (1 diff)
• NetworkProcess/curl/NetworkSessionCurl.cpp (modified) (1 diff)
• NetworkProcess/soup/NetworkSessionSoup.cpp (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-07-08  Chris Dumez  <cdumez@apple.com>
+
+        Speculative fix for crashes under LocalStorageDatabaseTracker::databasePath()
+        https://bugs.webkit.org/show_bug.cgi?id=199599
+        <rdar://problem/31169686>
+
+        Reviewed by Ryosuke Niwa.
+
+        Speculative fix for crashes under LocalStorageDatabaseTracker::databasePath():
+        - Add new localStorageDirectory() getter to LocalStorageDatabaseTracker which
+          calls isolatedCopy() on m_localStorageDirectory before returning it.
+          Use it everywhere instead of m_localStorageDirectory since it is not safe
+          to use the same String from various threads like it was done.
+        - Move localStorageDirectory when constructing the LocalStorageDatabaseTracker
+          instead of copying it.
+        - Make sure that LocalStorageDatabaseTracker and StorageManager are both
+          constructed and destroyed on the main thread.
+
+        * NetworkProcess/NetworkSession.cpp:
+        (WebKit::NetworkSession::NetworkSession):
+        * NetworkProcess/NetworkSession.h:
+        * NetworkProcess/WebStorage/LocalStorageDatabaseTracker.cpp:
+        (WebKit::LocalStorageDatabaseTracker::create):
+        (WebKit::LocalStorageDatabaseTracker::LocalStorageDatabaseTracker):
+        (WebKit::LocalStorageDatabaseTracker::localStorageDirectory const):
+        (WebKit::LocalStorageDatabaseTracker::~LocalStorageDatabaseTracker):
+        (WebKit::LocalStorageDatabaseTracker::deleteAllDatabases):
+        (WebKit::LocalStorageDatabaseTracker::origins const):
+        (WebKit::LocalStorageDatabaseTracker::databasePath const):
+        * NetworkProcess/WebStorage/LocalStorageDatabaseTracker.h:
+        * NetworkProcess/WebStorage/StorageManager.cpp:
+        (WebKit::StorageManager::create):
+        (WebKit::StorageManager::StorageManager):
+        (WebKit::StorageManager::~StorageManager):
+        * NetworkProcess/WebStorage/StorageManager.h:
+        * NetworkProcess/WebStorage/ios/LocalStorageDatabaseTrackerIOS.mm:
+        (WebKit::LocalStorageDatabaseTracker::platformMaybeExcludeFromBackup const):
+        * NetworkProcess/cocoa/NetworkSessionCocoa.mm:
+        (WebKit::NetworkSessionCocoa::NetworkSessionCocoa):
+        * NetworkProcess/curl/NetworkSessionCurl.cpp:
+        (WebKit::NetworkSessionCurl::NetworkSessionCurl):
+        * NetworkProcess/soup/NetworkSessionSoup.cpp:
+        (WebKit::NetworkSessionSoup::NetworkSessionSoup):
+
 2019-07-08  Chris Dumez  <cdumez@apple.com>
 

trunk/Source/WebKit/NetworkProcess/NetworkSession.cpp

@@ -78 +78 @@
 }
 
-NetworkSession::NetworkSession(NetworkProcess& networkProcess, PAL::SessionID sessionID, const String& localStorageDirectory, SandboxExtension::Handle& handle)
+NetworkSession::NetworkSession(NetworkProcess& networkProcess, PAL::SessionID sessionID, String&& localStorageDirectory, SandboxExtension::Handle& handle)
     : m_sessionID(sessionID)
     , m_networkProcess(networkProcess)
     , m_adClickAttribution(makeUniqueRef<AdClickAttributionManager>(sessionID))
-    , m_storageManager(StorageManager::create(localStorageDirectory))
+    , m_storageManager(StorageManager::create(WTFMove(localStorageDirectory)))
 {
     SandboxExtension::consumePermanently(handle);

trunk/Source/WebKit/NetworkProcess/NetworkSession.h

@@ -109 +109 @@
 
 protected:
-    NetworkSession(NetworkProcess&, PAL::SessionID, const String& localStorageDirectory, SandboxExtension::Handle&);
+    NetworkSession(NetworkProcess&, PAL::SessionID, String&& localStorageDirectory, SandboxExtension::Handle&);
 
     PAL::SessionID m_sessionID;

trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabaseTracker.cpp

@@ -40 +40 @@
 using namespace WebCore;
 
-Ref<LocalStorageDatabaseTracker> LocalStorageDatabaseTracker::create(Ref<WorkQueue>&& queue, const String& localStorageDirectory)
+Ref<LocalStorageDatabaseTracker> LocalStorageDatabaseTracker::create(Ref<WorkQueue>&& queue, String&& localStorageDirectory)
 {
-    return adoptRef(*new LocalStorageDatabaseTracker(WTFMove(queue), localStorageDirectory));
+    return adoptRef(*new LocalStorageDatabaseTracker(WTFMove(queue), WTFMove(localStorageDirectory)));
 }
 
-LocalStorageDatabaseTracker::LocalStorageDatabaseTracker(Ref<WorkQueue>&& queue, const String& localStorageDirectory)
+LocalStorageDatabaseTracker::LocalStorageDatabaseTracker(Ref<WorkQueue>&& queue, String&& localStorageDirectory)
     : m_queue(WTFMove(queue))
-    , m_localStorageDirectory(localStorageDirectory.isolatedCopy())
+    , m_localStorageDirectory(WTFMove(localStorageDirectory))
 {
+    ASSERT(RunLoop::isMain());
+
     // Make sure the encoding is initialized before we start dispatching things to the queue.
     UTF8Encoding();
@@ -58 +60 @@
 }
 
+String LocalStorageDatabaseTracker::localStorageDirectory() const
+{
+    return m_localStorageDirectory.isolatedCopy();
+}
+
 LocalStorageDatabaseTracker::~LocalStorageDatabaseTracker()
 {
+    ASSERT(RunLoop::isMain());
 }
 
@@ -83 +91 @@
 void LocalStorageDatabaseTracker::deleteAllDatabases()
 {
-    auto paths = FileSystem::listDirectory(m_localStorageDirectory, "*.localstorage");
+    auto localStorageDirectory = this->localStorageDirectory();
+    auto paths = FileSystem::listDirectory(localStorageDirectory, "*.localstorage");
     for (const auto& path : paths) {
         SQLiteFileSystem::deleteDatabaseFile(path);
@@ -90 +99 @@
     }
 
-    SQLiteFileSystem::deleteEmptyDatabaseDirectory(m_localStorageDirectory);
+    SQLiteFileSystem::deleteEmptyDatabaseDirectory(localStorageDirectory);
 }
 
@@ -115 +124 @@
 {
     Vector<SecurityOriginData> databaseOrigins;
-    auto paths = FileSystem::listDirectory(m_localStorageDirectory, "*.localstorage");
+    auto paths = FileSystem::listDirectory(localStorageDirectory(), "*.localstorage");
     
     for (const auto& path : paths) {
@@ -151 +160 @@
 String LocalStorageDatabaseTracker::databasePath(const String& filename) const
 {
-    if (!SQLiteFileSystem::ensureDatabaseDirectoryExists(m_localStorageDirectory)) {
+    auto localStorageDirectory = this->localStorageDirectory();
+    if (!SQLiteFileSystem::ensureDatabaseDirectoryExists(localStorageDirectory)) {
         if (!m_localStorageDirectory.isNull())
             LOG_ERROR("Unable to create LocalStorage database path %s", m_localStorageDirectory.utf8().data());
@@ -163 +173 @@
 #endif
 
-    return SQLiteFileSystem::appendDatabaseFileNameToPath(m_localStorageDirectory, filename);
+    return SQLiteFileSystem::appendDatabaseFileNameToPath(localStorageDirectory, filename);
 }
 

trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabaseTracker.h

@@ -36 +36 @@
 namespace WebKit {
 
-class LocalStorageDatabaseTracker : public ThreadSafeRefCounted<LocalStorageDatabaseTracker> {
+class LocalStorageDatabaseTracker : public ThreadSafeRefCounted<LocalStorageDatabaseTracker, WTF::DestructionThread::MainRunLoop> {
 public:
-    static Ref<LocalStorageDatabaseTracker> create(Ref<WorkQueue>&&, const String& localStorageDirectory);
+    static Ref<LocalStorageDatabaseTracker> create(Ref<WorkQueue>&&, String&& localStorageDirectory);
     ~LocalStorageDatabaseTracker();
 
@@ -65 +65 @@
 
 private:
-    LocalStorageDatabaseTracker(Ref<WorkQueue>&&, const String& localStorageDirectory);
+    LocalStorageDatabaseTracker(Ref<WorkQueue>&&, String&& localStorageDirectory);
 
     String databasePath(const String& filename) const;
+    String localStorageDirectory() const;
 
     enum DatabaseOpeningStrategy {
@@ -75 +76 @@
 
     Ref<WorkQueue> m_queue;
-    String m_localStorageDirectory;
+    
+    // It is not safe to use this member from a background thread, call localStorageDirectory() instead.
+    const String m_localStorageDirectory;
 
 #if PLATFORM(IOS_FAMILY)

trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp

@@ -482 +482 @@
 }
 
-Ref<StorageManager> StorageManager::create(const String& localStorageDirectory)
-{
-    return adoptRef(*new StorageManager(localStorageDirectory));
-}
-
-StorageManager::StorageManager(const String& localStorageDirectory)
+Ref<StorageManager> StorageManager::create(String&& localStorageDirectory)
+{
+    return adoptRef(*new StorageManager(WTFMove(localStorageDirectory)));
+}
+
+StorageManager::StorageManager(String&& localStorageDirectory)
     : m_queue(WorkQueue::create("com.apple.WebKit.StorageManager"))
 {
+    ASSERT(RunLoop::isMain());
+
     // Make sure the encoding is initialized before we start dispatching things to the queue.
     UTF8Encoding();
     if (!localStorageDirectory.isNull())
-        m_localStorageDatabaseTracker = LocalStorageDatabaseTracker::create(m_queue.copyRef(), localStorageDirectory);
+        m_localStorageDatabaseTracker = LocalStorageDatabaseTracker::create(m_queue.copyRef(), WTFMove(localStorageDirectory));
 }
 
 StorageManager::~StorageManager()
 {
+    ASSERT(RunLoop::isMain());
 }
 

trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h

@@ -46 +46 @@
 using GetValuesCallback = CompletionHandler<void(const HashMap<String, String>&)>;
 
-class StorageManager : public ThreadSafeRefCounted<StorageManager> {
+class StorageManager : public ThreadSafeRefCounted<StorageManager, WTF::DestructionThread::MainRunLoop> {
 public:
-    static Ref<StorageManager> create(const String& localStorageDirectory);
+    static Ref<StorageManager> create(String&& localStorageDirectory);
     ~StorageManager();
 
@@ -78 +78 @@
 
 private:
-    explicit StorageManager(const String& localStorageDirectory);
+    explicit StorageManager(String&& localStorageDirectory);
 
     // Message handlers.

trunk/Source/WebKit/NetworkProcess/WebStorage/ios/LocalStorageDatabaseTrackerIOS.mm

@@ -41 +41 @@
 
     if (linkedOnOrAfter(SDKVersion::FirstToExcludeLocalStorageFromBackup))
-        [[NSURL fileURLWithPath:(NSString *)m_localStorageDirectory isDirectory:YES] setResourceValue:@YES forKey:NSURLIsExcludedFromBackupKey error:nil];
+        [[NSURL fileURLWithPath:(NSString *)localStorageDirectory() isDirectory:YES] setResourceValue:@YES forKey:NSURLIsExcludedFromBackupKey error:nil];
 }
 

trunk/Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm

@@ -925 +925 @@
 
 NetworkSessionCocoa::NetworkSessionCocoa(NetworkProcess& networkProcess, NetworkSessionCreationParameters&& parameters)
-    : NetworkSession(networkProcess, parameters.sessionID, parameters.localStorageDirectory, parameters.localStorageDirectoryExtensionHandle)
+    : NetworkSession(networkProcess, parameters.sessionID, WTFMove(parameters.localStorageDirectory), parameters.localStorageDirectoryExtensionHandle)
     , m_boundInterfaceIdentifier(parameters.boundInterfaceIdentifier)
     , m_sourceApplicationBundleIdentifier(parameters.sourceApplicationBundleIdentifier)

trunk/Source/WebKit/NetworkProcess/curl/NetworkSessionCurl.cpp

@@ -39 +39 @@
 
 NetworkSessionCurl::NetworkSessionCurl(NetworkProcess& networkProcess, NetworkSessionCreationParameters&& parameters)
-    : NetworkSession(networkProcess, parameters.sessionID, parameters.localStorageDirectory, parameters.localStorageDirectoryExtensionHandle)
+    : NetworkSession(networkProcess, parameters.sessionID, WTFMove(parameters.localStorageDirectory), parameters.localStorageDirectoryExtensionHandle)
 {
     if (!parameters.cookiePersistentStorageFile.isEmpty())

trunk/Source/WebKit/NetworkProcess/soup/NetworkSessionSoup.cpp

@@ -41 +41 @@
 
 NetworkSessionSoup::NetworkSessionSoup(NetworkProcess& networkProcess, NetworkSessionCreationParameters&& parameters)
-    : NetworkSession(networkProcess, parameters.sessionID, parameters.localStorageDirectory, parameters.localStorageDirectoryExtensionHandle)
+    : NetworkSession(networkProcess, parameters.sessionID, WTFMove(parameters.localStorageDirectory), parameters.localStorageDirectoryExtensionHandle)
 {
     networkStorageSession()->setCookieObserverHandler([this] {