Nullptr crash in Page::sessionID() via WebKit::WebFrameLoaderClient::detachedFromPare...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 11 Sep 2019 00:54:05 +0000 (00:54 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 11 Sep 2019 00:54:05 +0000 (00:54 +0000)
https://bugs.webkit.org/show_bug.cgi?id=201625

Reviewed by Ryosuke Niwa.

This is based on a patch from Ryosuke Niwa.

Source/WebCore:

Drop setHasFrameSpecificStorageAccess() in WebCore and call it from the WebKit layer instead.

* dom/DocumentStorageAccess.cpp:
(WebCore::DocumentStorageAccess::requestStorageAccess):
(WebCore::DocumentStorageAccess::setHasFrameSpecificStorageAccess): Deleted.
* dom/DocumentStorageAccess.h:
* loader/EmptyFrameLoaderClient.h:
* loader/FrameLoaderClient.h:

Source/WebKit:

The crash was caused by WebFrameLoaderClient::sessionID() calling WebPage::sessionID() without
checking the nullity of WebPage::m_page which can be null. Added a null check.

Because passing a wrong session to RemoveStorageAccessForFrame could result in a leak, this patch
also replaces m_hasFrameSpecificStorageAccess boolean with an optioanl struct which stores
session ID, frame ID, and page ID even after WebCore::Frame or WebCore::Page had been cleared
or before WebFrameLoaderClient::m_frame is set.

* WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
(WebKit::WebFrameLoaderClient::sessionID const):
(WebKit::WebFrameLoaderClient::setHasFrameSpecificStorageAccess):
(WebKit::WebFrameLoaderClient::detachedFromParent2):
(WebKit::WebFrameLoaderClient::dispatchWillChangeDocument):
* WebProcess/WebCoreSupport/WebFrameLoaderClient.h:
* WebProcess/WebPage/WebFrame.h:
(WebKit::WebFrame::frameLoaderClient const):
* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::requestStorageAccess):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@249748 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/dom/DocumentStorageAccess.cpp
Source/WebCore/dom/DocumentStorageAccess.h
Source/WebCore/loader/EmptyFrameLoaderClient.h
Source/WebCore/loader/FrameLoaderClient.h
Source/WebKit/ChangeLog
Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.h
Source/WebKit/WebProcess/WebPage/WebFrame.h
Source/WebKit/WebProcess/WebPage/WebPage.cpp

index d40a248..912200a 100644 (file)
@@ -1,3 +1,21 @@
+2019-09-10  Chris Dumez  <cdumez@apple.com>
+
+        Nullptr crash in Page::sessionID() via WebKit::WebFrameLoaderClient::detachedFromParent2()
+        https://bugs.webkit.org/show_bug.cgi?id=201625
+
+        Reviewed by Ryosuke Niwa.
+
+        This is based on a patch from Ryosuke Niwa.
+
+        Drop setHasFrameSpecificStorageAccess() in WebCore and call it from the WebKit layer instead.
+
+        * dom/DocumentStorageAccess.cpp:
+        (WebCore::DocumentStorageAccess::requestStorageAccess):
+        (WebCore::DocumentStorageAccess::setHasFrameSpecificStorageAccess): Deleted.
+        * dom/DocumentStorageAccess.h:
+        * loader/EmptyFrameLoaderClient.h:
+        * loader/FrameLoaderClient.h:
+
 2019-09-10  Brady Eidson  <beidson@apple.com>
 
         Add SPI to save a PDF from the contents of a WKWebView.
index c1f4d88..ba01ae1 100644 (file)
@@ -181,10 +181,9 @@ void DocumentStorageAccess::requestStorageAccess(Ref<DeferredPromise>&& promise)
             }));
         }
 
-        if (wasGranted == StorageAccessWasGranted::Yes) {
-            setHasFrameSpecificStorageAccess(true);
+        if (wasGranted == StorageAccessWasGranted::Yes)
             promise->resolve();
-        else {
+        else {
             if (promptWasShown == StorageAccessPromptWasShown::Yes)
                 setWasExplicitlyDeniedFrameSpecificStorageAccess();
             promise->reject();
@@ -215,12 +214,6 @@ bool DocumentStorageAccess::hasFrameSpecificStorageAccess() const
     return frame && frame->loader().client().hasFrameSpecificStorageAccess();
 }
 
-void DocumentStorageAccess::setHasFrameSpecificStorageAccess(bool value)
-{
-    if (auto* frame = m_document.frame())
-        frame->loader().client().setHasFrameSpecificStorageAccess(value);
-}
-
 } // namespace WebCore
 
 #endif // ENABLE(RESOURCE_LOAD_STATISTICS)
index ce7f310..b96b13c 100644 (file)
@@ -65,7 +65,6 @@ private:
     static DocumentStorageAccess* from(Document&);
     static const char* supplementName();
     bool hasFrameSpecificStorageAccess() const;
-    void setHasFrameSpecificStorageAccess(bool);
     void setWasExplicitlyDeniedFrameSpecificStorageAccess() { ++m_numberOfTimesExplicitlyDeniedFrameSpecificStorageAccess; };
     bool isAllowedToRequestFrameSpecificStorageAccess() { return m_numberOfTimesExplicitlyDeniedFrameSpecificStorageAccess < maxNumberOfTimesExplicitlyDeniedFrameSpecificStorageAccess; };
     void enableTemporaryTimeUserGesture();
index 8cb3d1e..5b4812a 100644 (file)
@@ -208,7 +208,6 @@ class WEBCORE_EXPORT EmptyFrameLoaderClient : public FrameLoaderClient {
 #endif
 #if ENABLE(RESOURCE_LOAD_STATISTICS)
     bool hasFrameSpecificStorageAccess() final { return false; }
-    void setHasFrameSpecificStorageAccess(bool) final { }
 #endif
 };
 
index ceb7100..fe6ce4b 100644 (file)
@@ -379,7 +379,6 @@ public:
 
 #if ENABLE(RESOURCE_LOAD_STATISTICS)
     virtual bool hasFrameSpecificStorageAccess() { return false; }
-    virtual void setHasFrameSpecificStorageAccess(bool) { }
 #endif
 };
 
