Attachment 373678 Details for Bug 199599 – Patch

[patch] Patch

bug-199599-20190708161700.patch (text/plain), 17.52 KB, created by Chris Dumez on 2019-07-08 16:17:01 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-07-08 16:17:01 PDT

Size: 17.52 KB

patch

obsolete

>Subversion Revision: 247230
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 86b823efdf152d74c06ab13ccf5060c2c6c4dd61..fb3d53011eb55b8925a3b61b8c3014746be19fab 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,47 @@
>+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 NOBODY (OOPS!).
>+
>+        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>
> 
>         Cleanup uses of NetworkProcess::m_sessionByConnection
>diff --git a/Source/WebKit/NetworkProcess/NetworkSession.cpp b/Source/WebKit/NetworkProcess/NetworkSession.cpp
>index 804805baeea08ae1a2dcb2542c8096cdea79ac03..1e3a0d91ad9954a5b60f068a7c2e589f13dff546 100644
>--- a/Source/WebKit/NetworkProcess/NetworkSession.cpp
>+++ b/Source/WebKit/NetworkProcess/NetworkSession.cpp
>@@ -77,11 +77,11 @@ NetworkStorageSession* NetworkSession::networkStorageSession() const
>     return storageSession;
> }
> 
>-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);
>     m_adClickAttribution->setPingLoadFunction([this, weakThis = makeWeakPtr(this)](NetworkResourceLoadParameters&& loadParameters, CompletionHandler<void(const WebCore::ResourceError&, const WebCore::ResourceResponse&)>&& completionHandler) {
>diff --git a/Source/WebKit/NetworkProcess/NetworkSession.h b/Source/WebKit/NetworkProcess/NetworkSession.h
>index f521ad3caff02943416a950cef7d5f96bbc59385..4d48732e2616f5cd2589e2d2294608c7bfd9eac5 100644
>--- a/Source/WebKit/NetworkProcess/NetworkSession.h
>+++ b/Source/WebKit/NetworkProcess/NetworkSession.h
>@@ -108,7 +108,7 @@ public:
>     virtual void addWebSocketTask(WebSocketTask&) { }
> 
> protected:
>-    NetworkSession(NetworkProcess&, PAL::SessionID, const String& localStorageDirectory, SandboxExtension::Handle&);
>+    NetworkSession(NetworkProcess&, PAL::SessionID, String&& localStorageDirectory, SandboxExtension::Handle&);
> 
>     PAL::SessionID m_sessionID;
>     Ref<NetworkProcess> m_networkProcess;
>diff --git a/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabaseTracker.cpp b/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabaseTracker.cpp
>index 88987a45cd4fb9c2cbf1f04d8a2e0085d052e010..07a633fbfe9be59edbed539c96e19e4b1bbd2583 100644
>--- a/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabaseTracker.cpp
>+++ b/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabaseTracker.cpp
>@@ -39,15 +39,17 @@
> namespace WebKit {
> 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();
> 
>@@ -57,8 +59,14 @@ LocalStorageDatabaseTracker::LocalStorageDatabaseTracker(Ref<WorkQueue>&& queue,
>     });
> }
> 
>+String LocalStorageDatabaseTracker::localStorageDirectory() const
>+{
>+    return m_localStorageDirectory.isolatedCopy();
>+}
>+
> LocalStorageDatabaseTracker::~LocalStorageDatabaseTracker()
> {
>+    ASSERT(RunLoop::isMain());
> }
> 
> String LocalStorageDatabaseTracker::databasePath(const SecurityOriginData& securityOrigin) const
>@@ -82,14 +90,15 @@ void LocalStorageDatabaseTracker::deleteDatabaseWithOrigin(const SecurityOriginD
> 
> 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);
> 
>         // FIXME: Call out to the client.
>     }
> 
>-    SQLiteFileSystem::deleteEmptyDatabaseDirectory(m_localStorageDirectory);
>+    SQLiteFileSystem::deleteEmptyDatabaseDirectory(localStorageDirectory);
> }
> 
> Vector<SecurityOriginData> LocalStorageDatabaseTracker::databasesModifiedSince(WallTime time)
>@@ -114,7 +123,7 @@ Vector<SecurityOriginData> LocalStorageDatabaseTracker::databasesModifiedSince(W
> Vector<SecurityOriginData> LocalStorageDatabaseTracker::origins() const
> {
>     Vector<SecurityOriginData> databaseOrigins;
>-    auto paths = FileSystem::listDirectory(m_localStorageDirectory, "*.localstorage");
>+    auto paths = FileSystem::listDirectory(localStorageDirectory(), "*.localstorage");
>     
>     for (const auto& path : paths) {
>         auto filename = FileSystem::pathGetFileName(path);
>@@ -150,7 +159,8 @@ Vector<LocalStorageDatabaseTracker::OriginDetails> LocalStorageDatabaseTracker::
> 
> 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());
>         return String();
>@@ -162,7 +172,7 @@ String LocalStorageDatabaseTracker::databasePath(const String& filename) const
>     });
> #endif
> 
>-    return SQLiteFileSystem::appendDatabaseFileNameToPath(m_localStorageDirectory, filename);
>+    return SQLiteFileSystem::appendDatabaseFileNameToPath(localStorageDirectory, filename);
> }
> 
> } // namespace WebKit
>diff --git a/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabaseTracker.h b/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabaseTracker.h
>index 72b93c32277d07e7cc83b335a732bcf1785e7af9..fcaa123c2fa8d257465c73b7227d79c9390935c0 100644
>--- a/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabaseTracker.h
>+++ b/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabaseTracker.h
>@@ -35,9 +35,9 @@
> 
> 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();
> 
>     String databasePath(const WebCore::SecurityOriginData&) const;
>@@ -64,9 +64,10 @@ public:
>     Vector<OriginDetails> originDetails();
> 
> private:
>-    LocalStorageDatabaseTracker(Ref<WorkQueue>&&, const String& localStorageDirectory);
>+    LocalStorageDatabaseTracker(Ref<WorkQueue>&&, String&& localStorageDirectory);
> 
>     String databasePath(const String& filename) const;
>+    String localStorageDirectory() const;
> 
>     enum DatabaseOpeningStrategy {
>         CreateIfNonExistent,
>@@ -74,7 +75,7 @@ private:
>     };
> 
>     Ref<WorkQueue> m_queue;
>-    String m_localStorageDirectory;
>+    const String m_localStorageDirectory;
> 
> #if PLATFORM(IOS_FAMILY)
>     void platformMaybeExcludeFromBackup() const;
>diff --git a/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp b/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp
>index 15add2e5a97bcce78be9fcbd6ec020bdceef8535..6691294baaedc16a775b0d8f266e385450774df2 100644
>--- a/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp
>+++ b/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp
>@@ -481,22 +481,25 @@ void StorageManager::SessionStorageNamespace::cloneTo(SessionStorageNamespace& n
>         newSessionStorageNamespace.m_storageAreaMap.add(pair.key, pair.value->clone());
> }
> 
>-Ref<StorageManager> StorageManager::create(const String& localStorageDirectory)
>+Ref<StorageManager> StorageManager::create(String&& localStorageDirectory)
> {
>-    return adoptRef(*new StorageManager(localStorageDirectory));
>+    return adoptRef(*new StorageManager(WTFMove(localStorageDirectory)));
> }
> 
>-StorageManager::StorageManager(const String& 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());
> }
> 
> void StorageManager::createSessionStorageNamespace(uint64_t storageNamespaceID, unsigned quotaInBytes)
>diff --git a/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h b/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h
>index 75fb09933475add00dd42ab3a54e4b61334c8b11..fb9b7e6fcb64c3a53ab0b3a121e8f6f3e381d6cf 100644
>--- a/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h
>+++ b/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h
>@@ -45,9 +45,9 @@ class WebProcessProxy;
> 
> 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();
> 
>     void createSessionStorageNamespace(uint64_t storageNamespaceID, unsigned quotaInBytes);
>@@ -77,7 +77,7 @@ public:
>     void didReceiveSyncMessage(IPC::Connection&, IPC::Decoder&, std::unique_ptr<IPC::Encoder>& replyEncoder);
> 
> private:
>-    explicit StorageManager(const String& localStorageDirectory);
>+    explicit StorageManager(String&& localStorageDirectory);
> 
>     // Message handlers.
>     void createLocalStorageMap(IPC::Connection&, uint64_t storageMapID, uint64_t storageNamespaceID, WebCore::SecurityOriginData&&);
>diff --git a/Source/WebKit/NetworkProcess/WebStorage/ios/LocalStorageDatabaseTrackerIOS.mm b/Source/WebKit/NetworkProcess/WebStorage/ios/LocalStorageDatabaseTrackerIOS.mm
>index 9b01e896360f42c9f5097ff280b7ff615e65414f..0072484392707059410a93421ae9311e25cb9dad 100644
>--- a/Source/WebKit/NetworkProcess/WebStorage/ios/LocalStorageDatabaseTrackerIOS.mm
>+++ b/Source/WebKit/NetworkProcess/WebStorage/ios/LocalStorageDatabaseTrackerIOS.mm
>@@ -40,7 +40,7 @@ void LocalStorageDatabaseTracker::platformMaybeExcludeFromBackup() const
>     m_hasExcludedFromBackup = true;
> 
>     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];
> }
> 
> } // namespace WebKit
>diff --git a/Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm b/Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm
>index c90ef0a66abc0ee3bf9e92b8a2986025bb8e0607..018889a2866eeb7333100712b1ea9d0799d3e33d 100644
>--- a/Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm
>+++ b/Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm
>@@ -924,7 +924,7 @@ static NSDictionary *proxyDictionary(const URL& httpProxy, const URL& httpsProxy
> }
> 
> 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)
>     , m_sourceApplicationSecondaryIdentifier(parameters.sourceApplicationSecondaryIdentifier)
>diff --git a/Source/WebKit/NetworkProcess/curl/NetworkSessionCurl.cpp b/Source/WebKit/NetworkProcess/curl/NetworkSessionCurl.cpp
>index 42db20c8aa832d0b4691c0dbb521dbf08d97a960..18348a8d55783a7a0b1d2fdbda41a067ff067007 100644
>--- a/Source/WebKit/NetworkProcess/curl/NetworkSessionCurl.cpp
>+++ b/Source/WebKit/NetworkProcess/curl/NetworkSessionCurl.cpp
>@@ -38,7 +38,7 @@ namespace WebKit {
> using namespace WebCore;
> 
> 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())
>         networkStorageSession()->setCookieDatabase(makeUniqueRef<CookieJarDB>(parameters.cookiePersistentStorageFile));
>diff --git a/Source/WebKit/NetworkProcess/soup/NetworkSessionSoup.cpp b/Source/WebKit/NetworkProcess/soup/NetworkSessionSoup.cpp
>index 70d9137c59d765a1d6da4699b04dd085cf8509ce..f7bbfa5c8a67661f794425e14f5bbf2f586c203f 100644
>--- a/Source/WebKit/NetworkProcess/soup/NetworkSessionSoup.cpp
>+++ b/Source/WebKit/NetworkProcess/soup/NetworkSessionSoup.cpp
>@@ -40,7 +40,7 @@ namespace WebKit {
> using namespace WebCore;
> 
> 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] {
>         this->networkProcess().supplement<WebCookieManager>()->notifyCookiesDidChange(m_sessionID);

Flags:

rniwa: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 199599: 373678