Attachment 372304 Details for Bug 198947 – Patch

[patch] Patch

bug-198947-20190617175743.patch (text/plain), 16.91 KB, created by Alex Christensen on 2019-06-17 17:57:44 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Alex Christensen

Created: 2019-06-17 17:57:44 PDT

Size: 16.91 KB

patch

obsolete

>Index: Source/WebKit/ChangeLog
>===================================================================
>--- Source/WebKit/ChangeLog	(revision 246528)
>+++ Source/WebKit/ChangeLog	(working copy)
>@@ -1,3 +1,36 @@
>+2019-06-17  Alex Christensen  <achristensen@webkit.org>
>+
>+        NetworkSession::networkStorageSession can return null
>+        https://bugs.webkit.org/show_bug.cgi?id=198947
>+        <rdar://problem/51394449>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        We are seeing evidence of crashes from the map owning NetworkSessions and the map owning NetworkStorageSessions becoming out of sync
>+        because NetworkSession is refcounted but NetworkStorageSession is not.  I started the complete fix in https://bugs.webkit.org/show_bug.cgi?id=194926
>+        but for now let's add less risky null checks to prevent fallout.
>+
>+        * NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp:
>+        (WebKit::ResourceLoadStatisticsStore::updateClientSideCookiesAgeCap):
>+        * NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:
>+        (WebKit::WebResourceLoadStatisticsStore::hasStorageAccessForFrame):
>+        (WebKit::WebResourceLoadStatisticsStore::callHasStorageAccessForFrameHandler):
>+        (WebKit::WebResourceLoadStatisticsStore::grantStorageAccess):
>+        (WebKit::WebResourceLoadStatisticsStore::removeAllStorageAccess):
>+        (WebKit::WebResourceLoadStatisticsStore::setCacheMaxAgeCap):
>+        (WebKit::WebResourceLoadStatisticsStore::callUpdatePrevalentDomainsToBlockCookiesForHandler):
>+        (WebKit::WebResourceLoadStatisticsStore::removePrevalentDomains):
>+        * NetworkProcess/NetworkProcess.cpp:
>+        (WebKit::NetworkProcess::initializeNetworkProcess):
>+        * NetworkProcess/NetworkSession.cpp:
>+        (WebKit::NetworkSession::networkStorageSession const):
>+        * NetworkProcess/NetworkSession.h:
>+        * NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:
>+        (WebKit::NetworkDataTaskCocoa::applyCookieBlockingPolicy):
>+        (WebKit::NetworkDataTaskCocoa::NetworkDataTaskCocoa):
>+        (WebKit::NetworkDataTaskCocoa::willPerformHTTPRedirection):
>+        (WebKit::NetworkDataTaskCocoa::tryPasswordBasedAuthentication):
>+
> 2019-06-17  Tim Horton  <timothy_horton@apple.com>
> 
>         Fix the build with case-sensitive includes
>Index: Source/WebKit/NetworkProcess/NetworkProcess.cpp
>===================================================================
>--- Source/WebKit/NetworkProcess/NetworkProcess.cpp	(revision 246516)
>+++ Source/WebKit/NetworkProcess/NetworkProcess.cpp	(working copy)
>@@ -335,8 +335,9 @@ void NetworkProcess::initializeNetworkPr
>     initializeStorageQuota(parameters.defaultDataStoreParameters);
> 
>     auto* defaultSession = networkSession(PAL::SessionID::defaultSessionID());
>+    auto* defaultStorageSession = defaultSession->networkStorageSession();
>     for (const auto& cookie : parameters.defaultDataStoreParameters.pendingCookies)
>-        defaultSession->networkStorageSession().setCookie(cookie);
>+        defaultStorageSession->setCookie(cookie);
> 
>     for (auto& supplement : m_supplements.values())
>         supplement->initialize(parameters);
>Index: Source/WebKit/NetworkProcess/NetworkSession.cpp
>===================================================================
>--- Source/WebKit/NetworkProcess/NetworkSession.cpp	(revision 246516)
>+++ Source/WebKit/NetworkProcess/NetworkSession.cpp	(working copy)
>@@ -68,11 +68,13 @@ Ref<NetworkSession> NetworkSession::crea
> #endif
> }
> 
>-NetworkStorageSession& NetworkSession::networkStorageSession() const
>+NetworkStorageSession* NetworkSession::networkStorageSession() const
> {
>+    // FIXME: https://bugs.webkit.org/show_bug.cgi?id=194926 NetworkSession should own NetworkStorageSession
>+    // instead of having separate maps with the same key and different management.
>     auto* storageSession = m_networkProcess->storageSession(m_sessionID);
>-    RELEASE_ASSERT(storageSession);
>-    return *storageSession;
>+    ASSERT(storageSession);
>+    return storageSession;
> }
> 
> NetworkSession::NetworkSession(NetworkProcess& networkProcess, PAL::SessionID sessionID, const String& localStorageDirectory, SandboxExtension::Handle& handle)
>Index: Source/WebKit/NetworkProcess/NetworkSession.h
>===================================================================
>--- Source/WebKit/NetworkProcess/NetworkSession.h	(revision 246516)
>+++ Source/WebKit/NetworkProcess/NetworkSession.h	(working copy)
>@@ -72,7 +72,7 @@ public:
> 
>     PAL::SessionID sessionID() const { return m_sessionID; }
>     NetworkProcess& networkProcess() { return m_networkProcess; }
>-    WebCore::NetworkStorageSession& networkStorageSession() const;
>+    WebCore::NetworkStorageSession* networkStorageSession() const;
> 
>     void registerNetworkDataTask(NetworkDataTask& task) { m_dataTaskSet.add(&task); }
>     void unregisterNetworkDataTask(NetworkDataTask& task) { m_dataTaskSet.remove(&task); }
>Index: Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp
>===================================================================
>--- Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp	(revision 246516)
>+++ Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp	(working copy)
>@@ -389,8 +389,10 @@ void ResourceLoadStatisticsStore::update
> 
> #if ENABLE(RESOURCE_LOAD_STATISTICS)
>     RunLoop::main().dispatch([store = makeRef(m_store), seconds = m_parameters.clientSideCookiesAgeCapTime] () {
>-        if (auto* networkSession = store->networkSession())
>-            networkSession->networkStorageSession().setAgeCapForClientSideCookies(seconds);
>+        if (auto* networkSession = store->networkSession()) {
>+            if (auto* storageSession = networkSession->networkStorageSession())
>+                storageSession->setAgeCapForClientSideCookies(seconds);
>+        }
>     });
> #endif
> }
>Index: Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp
>===================================================================
>--- Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp	(revision 246516)
>+++ Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp	(working copy)
>@@ -300,7 +300,11 @@ void WebResourceLoadStatisticsStore::has
> 
> bool WebResourceLoadStatisticsStore::hasStorageAccessForFrame(const RegistrableDomain& resourceDomain, const RegistrableDomain& firstPartyDomain, uint64_t frameID, PageIdentifier pageID)
> {
>-    return m_networkSession ? m_networkSession->networkStorageSession().hasStorageAccess(resourceDomain, firstPartyDomain, frameID, pageID) : false;
>+    if (m_networkSession) {
>+        if (auto* storageSession = m_networkSession->networkStorageSession())
>+            return storageSession->hasStorageAccess(resourceDomain, firstPartyDomain, frameID, pageID);
>+    }
>+    return false;
> }
> 
> void WebResourceLoadStatisticsStore::callHasStorageAccessForFrameHandler(const RegistrableDomain& resourceDomain, const RegistrableDomain& firstPartyDomain, uint64_t frameID, PageIdentifier pageID, CompletionHandler<void(bool hasAccess)>&& callback)
>@@ -308,8 +312,10 @@ void WebResourceLoadStatisticsStore::cal
>     ASSERT(RunLoop::isMain());
> 
>     if (m_networkSession) {
>-        callback(m_networkSession->networkStorageSession().hasStorageAccess(resourceDomain, firstPartyDomain, frameID, pageID));
>-        return;
>+        if (auto* storageSession = m_networkSession->networkStorageSession()) {
>+            callback(storageSession->hasStorageAccess(resourceDomain, firstPartyDomain, frameID, pageID));
>+            return;
>+        }
>     }
> 
>     callback(false);
>@@ -401,9 +407,11 @@ StorageAccessWasGranted WebResourceLoadS
>     bool isStorageGranted = false;
> 
>     if (m_networkSession) {
>-        m_networkSession->networkStorageSession().grantStorageAccess(resourceDomain, firstPartyDomain, frameID, pageID);
>-        ASSERT(m_networkSession->networkStorageSession().hasStorageAccess(resourceDomain, firstPartyDomain, frameID, pageID));
>-        isStorageGranted = true;
>+        if (auto* storageSession = m_networkSession->networkStorageSession()) {
>+            storageSession->grantStorageAccess(resourceDomain, firstPartyDomain, frameID, pageID);
>+            ASSERT(storageSession->hasStorageAccess(resourceDomain, firstPartyDomain, frameID, pageID));
>+            isStorageGranted = true;
>+        }
>     }
> 
>     return isStorageGranted ? StorageAccessWasGranted::Yes : StorageAccessWasGranted::No;
>@@ -431,8 +439,10 @@ void WebResourceLoadStatisticsStore::rem
> {
>     ASSERT(RunLoop::isMain());
> 
>-    if (m_networkSession)
>-        m_networkSession->networkStorageSession().removeAllStorageAccess();
>+    if (m_networkSession) {
>+        if (auto* storageSession = m_networkSession->networkStorageSession())
>+            storageSession->removeAllStorageAccess();
>+    }
> 
>     completionHandler();
> }
>@@ -936,8 +946,10 @@ void WebResourceLoadStatisticsStore::set
>     ASSERT(RunLoop::isMain());
>     ASSERT(seconds >= 0_s);
>     
>-    if (m_networkSession)
>-        m_networkSession->networkStorageSession().setCacheMaxAgeCapForPrevalentResources(seconds);
>+    if (m_networkSession) {
>+        if (auto* storageSession = m_networkSession->networkStorageSession())
>+            storageSession->setCacheMaxAgeCapForPrevalentResources(seconds);
>+    }
> 
>     completionHandler();
> }
>@@ -946,16 +958,20 @@ void WebResourceLoadStatisticsStore::cal
> {
>     ASSERT(RunLoop::isMain());
> 
>-    if (m_networkSession)
>-        m_networkSession->networkStorageSession().setPrevalentDomainsToBlockCookiesFor(domainsToBlock);
>+    if (m_networkSession) {
>+        if (auto* storageSession = m_networkSession->networkStorageSession())
>+            storageSession->setPrevalentDomainsToBlockCookiesFor(domainsToBlock);
>+    }
> 
>     completionHandler();
> }
> 
> void WebResourceLoadStatisticsStore::removePrevalentDomains(const Vector<RegistrableDomain>& domains)
> {
>-    if (m_networkSession)
>-        m_networkSession->networkStorageSession().removePrevalentDomains(domains);
>+    if (m_networkSession) {
>+        if (auto* storageSession = m_networkSession->networkStorageSession())
>+            storageSession->removePrevalentDomains(domains);
>+    }
> }
> 
> void WebResourceLoadStatisticsStore::callRemoveDomainsHandler(const Vector<RegistrableDomain>& domains)
>Index: Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm
>===================================================================
>--- Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm	(revision 246516)
>+++ Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm	(working copy)
>@@ -138,9 +138,15 @@ void NetworkDataTaskCocoa::applyCookieBl
>     if (shouldBlock == m_hasBeenSetToUseStatelessCookieStorage)
>         return;
> 
>-    NSHTTPCookieStorage *storage = shouldBlock ? statelessCookieStorage() : m_session->networkStorageSession().nsCookieStorage();
>-    [m_task _setExplicitCookieStorage:storage._cookieStorage];
>-    m_hasBeenSetToUseStatelessCookieStorage = shouldBlock;
>+    NSHTTPCookieStorage *storage = nil;
>+    if (shouldBlock)
>+        storage = statelessCookieStorage();
>+    else if (auto* storageSession = m_session->networkStorageSession())
>+        storage = storageSession->nsCookieStorage();
>+    if (storage) {
>+        [m_task _setExplicitCookieStorage:storage._cookieStorage];
>+        m_hasBeenSetToUseStatelessCookieStorage = shouldBlock;
>+    }
> }
> #endif
> 
>@@ -180,10 +186,12 @@ NetworkDataTaskCocoa::NetworkDataTaskCoc
>         url = request.url();
>     
> #if USE(CREDENTIAL_STORAGE_WITH_NETWORK_SESSION)
>-        if (m_user.isEmpty() && m_password.isEmpty())
>-            m_initialCredential = m_session->networkStorageSession().credentialStorage().get(m_partition, url);
>-        else
>-            m_session->networkStorageSession().credentialStorage().set(m_partition, WebCore::Credential(m_user, m_password, WebCore::CredentialPersistenceNone), url);
>+        if (auto* storageSession = m_session->networkStorageSession()) {
>+            if (m_user.isEmpty() && m_password.isEmpty())
>+                m_initialCredential = storageSession->credentialStorage().get(m_partition, url);
>+            else
>+                storageSession->credentialStorage().set(m_partition, WebCore::Credential(m_user, m_password, WebCore::CredentialPersistenceNone), url);
>+        }
> #endif
>     }
> 
>@@ -196,7 +204,8 @@ NetworkDataTaskCocoa::NetworkDataTaskCoc
> 
>     bool shouldBlockCookies = false;
> #if ENABLE(RESOURCE_LOAD_STATISTICS)
>-    shouldBlockCookies = storedCredentialsPolicy == WebCore::StoredCredentialsPolicy::EphemeralStatelessCookieless || session.networkStorageSession().shouldBlockCookies(request, frameID, pageID);
>+    shouldBlockCookies = storedCredentialsPolicy == WebCore::StoredCredentialsPolicy::EphemeralStatelessCookieless
>+        || (session.networkStorageSession() && session.networkStorageSession()->shouldBlockCookies(request, frameID, pageID));
> #endif
>     restrictRequestReferrerToOriginIfNeeded(request, shouldBlockCookies);
> 
>@@ -351,7 +360,7 @@ void NetworkDataTaskCocoa::willPerformHT
>         // Only consider applying authentication credentials if this is actually a redirect and the redirect
>         // URL didn't include credentials of its own.
>         if (m_user.isEmpty() && m_password.isEmpty() && !redirectResponse.isNull()) {
>-            auto credential = m_session->networkStorageSession().credentialStorage().get(m_partition, request.url());
>+            auto credential = m_session->networkStorageSession() ? m_session->networkStorageSession()->credentialStorage().get(m_partition, request.url()) : WebCore::Credential();
>             if (!credential.isEmpty()) {
>                 m_initialCredential = credential;
> 
>@@ -366,7 +375,8 @@ void NetworkDataTaskCocoa::willPerformHT
>         request.setFirstPartyForCookies(request.url());
> 
> #if ENABLE(RESOURCE_LOAD_STATISTICS)
>-    bool shouldBlockCookies = m_storedCredentialsPolicy == WebCore::StoredCredentialsPolicy::EphemeralStatelessCookieless || m_session->networkStorageSession().shouldBlockCookies(request, m_frameID, m_pageID);
>+    bool shouldBlockCookies = m_storedCredentialsPolicy == WebCore::StoredCredentialsPolicy::EphemeralStatelessCookieless
>+        || (m_session->networkStorageSession() && m_session->networkStorageSession()->shouldBlockCookies(request, m_frameID, m_pageID));
> #if !RELEASE_LOG_DISABLED
>     if (m_session->shouldLogCookieInformation())
>         RELEASE_LOG_IF(isAlwaysOnLoggingAllowed(), Network, "%p - NetworkDataTaskCocoa::willPerformHTTPRedirection::logCookieInformation: pageID = %llu, frameID = %llu, taskID = %lu: %s cookies for redirect URL %s", this, m_pageID.toUInt64(), m_frameID, (unsigned long)[m_task taskIdentifier], (shouldBlockCookies ? "Blocking" : "Not blocking"), request.url().string().utf8().data());
>@@ -397,7 +407,7 @@ void NetworkDataTaskCocoa::willPerformHT
>                 return completionHandler({ });
>             if (!request.isNull()) {
> #if ENABLE(RESOURCE_LOAD_STATISTICS)
>-                bool shouldBlockCookies = m_session->networkStorageSession().shouldBlockCookies(request, m_frameID, m_pageID);
>+                bool shouldBlockCookies = m_session->networkStorageSession() && m_session->networkStorageSession()->shouldBlockCookies(request, m_frameID, m_pageID);
> #else
>                 bool shouldBlockCookies = false;
> #endif
>@@ -445,16 +455,18 @@ bool NetworkDataTaskCocoa::tryPasswordBa
>             // The stored credential wasn't accepted, stop using it.
>             // There is a race condition here, since a different credential might have already been stored by another ResourceHandle,
>             // but the observable effect should be very minor, if any.
>-            m_session->networkStorageSession().credentialStorage().remove(m_partition, challenge.protectionSpace());
>+            if (auto* storageSession = m_session->networkStorageSession())
>+                storageSession->credentialStorage().remove(m_partition, challenge.protectionSpace());
>         }
> 
>         if (!challenge.previousFailureCount()) {
>-            auto credential = m_session->networkStorageSession().credentialStorage().get(m_partition, challenge.protectionSpace());
>+            auto credential = m_session->networkStorageSession() ? m_session->networkStorageSession()->credentialStorage().get(m_partition, challenge.protectionSpace()) : WebCore::Credential();
>             if (!credential.isEmpty() && credential != m_initialCredential) {
>                 ASSERT(credential.persistence() == WebCore::CredentialPersistenceNone);
>                 if (challenge.failureResponse().httpStatusCode() == 401) {
>                     // Store the credential back, possibly adding it as a default for this directory.
>-                    m_session->networkStorageSession().credentialStorage().set(m_partition, credential, challenge.protectionSpace(), challenge.failureResponse().url());
>+                    if (auto* storageSession = m_session->networkStorageSession())
>+                        storageSession->credentialStorage().set(m_partition, credential, challenge.protectionSpace(), challenge.failureResponse().url());
>                 }
>                 completionHandler(AuthenticationChallengeDisposition::UseCredential, credential);
>                 return true;

Actions: View | Formatted Diff | Diff

Attachments on bug 198947: 372304 | 372307