index 6e31e7a..e4035cb 100644 (file)
@@ -1,3 +1,31 @@
+2019-09-10  Chris Dumez  <cdumez@apple.com>
+
+        Nullptr crash in Page::sessionID() via WebKit::WebFrameLoaderClient::detachedFromParent2()
+        https://bugs.webkit.org/show_bug.cgi?id=201625
+
+        Reviewed by Ryosuke Niwa.
+
+        This is based on a patch from Ryosuke Niwa.
+
+        The crash was caused by WebFrameLoaderClient::sessionID() calling WebPage::sessionID() without
+        checking the nullity of WebPage::m_page which can be null. Added a null check.
+
+        Because passing a wrong session to RemoveStorageAccessForFrame could result in a leak, this patch
+        also replaces m_hasFrameSpecificStorageAccess boolean with an optioanl struct which stores
+        session ID, frame ID, and page ID even after WebCore::Frame or WebCore::Page had been cleared
+        or before WebFrameLoaderClient::m_frame is set.
+
+        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
+        (WebKit::WebFrameLoaderClient::sessionID const):
+        (WebKit::WebFrameLoaderClient::setHasFrameSpecificStorageAccess):
+        (WebKit::WebFrameLoaderClient::detachedFromParent2):
+        (WebKit::WebFrameLoaderClient::dispatchWillChangeDocument):
+        * WebProcess/WebCoreSupport/WebFrameLoaderClient.h:
+        * WebProcess/WebPage/WebFrame.h:
+        (WebKit::WebFrame::frameLoaderClient const):
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::requestStorageAccess):
+
 2019-09-10  Brady Eidson  <beidson@apple.com>
 
         Add SPI to save a PDF from the contents of a WKWebView.
index 26d552f..830e5c8 100644 (file)
@@ -138,9 +138,23 @@ Optional<FrameIdentifier> WebFrameLoaderClient::frameID() const
 
 PAL::SessionID WebFrameLoaderClient::sessionID() const
 {
-    return m_frame && m_frame->page() ? m_frame->page()->sessionID() : PAL::SessionID::defaultSessionID();
+    WebPage* page = m_frame ? m_frame->page() : nullptr;
+    if (!page || !page->corePage()) {
+        ASSERT_NOT_REACHED();
+        return PAL::SessionID::defaultSessionID();
+    }
+    return page->sessionID();
 }
 
