Changeset 278717 in webkit


Ignore:
Timestamp:
Jun 10, 2021, 12:02:51 PM (4 years ago)
Author:
Ben Nham
Message:

Only cache GET requests in the memory cache
​https://bugs.webkit.org/show_bug.cgi?id=226359

Reviewed by Geoff Garen.

Source/WebCore:

Test: http/tests/cache/memory-cache-only-caches-get.html

We only cache GET requests at the disk cache level, but we don't have that same restriction
at the memory cache level. We should make the policies match. In particular, in long-running
webpages, we're accumulating POSTs from XMLHttpRequests in our memory cache and should stop
doing that, since POST response caching is generally not used or expected.

I also changed InspectorInstrumentation::willSendRequest to take an optional CachedResource
parameter because it currently uses InspectorPageAgent::cachedResource to find the resource,
which in turn expects to find the resource in the memory cache. That doesn't work anymore
for these non-GET requests, so we now pass down the CachedResource explicitly in the cases
where that's necessary.

  • inspector/InspectorInstrumentation.cpp:

(WebCore::InspectorInstrumentation::willSendRequestImpl):

  • inspector/InspectorInstrumentation.h:

(WebCore::InspectorInstrumentation::willSendRequest):

  • inspector/agents/InspectorNetworkAgent.cpp:

(WebCore::resourceTypeForCachedResource):
(WebCore::InspectorNetworkAgent::willSendRequest):

  • inspector/agents/InspectorNetworkAgent.h:
  • loader/DocumentLoader.cpp:

(WebCore::DocumentLoader::tryLoadingSubstituteData):
(WebCore::DocumentLoader::addSubresourceLoader):
(WebCore::DocumentLoader::loadMainResource):

  • loader/FrameLoader.cpp:

(WebCore::FrameLoader::requestFromDelegate):

  • loader/ResourceLoadNotifier.cpp:

(WebCore::ResourceLoadNotifier::willSendRequest):
(WebCore::ResourceLoadNotifier::dispatchWillSendRequest):

  • loader/ResourceLoadNotifier.h:
  • loader/ResourceLoader.cpp:

(WebCore::ResourceLoader::willSendRequestInternal):

  • loader/ResourceLoader.h:

(WebCore::ResourceLoader::cachedResource const):

  • loader/SubresourceLoader.cpp:
  • loader/SubresourceLoader.h:
  • loader/appcache/ApplicationCacheGroup.cpp:

(WebCore::ApplicationCacheGroup::update):
(WebCore::ApplicationCacheGroup::startLoadingEntry):

  • loader/cache/MemoryCache.cpp:

(WebCore::MemoryCache::add):

LayoutTests:

Added tests to make sure only GETs end up in the memory cache.

Also fixed a flaky test that was depending on a POST response in an iframe to be in the
memory cache when doing a history navigation.

  • http/tests/cache/memory-cache-only-caches-get-expected.txt: Added.
  • http/tests/cache/memory-cache-only-caches-get.html: Added.
  • http/tests/cache/resources/echo-cacheable.cgi: Added.
  • http/tests/navigation/post-frames-goback1.html:
  • platform/mac/TestExpectations:
Location:
trunk
Files:
3 added
18 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r278713 r278717  
     12021-06-10  Ben Nham  <nham@apple.com>
     2
     3        Only cache GET requests in the memory cache
     4        https://bugs.webkit.org/show_bug.cgi?id=226359
     5
     6        Reviewed by Geoff Garen.
     7
     8        Added tests to make sure only GETs end up in the memory cache.
     9
     10        Also fixed a flaky test that was depending on a POST response in an iframe to be in the
     11        memory cache when doing a history navigation.
     12
     13        * http/tests/cache/memory-cache-only-caches-get-expected.txt: Added.
     14        * http/tests/cache/memory-cache-only-caches-get.html: Added.
     15        * http/tests/cache/resources/echo-cacheable.cgi: Added.
     16        * http/tests/navigation/post-frames-goback1.html:
     17        * platform/mac/TestExpectations:
     18
    1192021-06-10  Truitt Savell  <tsavell@apple.com>
    220

  • trunk/LayoutTests/http/tests/navigation/post-frames-goback1.html

    r124692 r278717  
    99    testRunner.dumpBackForwardList();
    1010}
    11    
     11    
    1212onload = function()
    1313{
    … …  
    1515        delete sessionStorage.didNav;
    1616        delete sessionStorage.topShouldNavAndGoBack;
    17         if (window.testRunner)
    18             testRunner.notifyDone();
     17        if (window.testRunner) {
     18            // Wait until the iframe is showing the POST response before ending the test.
     19            let el = document.getElementById('target-frame');
     20            if (el.src !== 'about:blank' && el.readyState === 'complete') {
     21                testRunner.notifyDone();
     22                return;
     23            }
     24
     25            el.onload = function() {
     26                if (el.src !== 'about:blank')
     27                    testRunner.notifyDone();
     28            };
     29        }
    1930    } else {
    2031        sessionStorage.topShouldNavAndGoBack = true;
    … …  
    3142</form>
    3243
    33 <iframe name="target-frame" src="about:blank"></iframe>
     44<iframe id="target-frame" name="target-frame" src="about:blank"></iframe>
    3445</body>
    3546</html>

  • trunk/LayoutTests/platform/mac/TestExpectations

    r278629 r278717  
    807807# FIXME: Needs bugzilla (<rdar://problem/16040720>)
    808808fast/canvas/canvas-scale-strokePath-shadow.html [ Pass Failure ]
    809 
    810 # FIXME: Needs bugzilla (<rdar://problem/16663912>)
    811 # Seems like this should happen everywhere, but it only does on Yosemite.
    812 http/tests/navigation/post-frames-goback1.html [ Pass Failure ]
    813809
    814810# FIXME: Needs bugzilla (<rdar://problem/16802068>)

  • trunk/Source/WebCore/ChangeLog

    r278714 r278717  
     12021-06-10  Ben Nham  <nham@apple.com>
     2
     3        Only cache GET requests in the memory cache
     4        https://bugs.webkit.org/show_bug.cgi?id=226359
     5
     6        Reviewed by Geoff Garen.
     7
     8        Test: http/tests/cache/memory-cache-only-caches-get.html
     9
     10        We only cache GET requests at the disk cache level, but we don't have that same restriction
     11        at the memory cache level. We should make the policies match. In particular, in long-running
     12        webpages, we're accumulating POSTs from XMLHttpRequests in our memory cache and should stop
     13        doing that, since POST response caching is generally not used or expected.
     14
     15        I also changed InspectorInstrumentation::willSendRequest to take an optional CachedResource
     16        parameter because it currently uses InspectorPageAgent::cachedResource to find the resource,
     17        which in turn expects to find the resource in the memory cache. That doesn't work anymore
     18        for these non-GET requests, so we now pass down the CachedResource explicitly in the cases
     19        where that's necessary.
     20
     21        * inspector/InspectorInstrumentation.cpp:
     22        (WebCore::InspectorInstrumentation::willSendRequestImpl):
     23        * inspector/InspectorInstrumentation.h:
     24        (WebCore::InspectorInstrumentation::willSendRequest):
     25        * inspector/agents/InspectorNetworkAgent.cpp:
     26        (WebCore::resourceTypeForCachedResource):
     27        (WebCore::InspectorNetworkAgent::willSendRequest):
     28        * inspector/agents/InspectorNetworkAgent.h:
     29        * loader/DocumentLoader.cpp:
     30        (WebCore::DocumentLoader::tryLoadingSubstituteData):
     31        (WebCore::DocumentLoader::addSubresourceLoader):
     32        (WebCore::DocumentLoader::loadMainResource):
     33        * loader/FrameLoader.cpp:
     34        (WebCore::FrameLoader::requestFromDelegate):
     35        * loader/ResourceLoadNotifier.cpp:
     36        (WebCore::ResourceLoadNotifier::willSendRequest):
     37        (WebCore::ResourceLoadNotifier::dispatchWillSendRequest):
     38        * loader/ResourceLoadNotifier.h:
     39        * loader/ResourceLoader.cpp:
     40        (WebCore::ResourceLoader::willSendRequestInternal):
     41        * loader/ResourceLoader.h:
     42        (WebCore::ResourceLoader::cachedResource const):
     43        * loader/SubresourceLoader.cpp:
     44        * loader/SubresourceLoader.h:
     45        * loader/appcache/ApplicationCacheGroup.cpp:
     46        (WebCore::ApplicationCacheGroup::update):
     47        (WebCore::ApplicationCacheGroup::startLoadingEntry):
     48        * loader/cache/MemoryCache.cpp:
     49        (WebCore::MemoryCache::add):
     50
    1512021-06-10  Chris Dumez  <cdumez@apple.com>
    252

  • trunk/Source/WebCore/inspector/InspectorInstrumentation.cpp

    r278516 r278717  
    573573}
    574574
    575 void InspectorInstrumentation::willSendRequestImpl(InstrumentingAgents& instrumentingAgents, unsigned long identifier, DocumentLoader* loader, ResourceRequest& request, const ResourceResponse& redirectResponse)
    576 {
    577     if (auto* networkAgent = instrumentingAgents.enabledNetworkAgent())
    578         networkAgent->willSendRequest(identifier, loader, request, redirectResponse);
     575void InspectorInstrumentation::willSendRequestImpl(InstrumentingAgents& instrumentingAgents, unsigned long identifier, DocumentLoader* loader, ResourceRequest& request, const ResourceResponse& redirectResponse, const CachedResource* cachedResource)
     576{
     577    if (auto* networkAgent = instrumentingAgents.enabledNetworkAgent())
     578        networkAgent->willSendRequest(identifier, loader, request, redirectResponse, cachedResource);
    579579}
    580580

  • trunk/Source/WebCore/inspector/InspectorInstrumentation.h

    r278516 r278717  
    194194    static void applyEmulatedMedia(Frame&, String&);
    195195
    196     static void willSendRequest(Frame*, unsigned long identifier, DocumentLoader*, ResourceRequest&, const ResourceResponse& redirectResponse);
     196    static void willSendRequest(Frame*, unsigned long identifier, DocumentLoader*, ResourceRequest&, const ResourceResponse& redirectResponse, const CachedResource*);
    197197    static void didLoadResourceFromMemoryCache(Page&, DocumentLoader*, CachedResource*);
    198198    static void didReceiveResourceResponse(Frame&, unsigned long identifier, DocumentLoader*, const ResourceResponse&, ResourceLoader*);
    … …  
    419419    static void applyEmulatedMediaImpl(InstrumentingAgents&, String&);
    420420
    421     static void willSendRequestImpl(InstrumentingAgents&, unsigned long identifier, DocumentLoader*, ResourceRequest&, const ResourceResponse& redirectResponse);
     421    static void willSendRequestImpl(InstrumentingAgents&, unsigned long identifier, DocumentLoader*, ResourceRequest&, const ResourceResponse& redirectResponse, const CachedResource*);
    422422    static void willSendRequestOfTypeImpl(InstrumentingAgents&, unsigned long identifier, DocumentLoader*, ResourceRequest&, LoadType);
    423423    static void markResourceAsCachedImpl(InstrumentingAgents&, unsigned long identifier);
    … …  
    10501050}
    10511051
    1052 inline void InspectorInstrumentation::willSendRequest(Frame* frame, unsigned long identifier, DocumentLoader* loader, ResourceRequest& request, const ResourceResponse& redirectResponse)
    1053 {
    1054     FAST_RETURN_IF_NO_FRONTENDS(void());
    1055     if (auto* agents = instrumentingAgents(frame))
    1056         willSendRequestImpl(*agents, identifier, loader, request, redirectResponse);
     1052inline void InspectorInstrumentation::willSendRequest(Frame* frame, unsigned long identifier, DocumentLoader* loader, ResourceRequest& request, const ResourceResponse& redirectResponse, const CachedResource* cachedResource)
     1053{
     1054    FAST_RETURN_IF_NO_FRONTENDS(void());
     1055    if (auto* agents = instrumentingAgents(frame))
     1056        willSendRequestImpl(*agents, identifier, loader, request, redirectResponse, cachedResource);
    10571057}
    10581058
    … …  
    10601060{
    10611061    FAST_RETURN_IF_NO_FRONTENDS(void());
    1062     willSendRequestImpl(instrumentingAgents(globalScope), identifier, nullptr, request, ResourceResponse { });
     1062    willSendRequestImpl(instrumentingAgents(globalScope), identifier, nullptr, request, ResourceResponse { }, nullptr);
    10631063}
    10641064

  • trunk/Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp

    r278702 r278717  
    466466}
    467467
    468 static InspectorPageAgent::ResourceType resourceTypeForCachedResource(CachedResource* resource)
     468static InspectorPageAgent::ResourceType resourceTypeForCachedResource(const CachedResource* resource)
    469469{
    470470    if (resource)
    … …  
    487487}
    488488
    489 void InspectorNetworkAgent::willSendRequest(unsigned long identifier, DocumentLoader* loader, ResourceRequest& request, const ResourceResponse& redirectResponse)
    490 {
    491     auto* cachedResource = loader ? InspectorPageAgent::cachedResource(loader->frame(), request.url()) : nullptr;
     489void InspectorNetworkAgent::willSendRequest(unsigned long identifier, DocumentLoader* loader, ResourceRequest& request, const ResourceResponse& redirectResponse, const CachedResource* cachedResource)
     490{
     491    if (!cachedResource && loader)
     492        cachedResource = InspectorPageAgent::cachedResource(loader->frame(), request.url());
    492493    willSendRequest(identifier, loader, request, redirectResponse, resourceTypeForCachedResource(cachedResource));
    493494}

  • trunk/Source/WebCore/inspector/agents/InspectorNetworkAgent.h

    r278516 r278717  
    9999    void willRecalculateStyle();
    100100    void didRecalculateStyle();
    101     void willSendRequest(unsigned long identifier, DocumentLoader*, ResourceRequest&, const ResourceResponse& redirectResponse);
     101    void willSendRequest(unsigned long identifier, DocumentLoader*, ResourceRequest&, const ResourceResponse& redirectResponse, const CachedResource*);
    102102    void willSendRequestOfType(unsigned long identifier, DocumentLoader*, ResourceRequest&, InspectorInstrumentation::LoadType);
    103103    void didReceiveResponse(unsigned long identifier, DocumentLoader*, const ResourceResponse&, ResourceLoader*);

  • trunk/Source/WebCore/loader/DocumentLoader.cpp

    r278702 r278717  
    748748    m_identifierForLoadWithoutResourceLoader = m_frame->page()->progress().createUniqueIdentifier();
    749749    frameLoader()->notifier().assignIdentifierToInitialRequest(m_identifierForLoadWithoutResourceLoader, this, m_request);
    750     frameLoader()->notifier().dispatchWillSendRequest(this, m_identifierForLoadWithoutResourceLoader, m_request, ResourceResponse());
     750    frameLoader()->notifier().dispatchWillSendRequest(this, m_identifierForLoadWithoutResourceLoader, m_request, ResourceResponse(), nullptr);
    751751
    752752    if (!m_deferMainResourceDataLoad || frameLoader()->loadsSynchronously())
    … …  
    18461846        case Document::AboutToEnterBackForwardCache: {
    18471847            // A page about to enter the BackForwardCache should only be able to start ping loads.
    1848             auto* cachedResource = MemoryCache::singleton().resourceForRequest(loader->request(), loader->frameLoader()->frame().page()->sessionID());
     1848            auto* cachedResource = downcast<SubresourceLoader>(loader)->cachedResource();
    18491849            ASSERT(cachedResource && (CachedResource::shouldUsePingLoad(cachedResource->type()) || cachedResource->options().keepAlive));
    18501850            break;
    … …  
    20762076        m_identifierForLoadWithoutResourceLoader = m_frame->page()->progress().createUniqueIdentifier();
    20772077        frameLoader()->notifier().assignIdentifierToInitialRequest(m_identifierForLoadWithoutResourceLoader, this, mainResourceRequest.resourceRequest());
    2078         frameLoader()->notifier().dispatchWillSendRequest(this, m_identifierForLoadWithoutResourceLoader, mainResourceRequest.resourceRequest(), ResourceResponse());
     2078        frameLoader()->notifier().dispatchWillSendRequest(this, m_identifierForLoadWithoutResourceLoader, mainResourceRequest.resourceRequest(), ResourceResponse(), nullptr);
    20792079    }
    20802080

  • trunk/Source/WebCore/loader/FrameLoader.cpp

    r278391 r278717  
    35653565
    35663566    ResourceRequest newRequest(request);
    3567     notifier().dispatchWillSendRequest(m_documentLoader.get(), identifier, newRequest, ResourceResponse());
     3567    notifier().dispatchWillSendRequest(m_documentLoader.get(), identifier, newRequest, ResourceResponse(), nullptr);
    35683568
    35693569    if (newRequest.isNull())

  • trunk/Source/WebCore/loader/ResourceLoadNotifier.cpp

    r278516 r278717  
    6666    m_frame.loader().applyUserAgentIfNeeded(clientRequest);
    6767
    68     dispatchWillSendRequest(loader->documentLoader(), loader->identifier(), clientRequest, redirectResponse);
     68    dispatchWillSendRequest(loader->documentLoader(), loader->identifier(), clientRequest, redirectResponse, loader->cachedResource());
    6969}
    7070
    … …  
    120120}
    121121
    122 void ResourceLoadNotifier::dispatchWillSendRequest(DocumentLoader* loader, unsigned long identifier, ResourceRequest& request, const ResourceResponse& redirectResponse)
     122void ResourceLoadNotifier::dispatchWillSendRequest(DocumentLoader* loader, unsigned long identifier, ResourceRequest& request, const ResourceResponse& redirectResponse, const CachedResource* cachedResource)
    123123{
    124124#if USE(QUICK_LOOK)
    … …  
    150150        m_frame.loader().documentLoader()->didTellClientAboutLoad(request.url().string());
    151151
    152     InspectorInstrumentation::willSendRequest(&m_frame, identifier, loader, request, redirectResponse);
     152    InspectorInstrumentation::willSendRequest(&m_frame, identifier, loader, request, redirectResponse, cachedResource);
    153153}
    154154

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

    r278516 r278717  
    3636
    3737class AuthenticationChallenge;
     38class CachedResource;
    3839class DocumentLoader;
    3940class Frame;
    … …  
    5960
    6061    void assignIdentifierToInitialRequest(unsigned long identifier, DocumentLoader*, const ResourceRequest&);
    61     void dispatchWillSendRequest(DocumentLoader*, unsigned long identifier, ResourceRequest&, const ResourceResponse& redirectResponse);
     62    void dispatchWillSendRequest(DocumentLoader*, unsigned long identifier, ResourceRequest&, const ResourceResponse& redirectResponse, const CachedResource*);
    6263    void dispatchDidReceiveResponse(DocumentLoader*, unsigned long identifier, const ResourceResponse&, ResourceLoader* = nullptr);
    6364    void dispatchDidReceiveData(DocumentLoader*, unsigned long identifier, const uint8_t* data, int dataLength, int encodedDataLength);

  • trunk/Source/WebCore/loader/ResourceLoader.cpp

    r278516 r278717  
    409409    }
    410410    else
    411         InspectorInstrumentation::willSendRequest(m_frame.get(), m_identifier, m_frame->loader().documentLoader(), request, redirectResponse);
     411        InspectorInstrumentation::willSendRequest(m_frame.get(), m_identifier, m_frame->loader().documentLoader(), request, redirectResponse, cachedResource());
    412412
    413413#if USE(QUICK_LOOK)

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

    r278516 r278717  
    4949
    5050class AuthenticationChallenge;
     51class CachedResource;
    5152class DocumentLoader;
    5253class Frame;
    … …  
    131132    WEBCORE_EXPORT bool isAllowedToAskUserForCredentials() const;
    132133    WEBCORE_EXPORT bool shouldIncludeCertificateInfo() const;
     134   
     135    virtual CachedResource* cachedResource() const { return nullptr; }
    133136
    134137    bool reachedTerminalState() const { return m_reachedTerminalState; }

  • trunk/Source/WebCore/loader/SubresourceLoader.cpp

    r278516 r278717  
    153153#endif
    154154
    155 CachedResource* SubresourceLoader::cachedResource()
    156 {
    157     return m_resource;
    158 }
    159 
    160155void SubresourceLoader::cancelIfNotFinishing()
    161156{

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

    r278516 r278717  
    5050    void cancelIfNotFinishing();
    5151    bool isSubresourceLoader() const override;
    52     CachedResource* cachedResource();
     52    CachedResource* cachedResource() const override { return m_resource; };
    5353    WEBCORE_EXPORT const HTTPHeaderMap* originalHeaders() const;
    5454

  • trunk/Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp

    r278702 r278717  
    439439
    440440    m_currentResourceIdentifier = m_frame->page()->progress().createUniqueIdentifier();
    441     InspectorInstrumentation::willSendRequest(m_frame.get(), m_currentResourceIdentifier, m_frame->loader().documentLoader(), request, ResourceResponse { });
     441    InspectorInstrumentation::willSendRequest(m_frame.get(), m_currentResourceIdentifier, m_frame->loader().documentLoader(), request, ResourceResponse { }, nullptr);
    442442
    443443    m_manifestLoader = ApplicationCacheResourceLoader::create(ApplicationCacheResource::Type::Manifest, documentLoader.cachedResourceLoader(), WTFMove(request), [this] (auto&& resourceOrError) {
    … …  
    900900
    901901    m_currentResourceIdentifier = m_frame->page()->progress().createUniqueIdentifier();
    902     InspectorInstrumentation::willSendRequest(m_frame.get(), m_currentResourceIdentifier, m_frame->loader().documentLoader(), request, ResourceResponse { });
     902    InspectorInstrumentation::willSendRequest(m_frame.get(), m_currentResourceIdentifier, m_frame->loader().documentLoader(), request, ResourceResponse { }, nullptr);
    903903
    904904    auto& documentLoader = *m_frame->loader().documentLoader();

  • trunk/Source/WebCore/loader/cache/MemoryCache.cpp

    r278119 r278717  
    113113{
    114114    if (disabled())
     115        return false;
     116
     117    if (resource.resourceRequest().httpMethod() != "GET")
    115118        return false;
    116119
Note: See TracChangeset for help on using the changeset viewer.