Changeset 249748 in webkit


Ignore:
Timestamp:
Sep 10, 2019 5:54:05 PM (5 years ago)
Author:
Chris Dumez
Message:

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.

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):

Location:
trunk/Source
Files:
10 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r249739 r249748  
     12019-09-10  Chris Dumez  <cdumez@apple.com>
     2
     3        Nullptr crash in Page::sessionID() via WebKit::WebFrameLoaderClient::detachedFromParent2()
     4        https://bugs.webkit.org/show_bug.cgi?id=201625
     5
     6        Reviewed by Ryosuke Niwa.
     7
     8        This is based on a patch from Ryosuke Niwa.
     9
     10        Drop setHasFrameSpecificStorageAccess() in WebCore and call it from the WebKit layer instead.
     11
     12        * dom/DocumentStorageAccess.cpp:
     13        (WebCore::DocumentStorageAccess::requestStorageAccess):
     14        (WebCore::DocumentStorageAccess::setHasFrameSpecificStorageAccess): Deleted.
     15        * dom/DocumentStorageAccess.h:
     16        * loader/EmptyFrameLoaderClient.h:
     17        * loader/FrameLoaderClient.h:
     18
    1192019-09-10  Brady Eidson  <beidson@apple.com>
    220

  • trunk/Source/WebCore/dom/DocumentStorageAccess.cpp

    r249359 r249748  
    182182        }
    183183
    184         if (wasGranted == StorageAccessWasGranted::Yes) {
    185             setHasFrameSpecificStorageAccess(true);
     184        if (wasGranted == StorageAccessWasGranted::Yes)
    186185            promise->resolve();
    187         } else {
     186        else {
    188187            if (promptWasShown == StorageAccessPromptWasShown::Yes)
    189188                setWasExplicitlyDeniedFrameSpecificStorageAccess();
    … …  
    216215}
    217216
    218 void DocumentStorageAccess::setHasFrameSpecificStorageAccess(bool value)
    219 {
    220     if (auto* frame = m_document.frame())
    221         frame->loader().client().setHasFrameSpecificStorageAccess(value);
    222 }
    223 
    224217} // namespace WebCore
    225218

  • trunk/Source/WebCore/dom/DocumentStorageAccess.h

    r246647 r249748  
    6666    static const char* supplementName();
    6767    bool hasFrameSpecificStorageAccess() const;
    68     void setHasFrameSpecificStorageAccess(bool);
    6968    void setWasExplicitlyDeniedFrameSpecificStorageAccess() { ++m_numberOfTimesExplicitlyDeniedFrameSpecificStorageAccess; };
    7069    bool isAllowedToRequestFrameSpecificStorageAccess() { return m_numberOfTimesExplicitlyDeniedFrameSpecificStorageAccess < maxNumberOfTimesExplicitlyDeniedFrameSpecificStorageAccess; };

  • trunk/Source/WebCore/loader/EmptyFrameLoaderClient.h

    r249519 r249748  
    209209#if ENABLE(RESOURCE_LOAD_STATISTICS)
    210210    bool hasFrameSpecificStorageAccess() final { return false; }
    211     void setHasFrameSpecificStorageAccess(bool) final { }
    212211#endif
    213212};

  • trunk/Source/WebCore/loader/FrameLoaderClient.h

    r249501 r249748  
    380380#if ENABLE(RESOURCE_LOAD_STATISTICS)
    381381    virtual bool hasFrameSpecificStorageAccess() { return false; }
    382     virtual void setHasFrameSpecificStorageAccess(bool) { }
    383382#endif
    384383};

  • trunk/Source/WebKit/ChangeLog

    r249739 r249748  
     12019-09-10  Chris Dumez  <cdumez@apple.com>
     2
     3        Nullptr crash in Page::sessionID() via WebKit::WebFrameLoaderClient::detachedFromParent2()
     4        https://bugs.webkit.org/show_bug.cgi?id=201625
     5
     6        Reviewed by Ryosuke Niwa.
     7
     8        This is based on a patch from Ryosuke Niwa.
     9
     10        The crash was caused by WebFrameLoaderClient::sessionID() calling WebPage::sessionID() without
     11        checking the nullity of WebPage::m_page which can be null. Added a null check.
     12
     13        Because passing a wrong session to RemoveStorageAccessForFrame could result in a leak, this patch
     14        also replaces m_hasFrameSpecificStorageAccess boolean with an optioanl struct which stores
     15        session ID, frame ID, and page ID even after WebCore::Frame or WebCore::Page had been cleared
     16        or before WebFrameLoaderClient::m_frame is set.
     17
     18        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
     19        (WebKit::WebFrameLoaderClient::sessionID const):
     20        (WebKit::WebFrameLoaderClient::setHasFrameSpecificStorageAccess):
     21        (WebKit::WebFrameLoaderClient::detachedFromParent2):
     22        (WebKit::WebFrameLoaderClient::dispatchWillChangeDocument):
     23        * WebProcess/WebCoreSupport/WebFrameLoaderClient.h:
     24        * WebProcess/WebPage/WebFrame.h:
     25        (WebKit::WebFrame::frameLoaderClient const):
     26        * WebProcess/WebPage/WebPage.cpp:
     27        (WebKit::WebPage::requestStorageAccess):
     28
    1292019-09-10  Brady Eidson  <beidson@apple.com>
    230

  • trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp

    r249501 r249748  
    139139PAL::SessionID WebFrameLoaderClient::sessionID() const
    140140{
    141     return m_frame && m_frame->page() ? m_frame->page()->sessionID() : PAL::SessionID::defaultSessionID();
    142 }
     141    WebPage* page = m_frame ? m_frame->page() : nullptr;
     142    if (!page || !page->corePage()) {
     143        ASSERT_NOT_REACHED();
     144        return PAL::SessionID::defaultSessionID();
     145    }
     146    return page->sessionID();
     147}
     148
     149#if ENABLE(RESOURCE_LOAD_STATISTICS)
     150void WebFrameLoaderClient::setHasFrameSpecificStorageAccess(FrameSpecificStorageAccessIdentifier&& frameSpecificStorageAccessIdentifier )
     151{
     152    ASSERT(!m_frameSpecificStorageAccessIdentifier);
     153
     154    m_frameSpecificStorageAccessIdentifier = WTFMove(frameSpecificStorageAccessIdentifier);
     155}
     156#endif
    143157
    144158void WebFrameLoaderClient::frameLoaderDestroyed()
    … …  
    182196
    183197#if ENABLE(RESOURCE_LOAD_STATISTICS)
    184     if (m_hasFrameSpecificStorageAccess) {
    185         WebProcess::singleton().ensureNetworkProcessConnection().connection().send(Messages::NetworkConnectionToWebProcess::RemoveStorageAccessForFrame(sessionID(), frameID().value(), pageID().value()), 0);
    186         m_hasFrameSpecificStorageAccess = false;
     198    if (m_frameSpecificStorageAccessIdentifier) {
     199        WebProcess::singleton().ensureNetworkProcessConnection().connection().send(Messages::NetworkConnectionToWebProcess::RemoveStorageAccessForFrame(
     200            m_frameSpecificStorageAccessIdentifier->sessionID, m_frameSpecificStorageAccessIdentifier->frameID, m_frameSpecificStorageAccessIdentifier->pageID), 0);
     201        m_frameSpecificStorageAccessIdentifier = WTF::nullopt;
    187202    }
    188203#endif
    … …  
    409424        return;
    410425
    411     if (m_hasFrameSpecificStorageAccess && !WebCore::areRegistrableDomainsEqual(currentUrl, newUrl)) {
    412         WebProcess::singleton().ensureNetworkProcessConnection().connection().send(Messages::NetworkConnectionToWebProcess::RemoveStorageAccessForFrame(sessionID(), frameID().value(), pageID().value()), 0);
    413         m_hasFrameSpecificStorageAccess = false;
     426    if (m_frameSpecificStorageAccessIdentifier && !WebCore::areRegistrableDomainsEqual(currentUrl, newUrl)) {
     427        WebProcess::singleton().ensureNetworkProcessConnection().connection().send(Messages::NetworkConnectionToWebProcess::RemoveStorageAccessForFrame(
     428            m_frameSpecificStorageAccessIdentifier->sessionID, m_frameSpecificStorageAccessIdentifier->frameID, m_frameSpecificStorageAccessIdentifier->pageID), 0);
     429        m_frameSpecificStorageAccessIdentifier = WTF::nullopt;
    414430    }
    415431#endif

  • trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.h

    r249501 r249748  
    2828#include "WebPageProxyIdentifier.h"
    2929#include <WebCore/FrameLoaderClient.h>
     30#include <pal/SessionID.h>
    3031
    3132namespace PAL {
    … …  
    5960
    6061#if ENABLE(RESOURCE_LOAD_STATISTICS)
    61     bool hasFrameSpecificStorageAccess() { return m_hasFrameSpecificStorageAccess; }
    62     void setHasFrameSpecificStorageAccess(bool value) { m_hasFrameSpecificStorageAccess = value; };
     62    bool hasFrameSpecificStorageAccess() final { return !!m_frameSpecificStorageAccessIdentifier; }
     63   
     64    struct FrameSpecificStorageAccessIdentifier {
     65        PAL::SessionID sessionID;
     66        WebCore::FrameIdentifier frameID;
     67        WebCore::PageIdentifier pageID;
     68    };
     69    void setHasFrameSpecificStorageAccess(FrameSpecificStorageAccessIdentifier&&);
    6370#endif
    6471   
    … …  
    285292    bool m_useIconLoadingClient { false };
    286293#if ENABLE(RESOURCE_LOAD_STATISTICS)
    287     bool m_hasFrameSpecificStorageAccess { false };
     294    Optional<FrameSpecificStorageAccessIdentifier> m_frameSpecificStorageAccessIdentifier;
    288295#endif
    289296};

  • trunk/Source/WebKit/WebProcess/WebPage/WebFrame.h

    r249093 r249748  
    173173#endif
    174174
     175    WebFrameLoaderClient* frameLoaderClient() const { return m_frameLoaderClient.get(); }
     176
    175177private:
    176178    static Ref<WebFrame> create(std::unique_ptr<WebFrameLoaderClient>);

  • trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp

    r249739 r249748  
    65396539void WebPage::requestStorageAccess(RegistrableDomain&& subFrameDomain, RegistrableDomain&& topFrameDomain, WebFrame& frame, CompletionHandler<void(WebCore::StorageAccessWasGranted, WebCore::StorageAccessPromptWasShown)>&& completionHandler)
    65406540{
    6541     WebProcess::singleton().ensureNetworkProcessConnection().connection().sendWithAsyncReply(Messages::NetworkConnectionToWebProcess::RequestStorageAccess(sessionID(), WTFMove(subFrameDomain), WTFMove(topFrameDomain), frame.frameID(), m_identifier, m_webPageProxyIdentifier), WTFMove(completionHandler));
     6541    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 {
     6542        if (wasGranted == StorageAccessWasGranted::Yes)
     6543            frame->frameLoaderClient()->setHasFrameSpecificStorageAccess({ sessionID, frameID, pageID });
     6544        completionHandler(wasGranted, promptWasShown);
     6545    });
    65426546}
    65436547
Note: See TracChangeset for help on using the changeset viewer.