+#if ENABLE(RESOURCE_LOAD_STATISTICS)
+void WebFrameLoaderClient::setHasFrameSpecificStorageAccess(FrameSpecificStorageAccessIdentifier&& frameSpecificStorageAccessIdentifier )
+{
+    ASSERT(!m_frameSpecificStorageAccessIdentifier);
+
+    m_frameSpecificStorageAccessIdentifier = WTFMove(frameSpecificStorageAccessIdentifier);
+}
+#endif
+
 void WebFrameLoaderClient::frameLoaderDestroyed()
 {
     m_frame->invalidate();
@@ -181,9 +195,10 @@ void WebFrameLoaderClient::detachedFromParent2()
         return;
 
 #if ENABLE(RESOURCE_LOAD_STATISTICS)
-    if (m_hasFrameSpecificStorageAccess) {
-        WebProcess::singleton().ensureNetworkProcessConnection().connection().send(Messages::NetworkConnectionToWebProcess::RemoveStorageAccessForFrame(sessionID(), frameID().value(), pageID().value()), 0);
-        m_hasFrameSpecificStorageAccess = false;
+    if (m_frameSpecificStorageAccessIdentifier) {
+        WebProcess::singleton().ensureNetworkProcessConnection().connection().send(Messages::NetworkConnectionToWebProcess::RemoveStorageAccessForFrame(
+            m_frameSpecificStorageAccessIdentifier->sessionID, m_frameSpecificStorageAccessIdentifier->frameID, m_frameSpecificStorageAccessIdentifier->pageID), 0);
+        m_frameSpecificStorageAccessIdentifier = WTF::nullopt;
     }
 #endif
 
@@ -408,9 +423,10 @@ void WebFrameLoaderClient::dispatchWillChangeDocument(const URL& currentUrl, con
     if (!webPage)
         return;
 
-    if (m_hasFrameSpecificStorageAccess && !WebCore::areRegistrableDomainsEqual(currentUrl, newUrl)) {
-        WebProcess::singleton().ensureNetworkProcessConnection().connection().send(Messages::NetworkConnectionToWebProcess::RemoveStorageAccessForFrame(sessionID(), frameID().value(), pageID().value()), 0);
-        m_hasFrameSpecificStorageAccess = false;
+    if (m_frameSpecificStorageAccessIdentifier && !WebCore::areRegistrableDomainsEqual(currentUrl, newUrl)) {
+        WebProcess::singleton().ensureNetworkProcessConnection().connection().send(Messages::NetworkConnectionToWebProcess::RemoveStorageAccessForFrame(
+            m_frameSpecificStorageAccessIdentifier->sessionID, m_frameSpecificStorageAccessIdentifier->frameID, m_frameSpecificStorageAccessIdentifier->pageID), 0);
+        m_frameSpecificStorageAccessIdentifier = WTF::nullopt;
     }
 #endif
 }
index 44ce341..5ab31e5 100644 (file)
@@ -27,6 +27,7 @@
 
 #include "WebPageProxyIdentifier.h"
 #include <WebCore/FrameLoaderClient.h>
+#include <pal/SessionID.h>
 
 namespace PAL {
 class SessionID;
@@ -58,8 +59,14 @@ public:
     PAL::SessionID sessionID() const final;
 
 #if ENABLE(RESOURCE_LOAD_STATISTICS)
-    bool hasFrameSpecificStorageAccess() { return m_hasFrameSpecificStorageAccess; }
-    void setHasFrameSpecificStorageAccess(bool value) { m_hasFrameSpecificStorageAccess = value; };
+    bool hasFrameSpecificStorageAccess() final { return !!m_frameSpecificStorageAccessIdentifier; }
+    
+    struct FrameSpecificStorageAccessIdentifier {
+        PAL::SessionID sessionID;
+        WebCore::FrameIdentifier frameID;
+        WebCore::PageIdentifier pageID;
+    };
+    void setHasFrameSpecificStorageAccess(FrameSpecificStorageAccessIdentifier&&);
 #endif
     
 private:
@@ -284,7 +291,7 @@ private:
     bool m_frameCameFromPageCache;
     bool m_useIconLoadingClient { false };
 #if ENABLE(RESOURCE_LOAD_STATISTICS)
-    bool m_hasFrameSpecificStorageAccess { false };
+    Optional<FrameSpecificStorageAccessIdentifier> m_frameSpecificStorageAccessIdentifier;
 #endif
 };
 
index 5a03a3f..3df056c 100644 (file)
@@ -172,6 +172,8 @@ public:
     void setFirstLayerTreeTransactionIDAfterDidCommitLoad(TransactionID transactionID) { m_firstLayerTreeTransactionIDAfterDidCommitLoad = transactionID; }
 #endif
 
+    WebFrameLoaderClient* frameLoaderClient() const { return m_frameLoaderClient.get(); }
+
 private:
     static Ref<WebFrame> create(std::unique_ptr<WebFrameLoaderClient>);
     explicit WebFrame(std::unique_ptr<WebFrameLoaderClient>);
index cec5644..c9d2db2 100644 (file)
@@ -6538,7 +6538,11 @@ void WebPage::hasStorageAccess(RegistrableDomain&& subFrameDomain, RegistrableDo
 
 void WebPage::requestStorageAccess(RegistrableDomain&& subFrameDomain, RegistrableDomain&& topFrameDomain, WebFrame& frame, CompletionHandler<void(WebCore::StorageAccessWasGranted, WebCore::StorageAccessPromptWasShown)>&& completionHandler)
 {
-    WebProcess::singleton().ensureNetworkProcessConnection().connection().sendWithAsyncReply(Messages::NetworkConnectionToWebProcess::RequestStorageAccess(sessionID(), WTFMove(subFrameDomain), WTFMove(topFrameDomain), frame.frameID(), m_identifier, m_webPageProxyIdentifier), WTFMove(completionHandler));
+    WebProcess::singleton().ensureNetworkProcessConnection().connection().sendWithAsyncReply(Messages::NetworkConnectionToWebProcess::RequestStorageAccess(sessionID(), WTFMove(subFrameDomain), WTFMove(topFrameDomain), frame.frameID(), m_identifier, m_webPageProxyIdentifier), [completionHandler = WTFMove(completionHandler), frame = makeRef(frame), sessionID = sessionID(), pageID = m_identifier, frameID = frame.frameID()](StorageAccessWasGranted wasGranted, WebCore::StorageAccessPromptWasShown promptWasShown) mutable {
+        if (wasGranted == StorageAccessWasGranted::Yes)
+            frame->frameLoaderClient()->setHasFrameSpecificStorageAccess({ sessionID, frameID, pageID });
+        completionHandler(wasGranted, promptWasShown);
+    });
 }
 
 void WebPage::wasLoadedWithDataTransferFromPrevalentResource()