Changeset 138962 in webkit

Timestamp:

Jan 7, 2013 10:40:55 AM (11 years ago)

Author:

ap@apple.com

Message:

ResourceHandle::willLoadFromCache is evil
​https://bugs.webkit.org/show_bug.cgi?id=106147

Reviewed by Brady Eidson.

For back/forward navigations to a page that's a result of form submission, we may
never silently re-submit the form. So, we show a warning dialog when about to re-submit,
but try to load from cache if possible.

This patch changes the logic so that we always try to fetch from cache, without
any preflighting. If cache load fails, we restart the load as a known re-submit.

No behavior change expected, so no tests.

• html/HTMLAnchorElement.cpp: (WebCore::HTMLAnchorElement::handleClick): Added a FIXME.

• loader/DocumentLoader.cpp: (WebCore::DocumentLoader::startLoadingMainResource): Amended a FIXME with some information about why this call may still be needed.

• loader/FrameLoader.h:
• loader/FrameLoader.cpp: (WebCore::FrameLoader::loadURLIntoChildFrame): Pass an explicit argument for unchanged caching behavior. (WebCore::FrameLoader::reloadWithOverrideEncoding): Added a FIXME. This function can silently re-submit a form. (WebCore::FrameLoader::addExtraFieldsToMainResourceRequest): Added a FIXME about an incorrect use of current load type. (WebCore::FrameLoader::addExtraFieldsToRequest): Make sure that a correct caching policy is used for subresources even if main resource was loaded from cache. We didn't need that before because initial request had wrong extra fields due to a use of m_loadType when it was first called. Removed code to change caching policy for b/f navigations. This function does not have enough context to decide what the policy should be. (WebCore::FrameLoader::loadDifferentDocumentItem): Added an argument telling the function whether it should attempt loading from cache. It should do that on first attempt to navigate to a form submission result, but not if that failed. Pass a correct loadType - m_loadType is one for _previous_ load. Removed a special case for https - we've long stopped prohibiting caching of https resources, and using a resource that's already cached should definitely be allowed. (WebCore::FrameLoader::loadItem): Pass an explicit argument for unchanged caching behavior. (WebCore::FrameLoader::retryAfterFailedCacheOnlyMainResourceLoad): Added.

• loader/MainResourceLoader.cpp: (WebCore::MainResourceLoader::notifyFinished): Removed a check for m_resource being null, because we were immediately dereferencing it anyway. Call retryAfterFailedCacheOnlyMainResourceLoad() to let FrameLoader restart the navigation.

• platform/network/ResourceHandle.h:
• platform/network/blackberry/ResourceHandleBlackBerry.cpp:
• platform/network/cf/ResourceHandleCFNet.cpp:
• platform/network/chromium/ResourceHandle.cpp:
• platform/network/curl/ResourceHandleCurl.cpp:
• platform/network/mac/ResourceHandleMac.mm:
• platform/network/qt/ResourceHandleQt.cpp:
• platform/network/soup/ResourceHandleSoup.cpp:
• platform/network/win/ResourceHandleWin.cpp: Removed willLoadFromCache() - the new logic is cross-platform.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• html/HTMLAnchorElement.cpp (modified) (1 diff)
• loader/DocumentLoader.cpp (modified) (1 diff)
• loader/FrameLoader.cpp (modified) (11 diffs)
• loader/FrameLoader.h (modified) (3 diffs)
• loader/MainResourceLoader.cpp (modified) (1 diff)
• platform/network/ResourceHandle.h (modified) (1 diff)
• platform/network/blackberry/ResourceHandleBlackBerry.cpp (modified) (1 diff)
• platform/network/cf/ResourceHandleCFNet.cpp (modified) (1 diff)
• platform/network/chromium/ResourceHandle.cpp (modified) (1 diff)
• platform/network/curl/ResourceHandleCurl.cpp (modified) (1 diff)
• platform/network/mac/ResourceHandleMac.mm (modified) (1 diff)
• platform/network/qt/ResourceHandleQt.cpp (modified) (1 diff)
• platform/network/soup/ResourceHandleSoup.cpp (modified) (1 diff)
• platform/network/win/ResourceHandleWin.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2013-01-04  Alexey Proskuryakov  <ap@apple.com>
+
+        ResourceHandle::willLoadFromCache is evil
+        https://bugs.webkit.org/show_bug.cgi?id=106147
+
+        Reviewed by Brady Eidson.
+
+        For back/forward navigations to a page that's a result of form submission, we may
+        never silently re-submit the form. So, we show a warning dialog when about to re-submit,
+        but try to load from cache if possible.
+
+        This patch changes the logic so that we always try to fetch from cache, without
+        any preflighting. If cache load fails, we restart the load as a known re-submit.
+
+        No behavior change expected, so no tests.
+
+        * html/HTMLAnchorElement.cpp: (WebCore::HTMLAnchorElement::handleClick):
+        Added a FIXME.
+
+        * loader/DocumentLoader.cpp: (WebCore::DocumentLoader::startLoadingMainResource):
+        Amended a FIXME with some information about why this call may still be needed.
+
+        * loader/FrameLoader.h:
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::loadURLIntoChildFrame): Pass an explicit argument for unchanged caching behavior.
+        (WebCore::FrameLoader::reloadWithOverrideEncoding): Added a FIXME. This function
+        can silently re-submit a form.
+        (WebCore::FrameLoader::addExtraFieldsToMainResourceRequest): Added a FIXME about
+        an incorrect use of current load type.
+        (WebCore::FrameLoader::addExtraFieldsToRequest): Make sure that a correct caching
+        policy is used for subresources even if main resource was loaded from cache. We
+        didn't need that before because initial request had wrong extra fields due to a use
+        of m_loadType when it was first called.
+        Removed code to change caching policy for b/f navigations. This function does not
+        have enough context to decide what the policy should be.
+        (WebCore::FrameLoader::loadDifferentDocumentItem): Added an argument telling the
+        function whether it should attempt loading from cache. It should do that on first
+        attempt to navigate to a form submission result, but not if that failed.
+        Pass a correct loadType - m_loadType is one for _previous_ load.
+        Removed a special case for https - we've long stopped prohibiting caching of https
+        resources, and using a resource that's already cached should definitely be allowed.
+        (WebCore::FrameLoader::loadItem): Pass an explicit argument for unchanged caching behavior.
+        (WebCore::FrameLoader::retryAfterFailedCacheOnlyMainResourceLoad): Added.
+
+        * loader/MainResourceLoader.cpp: (WebCore::MainResourceLoader::notifyFinished):
+        Removed a check for m_resource being null, because we were immediately dereferencing
+        it anyway.
+        Call retryAfterFailedCacheOnlyMainResourceLoad() to let FrameLoader restart the navigation.
+
+        * platform/network/ResourceHandle.h:
+        * platform/network/blackberry/ResourceHandleBlackBerry.cpp:
+        * platform/network/cf/ResourceHandleCFNet.cpp:
+        * platform/network/chromium/ResourceHandle.cpp:
+        * platform/network/curl/ResourceHandleCurl.cpp:
+        * platform/network/mac/ResourceHandleMac.mm:
+        * platform/network/qt/ResourceHandleQt.cpp:
+        * platform/network/soup/ResourceHandleSoup.cpp:
+        * platform/network/win/ResourceHandleWin.cpp:
+        Removed willLoadFromCache() - the new logic is cross-platform.
+
 2013-01-07  Alberto Garcia  <agarcia@igalia.com>
 

trunk/Source/WebCore/html/HTMLAnchorElement.cpp

@@ -513 +513 @@
         ResourceRequest request(kurl);
 
+        // FIXME: Why are we not calling addExtraFieldsToMainResourceRequest() if this check fails? It sets many important header fields.
         if (!hasRel(RelationNoReferrer)) {
             String referrer = SecurityPolicy::generateReferrerHeader(document()->referrerPolicy(), kurl, frame->loader()->outgoingReferrer());

trunk/Source/WebCore/loader/DocumentLoader.cpp

@@ -885 +885 @@
     // FIXME: Is there any way the extra fields could have not been added by now?
     // If not, it would be great to remove this line of code.
+    // Note that currently, some requests may have incorrect extra fields even if this function has been called,
+    // because we pass a wrong loadType (see FIXME in addExtraFieldsToMainResourceRequest()).
     frameLoader()->addExtraFieldsToMainResourceRequest(m_request);
     m_mainResourceLoader->load(m_request, m_substituteData);

trunk/Source/WebCore/loader/FrameLoader.cpp

@@ -853 +853 @@
         HistoryItem* childItem = parentItem->childItemWithTarget(childFrame->tree()->uniqueName());
         if (childItem) {
-            childFrame->loader()->loadDifferentDocumentItem(childItem, loadType());
+            childFrame->loader()->loadDifferentDocumentItem(childItem, loadType(), MayAttemptCacheOnlyLoadForFormSubmissionItem);
             return;
         }
@@ -1462 +1462 @@
         request.setURL(unreachableURL);
 
+    // FIXME: If the resource is a result of form submission and is not cached, the form will be silently resubmitted.
+    // We should ask the user for confirmation in this case.
     request.setCachePolicy(ReturnCacheDataElseLoad);
 
@@ -2419 +2421 @@
 void FrameLoader::addExtraFieldsToMainResourceRequest(ResourceRequest& request)
 {
+    // FIXME: Using m_loadType seems wrong for some callers.
+    // If we are only preparing to load the main resource, that is previous load's load type!
     addExtraFieldsToRequest(request, m_loadType, true);
 }
@@ -2425 +2429 @@
 {
     // Don't set the cookie policy URL if it's already been set.
-    // But make sure to set it on all requests, as it has significance beyond the cookie policy for all protocols (<rdar://problem/6616664>).
+    // But make sure to set it on all requests regardless of protocol, as it has significance beyond the cookie policy (<rdar://problem/6616664>).
     if (request.firstPartyForCookies().isEmpty()) {
         if (mainResource && isLoadingMainFrame())
@@ -2438 +2442 @@
 
     applyUserAgent(request);
-    
-    // If we inherit cache policy from a main resource, we use the DocumentLoader's 
-    // original request cache policy for two reasons:
-    // 1. For POST requests, we mutate the cache policy for the main resource,
-    //    but we do not want this to apply to subresources
-    // 2. Delegates that modify the cache policy using willSendRequest: should
-    //    not affect any other resources. Such changes need to be done
-    //    per request.
+
     if (!mainResource) {
         if (request.isConditional())
             request.setCachePolicy(ReloadIgnoringCacheData);
-        else if (documentLoader()->isLoadingInAPISense())
-            request.setCachePolicy(documentLoader()->originalRequest().cachePolicy());
-        else
+        else if (documentLoader()->isLoadingInAPISense()) {
+            // If we inherit cache policy from a main resource, we use the DocumentLoader's
+            // original request cache policy for two reasons:
+            // 1. For POST requests, we mutate the cache policy for the main resource,
+            //    but we do not want this to apply to subresources
+            // 2. Delegates that modify the cache policy using willSendRequest: should
+            //    not affect any other resources. Such changes need to be done
+            //    per request.
+            ResourceRequestCachePolicy mainDocumentOriginalCachePolicy = documentLoader()->originalRequest().cachePolicy();
+            // Back-forward navigations try to load main resource from cache only to avoid re-submitting form data, and start over (with a warning dialog) if that fails.
+            // This policy is set on initial request too, but should not be inherited.
+            ResourceRequestCachePolicy subresourceCachePolicy = (mainDocumentOriginalCachePolicy == ReturnCacheDataDontLoad) ? ReturnCacheDataElseLoad : mainDocumentOriginalCachePolicy;
+            request.setCachePolicy(subresourceCachePolicy);
+        } else
             request.setCachePolicy(UseProtocolCachePolicy);
+
+    // FIXME: Other FrameLoader functions have duplicated code for setting cache policy of main request when reloading.
+    // It seems better to manage it explicitly than to hide the logic inside addExtraFieldsToRequest().
     } else if (loadType == FrameLoadTypeReload || loadType == FrameLoadTypeReloadFromOrigin || request.isConditional())
         request.setCachePolicy(ReloadIgnoringCacheData);
-    else if (isBackForwardLoadType(loadType) && m_stateMachine.committedFirstRealDocumentLoad())
-        request.setCachePolicy(ReturnCacheDataElseLoad);
-        
+
     if (request.cachePolicy() == ReloadIgnoringCacheData) {
         if (loadType == FrameLoadTypeReload)
@@ -3034 +3043 @@
 // which should be methods of HistoryController and some of which should be
 // methods of FrameLoader.
-void FrameLoader::loadDifferentDocumentItem(HistoryItem* item, FrameLoadType loadType)
+void FrameLoader::loadDifferentDocumentItem(HistoryItem* item, FrameLoadType loadType, FormSubmissionCacheLoadPolicy cacheLoadPolicy)
 {
     // Remember this item so we can traverse any child items as child frames load
@@ -3069 +3078 @@
         // Make sure to add extra fields to the request after the Origin header is added for the FormData case.
         // See https://bugs.webkit.org/show_bug.cgi?id=22194 for more discussion.
-        addExtraFieldsToRequest(request, m_loadType, true);
+        addExtraFieldsToRequest(request, loadType, true);
         
         // FIXME: Slight hack to test if the NSURL cache contains the page we're going to.
@@ -3079 +3088 @@
         // extremely rare, but in that case the user will get an error on the navigation.
         
-        if (ResourceHandle::willLoadFromCache(request, m_frame))
+        if (cacheLoadPolicy == MayAttemptCacheOnlyLoadForFormSubmissionItem) {
+            request.setCachePolicy(ReturnCacheDataDontLoad);
             action = NavigationAction(request, loadType, false);
-        else {
-            request.setCachePolicy(ReloadIgnoringCacheData);
+        } else {
+            request.setCachePolicy(ReturnCacheDataElseLoad);
             action = NavigationAction(request, NavigationTypeFormResubmitted);
         }
@@ -3096 +3106 @@
                 // If the first load within a frame is a navigation within a back/forward list that was attached 
                 // without any of the items being loaded then we should use the default caching policy (<rdar://problem/8131355>).
-                if (m_stateMachine.committedFirstRealDocumentLoad() && !itemURL.protocolIs("https"))
+                if (m_stateMachine.committedFirstRealDocumentLoad())
                     request.setCachePolicy(ReturnCacheDataElseLoad);
                 break;
@@ -3107 +3117 @@
         }
 
-        addExtraFieldsToRequest(request, m_loadType, true);
+        addExtraFieldsToRequest(request, loadType, true);
 
         ResourceRequest requestForOriginalURL(request);
@@ -3127 +3137 @@
         loadSameDocumentItem(item);
     else
-        loadDifferentDocumentItem(item, loadType);
+        loadDifferentDocumentItem(item, loadType, MayAttemptCacheOnlyLoadForFormSubmissionItem);
+}
+
+void FrameLoader::retryAfterFailedCacheOnlyMainResourceLoad()
+{
+    ASSERT(m_state == FrameStateProvisional);
+    ASSERT(!m_loadingFromCachedPage);
+    // We only use cache-only loads to avoid resubmitting forms.
+    ASSERT(isBackForwardLoadType(m_loadType));
+    ASSERT(m_history.provisionalItem()->formData());
+    ASSERT(m_history.provisionalItem() == m_requestedHistoryItem.get());
+
+    FrameLoadType loadType = m_loadType;
+    HistoryItem* item = m_history.provisionalItem();
+
+    stopAllLoaders(ShouldNotClearProvisionalItem);
+    loadDifferentDocumentItem(item, loadType, MayNotAttemptCacheOnlyLoadForFormSubmissionItem);
 }
 

trunk/Source/WebCore/loader/FrameLoader.h

@@ -124 +124 @@
     HistoryItem* requestedHistoryItem() const { return m_requestedHistoryItem.get(); }
 
+    void retryAfterFailedCacheOnlyMainResourceLoad();
+
     static void reportLocalLoadFailed(Frame*, const String& url);
 
@@ -283 +285 @@
 
 private:
+    enum FormSubmissionCacheLoadPolicy {
+        MayAttemptCacheOnlyLoadForFormSubmissionItem,
+        MayNotAttemptCacheOnlyLoadForFormSubmissionItem
+    };
+
     bool allChildrenAreComplete() const; // immediate children, not all descendants
 
@@ -288 +295 @@
     
     void loadSameDocumentItem(HistoryItem*);
-    void loadDifferentDocumentItem(HistoryItem*, FrameLoadType);
+    void loadDifferentDocumentItem(HistoryItem*, FrameLoadType, FormSubmissionCacheLoadPolicy);
     
     void loadProvisionalItemFromCachedPage();

trunk/Source/WebCore/loader/MainResourceLoader.cpp

@@ -551 +551 @@
 {
     ASSERT_UNUSED(resource, m_resource == resource);
-    if (!m_resource || (!m_resource->errorOccurred() && !m_resource->wasCanceled())) {
+    ASSERT(m_resource);
+    if (!m_resource->errorOccurred() && !m_resource->wasCanceled()) {
         didFinishLoading(m_resource->loadFinishTime());
+        return;
+    }
+
+    if (m_documentLoader->request().cachePolicy() == ReturnCacheDataDontLoad && !m_resource->wasCanceled()) {
+        frameLoader()->retryAfterFailedCacheOnlyMainResourceLoad();
         return;
     }

trunk/Source/WebCore/platform/network/ResourceHandle.h

@@ -105 +105 @@
     static void loadResourceSynchronously(NetworkingContext*, const ResourceRequest&, StoredCredentials, ResourceError&, ResourceResponse&, Vector<char>& data);
 
-    static bool willLoadFromCache(ResourceRequest&, Frame*);
     static void cacheMetadata(const ResourceResponse&, const Vector<char>&);
 

trunk/Source/WebCore/platform/network/blackberry/ResourceHandleBlackBerry.cpp

@@ -127 +127 @@
 }
 
-bool ResourceHandle::willLoadFromCache(ResourceRequest&, Frame*)
-{
-    notImplemented();
-    return false;
-}
-
 void ResourceHandle::cancel()
 {

trunk/Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp

@@ -809 +809 @@
 }
 
-bool ResourceHandle::willLoadFromCache(ResourceRequest& request, Frame*)
-{
-    request.setCachePolicy(ReturnCacheDataDontLoad);
-    
-    CFURLResponseRef cfResponse = 0;
-    CFErrorRef cfError = 0;
-    RetainPtr<CFDataRef> data = adoptCF(CFURLConnectionSendSynchronousRequest(request.cfURLRequest(), &cfResponse, &cfError, request.timeoutInterval()));
-    bool cached = cfResponse && !cfError;
-
-    if (cfError)
-        CFRelease(cfError);
-    if (cfResponse)
-        CFRelease(cfResponse);
-
-    return cached;
-}
-
 #if PLATFORM(MAC)
 void ResourceHandle::schedule(SchedulePair* pair)

trunk/Source/WebCore/platform/network/chromium/ResourceHandle.cpp

@@ -278 +278 @@
 
 // static
-bool ResourceHandle::willLoadFromCache(ResourceRequest& request, Frame*)
-{
-    // This method is used to determine if a POST request can be repeated from
-    // cache, but you cannot really know until you actually try to read from the
-    // cache. Even if we checked now, something else could come along and wipe
-    // out the cache entry by the time we fetch it.
-    //
-    // So, we always say yes here, to prevent the FrameLoader from initiating a
-    // reload. Then in FrameLoaderClientImpl::dispatchWillSendRequest, we
-    // fix-up the cache policy of the request to force a load from the cache.
-    //
-    ASSERT(request.httpMethod() == "POST");
-    return true;
-}
-
-// static
 void ResourceHandle::cacheMetadata(const ResourceResponse& response, const Vector<char>& data)
 {

trunk/Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp

@@ -167 +167 @@
 }
 
-bool ResourceHandle::willLoadFromCache(ResourceRequest&, Frame*)
-{
-    notImplemented();
-    return false;
-}
-
 bool ResourceHandle::loadsBlocked()
 {

trunk/Source/WebCore/platform/network/mac/ResourceHandleMac.mm

@@ -358 +358 @@
 }
 
-bool ResourceHandle::willLoadFromCache(ResourceRequest& request, Frame*)
-{
-    request.setCachePolicy(ReturnCacheDataDontLoad);
-    NSURLResponse *nsURLResponse = nil;
-    BEGIN_BLOCK_OBJC_EXCEPTIONS;
-
-    [NSURLConnection sendSynchronousRequest:request.nsURLRequest() returningResponse:&nsURLResponse error:nil];
-    
-    END_BLOCK_OBJC_EXCEPTIONS;
-    
-    return nsURLResponse;
-}
-
 CFStringRef ResourceHandle::synchronousLoadRunLoopMode()
 {

trunk/Source/WebCore/platform/network/qt/ResourceHandleQt.cpp

@@ -126 +126 @@
 }
 
-bool ResourceHandle::willLoadFromCache(ResourceRequest& request, Frame* frame)
-{
-    if (!frame)
-        return false;
-
-    QNetworkAccessManager* manager = 0;
-    QAbstractNetworkCache* cache = 0;
-    if (frame->loader()->networkingContext()) {
-        manager = frame->loader()->networkingContext()->networkAccessManager();
-        cache = manager->cache();
-    }
-
-    if (!cache)
-        return false;
-
-    QNetworkCacheMetaData data = cache->metaData(request.url());
-    if (data.isValid()) {
-        request.setCachePolicy(ReturnCacheDataDontLoad);
-        return true;
-    }
-
-    return false;
-}
-
 void ResourceHandle::loadResourceSynchronously(NetworkingContext* context, const ResourceRequest& request, StoredCredentials /*storedCredentials*/, ResourceError& error, ResourceResponse& response, Vector<char>& data)
 {

trunk/Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp

@@ -1174 +1174 @@
 }
 
-bool ResourceHandle::willLoadFromCache(ResourceRequest&, Frame*)
-{
-    // Not having this function means that we'll ask the user about re-posting a form
-    // even when we go back to a page that's still in the cache.
-    notImplemented();
-    return false;
-}
-
 void ResourceHandle::loadResourceSynchronously(NetworkingContext* context, const ResourceRequest& request, StoredCredentials /*storedCredentials*/, ResourceError& error, ResourceResponse& response, Vector<char>& data)
 {

trunk/Source/WebCore/platform/network/win/ResourceHandleWin.cpp

@@ -432 +432 @@
 }
 
-bool ResourceHandle::willLoadFromCache(ResourceRequest&, Frame*)
+void prefetchDNS(const String&)
 {
     notImplemented();
+}
+
+bool ResourceHandle::loadsBlocked()
+{
     return false;
 }
 
-void prefetchDNS(const String&)
+void ResourceHandle::platformSetDefersLoading(bool)
 {
     notImplemented();
 }
 
-bool ResourceHandle::loadsBlocked()
-{
-    return false;
-}
-
-void ResourceHandle::platformSetDefersLoading(bool)
-{
-    notImplemented();
-}
-
 } // namespace WebCore