WebKit/mac:
authorbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 27 Feb 2008 23:09:00 +0000 (23:09 +0000)
committerbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 27 Feb 2008 23:09:00 +0000 (23:09 +0000)
        Reviewed by Mark Rowe (code) and Darin (concept)

        Much better fix for <rdar://problem/4930688> (see r19549)
        Original fix for <rdar://problem/3947312> (and 14 dupes)

        Let me tell you a story:
        A long time ago, in a cvs repository far, far away, loader code was almost all up in WebKit.
        WebArchive code was intertwined with that code in bizarre and complex ways.
        During the months long loader re-factoring where we pushed much loader code down into WebCore,
        many portions of the WebKit loader were thinned out until they ceased to exist.  Others remained
        with a sole purpose.

        One such section of code whose lineage traces back from WebFrameLoaderClient to WebFrameLoader
        to WebLoader was originally rooted in the method [WebLoader loadRequest:].  This method was the
        single entry point for almost all loading (network or web archives)

        This method would check various headers and other fields on the NSURLRequest and NSURLResponse
        to make decisions about the load.  If the cache control fields were expired or other conditions
        in the headers were met, the load would be forced to go out to the network.

        As the loader was moved and tweaked repeatedly, most of this code was pruned or re-factored.
        At some point, all that remained was the special cases for loading WebArchives.

        Somewhere in the r16,000s, this remaining responsibility was noticed and related methods we renamed
        to be WebArchive specific, further cementing the assumed design.

        Problem is, the design was bad.  A WebArchive is meant to be a static snapshot of a WebPage at a
        specific point in time.  Referring to the request to see if the resource should be reloaded seems
        nonsensical, as does referring to the response headers to see if the resource is "expired".  In the
        context of loading a WebArchive, available data should *always* be loaded from the WebArchive, at least
        during the initial load!

        After discovering the secret to reproducing all of these bugs is both emptying our your Foundation
        cache and disconnecting your network, it was easy to reproduce the 16 individually reported cases
        that were all symptoms of this bug, and easy to verify that they are fixed with this patch.

        * WebCoreSupport/WebFrameLoaderClient.h:
        * WebCoreSupport/WebFrameLoaderClient.mm:
        (WebFrameLoaderClient::willUseArchive): Do not call either form of "canUseArchivedResource()" that
          inspect the request or response objects - We are loading from a WebArchive, and we should never
          make the decision to go out to the network when we actually have the resource available.

        * WebCoreSupport/WebSystemInterface.m:
        (InitWebCoreSystemInterface):  Remove two methods that are no longer used anywhere in WebKit

LayoutTests:

        Reviewed by Mark Rowe

        Test for better fix for <rdar://problem/4930688> (see r19549) and
        original fix for <rdar://problem/3947312> (and 14 dupes)

        Crafting custom WebArchives for layout tests is a pain and something that should
        be resolved if we decide to pursue a new format.

        Using a custom php script to act as a stand in for an image resource, I set its
        cache-control header to expire immediately.  Without the fix for the above bugs,
        the resource will be "expired" and an attempt to fetch it from the network will
        go out and fail.  This failure will manifest with different ResourceLoadDelegate
        information, as well as different dimensions in the render tree - the missing
        image icon versus the archived image.

        With the fix in place, the response will be ignored and the image will be pulled
        from the WebArchive.

        * webarchive/loading/cache-expired-subresource-expected.txt: Added.
        * webarchive/loading/cache-expired-subresource.html: Added.
        * webarchive/loading/resources/cache-expired-subresource.webarchive: Added.

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

LayoutTests/ChangeLog
LayoutTests/webarchive/loading/cache-expired-subresource-expected.txt [new file with mode: 0644]
LayoutTests/webarchive/loading/cache-expired-subresource.html [new file with mode: 0644]
LayoutTests/webarchive/loading/resources/cache-expired-subresource.webarchive [new file with mode: 0644]
WebKit/mac/ChangeLog
WebKit/mac/WebCoreSupport/WebFrameLoaderClient.h
WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm
WebKit/mac/WebCoreSupport/WebSystemInterface.m

index 5cd8a48..557202b 100644 (file)
@@ -1,3 +1,27 @@
+2008-02-27  Brady Eidson  <beidson@apple.com>
+
+        Reviewed by Mark Rowe
+
+        Test for better fix for <rdar://problem/4930688> (see r19549) and
+        original fix for <rdar://problem/3947312> (and 14 dupes)
+        
+        Crafting custom WebArchives for layout tests is a pain and something that should
+        be resolved if we decide to pursue a new format.
+
+        Using a custom php script to act as a stand in for an image resource, I set its 
+        cache-control header to expire immediately.  Without the fix for the above bugs,
+        the resource will be "expired" and an attempt to fetch it from the network will
+        go out and fail.  This failure will manifest with different ResourceLoadDelegate
+        information, as well as different dimensions in the render tree - the missing 
+        image icon versus the archived image.
+
+        With the fix in place, the response will be ignored and the image will be pulled
+        from the WebArchive.
+
+        * webarchive/loading/cache-expired-subresource-expected.txt: Added.
+        * webarchive/loading/cache-expired-subresource.html: Added.
+        * webarchive/loading/resources/cache-expired-subresource.webarchive: Added.
+
 2008-02-27  Kevin McCullough  <kmccullough@apple.com>
 
         Landing test that was forgotten in the original patch (r30087).
diff --git a/LayoutTests/webarchive/loading/cache-expired-subresource-expected.txt b/LayoutTests/webarchive/loading/cache-expired-subresource-expected.txt
new file mode 100644 (file)
index 0000000..4b51c83
--- /dev/null
@@ -0,0 +1,42 @@
+main frame - didStartProvisionalLoadForFrame
+main frame - didCommitLoadForFrame
+frame "<!--framePath //<!--frame0-->-->" - didStartProvisionalLoadForFrame
+resources/cache-expired-subresource.webarchive - willSendRequest <NSURLRequest resources/cache-expired-subresource.webarchive> redirectResponse (null)
+main frame - didFinishDocumentLoadForFrame
+<unknown> - didFinishLoading
+resources/cache-expired-subresource.webarchive - didReceiveResponse <NSURLResponse resources/cache-expired-subresource.webarchive>
+frame "<!--framePath //<!--frame0-->-->" - didCommitLoadForFrame
+http://localhost/pink-bullet.png - willSendRequest <NSURLRequest http://localhost/pink-bullet.png> redirectResponse (null)
+frame "<!--framePath //<!--frame0-->-->" - didFinishDocumentLoadForFrame
+resources/cache-expired-subresource.webarchive - didFinishLoading
+http://localhost/pink-bullet.png - didReceiveResponse <NSURLResponse http://localhost/test.php>
+frame "<!--framePath //<!--frame0-->-->" - didHandleOnloadEventsForFrame
+main frame - didHandleOnloadEventsForFrame
+frame "<!--framePath //<!--frame0-->-->" - didFinishLoadForFrame
+main frame - didFinishLoadForFrame
+layer at (0,0) size 800x600
+  RenderView at (0,0) size 800x600
+layer at (0,0) size 800x600
+  RenderBlock {HTML} at (0,0) size 800x600
+    RenderBody {BODY} at (8,8) size 784x584
+      RenderPartObject {IFRAME} at (0,0) size 304x154 [border: (2px inset #000000)]
+        layer at (0,0) size 300x150
+          RenderView at (0,0) size 300x150
+        layer at (0,0) size 300x150
+          RenderBlock {HTML} at (0,0) size 300x150
+            RenderBody {BODY} at (8,8) size 284x134
+              RenderText {#text} at (0,0) size 233x18
+                text run at (0,0) width 233: "This page has a small pink circle in it"
+              RenderBR {BR} at (233,14) size 0x0
+              RenderImage {IMG} at (0,18) size 11x9
+              RenderBR {BR} at (11,27) size 0x0
+              RenderText {#text} at (0,27) size 159x18
+                text run at (0,27) width 104: "See, right there! "
+                text run at (104,27) width 55: "A circle!"
+              RenderBR {BR} at (159,41) size 0x0
+      RenderBR {BR} at (304,154) size 0x0
+      RenderText {#text} at (0,154) size 777x36
+        text run at (0,154) width 777: "This tests that loading a webarchive with a subresource that has an expired cache header does not attempt to hit the network"
+        text run at (0,172) width 301: "for resource, and actually uses the archived data"
+      RenderText {#text} at (0,0) size 0x0
+      RenderText {#text} at (0,0) size 0x0
diff --git a/LayoutTests/webarchive/loading/cache-expired-subresource.html b/LayoutTests/webarchive/loading/cache-expired-subresource.html
new file mode 100644 (file)
index 0000000..6fd3826
--- /dev/null
@@ -0,0 +1,17 @@
+<html>
+<script>
+    if (window.layoutTestController) {
+        layoutTestController.dumpResourceLoadCallbacks();
+        layoutTestController.waitUntilDone();
+    }
+    
+    function frameLoaded() {
+        if (window.layoutTestController)
+            layoutTestController.notifyDone();
+    }
+</script>
+<body>
+<iframe onload="frameLoaded();" src="resources/cache-expired-subresource.webarchive"></iframe><br>
+This tests that loading a webarchive with a subresource that has an expired cache header does not attempt to hit the network for resource, and actually uses the archived data
+</body>
+</html>
diff --git a/LayoutTests/webarchive/loading/resources/cache-expired-subresource.webarchive b/LayoutTests/webarchive/loading/resources/cache-expired-subresource.webarchive
new file mode 100644 (file)
index 0000000..39866fd
--- /dev/null
@@ -0,0 +1,78 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<plist version="1.0">
+<dict>
+       <key>WebMainResource</key>
+       <dict>
+               <key>WebResourceData</key>
+               <data>
+               PGh0bWw+PGhlYWQ+CjwvaGVhZD48Ym9keT4KVGhpcyBwYWdlIGhhcyBhIHNt
+               YWxsIHBpbmsgY2lyY2xlIGluIGl0PGJyPgo8aW1nIHNyYz0icGluay1idWxs
+               ZXQucG5nIj48YnI+ClNlZSwgcmlnaHQgdGhlcmUhICBBIGNpcmNsZSE8YnI+
+               Cgo8L2JvZHk+PC9odG1sPg==
+               </data>
+               <key>WebResourceFrameName</key>
+               <string></string>
+               <key>WebResourceMIMEType</key>
+               <string>text/html</string>
+               <key>WebResourceTextEncodingName</key>
+               <string>UTF-8</string>
+               <key>WebResourceURL</key>
+               <string>http://localhost/</string>
+       </dict>
+       <key>WebSubresources</key>
+       <array>
+               <dict>
+                       <key>WebResourceData</key>
+                       <data>
+                       iVBORw0KGgoAAAANSUhEUgAAAAsAAAAJCAYAAADkZNYtAAAABGdB
+                       TUEAANbY1E9YMgAAABl0RVh0U29mdHdhcmUAQWRvYmUgSW1hZ2VS
+                       ZWFkeXHJZTwAAAF7SURBVHjaLM/fK0NhGAfw7/ucM2djm20syzDW
+                       alrD/Ei5UFMk/4C4kRQ3biV/gks3Su5IuXBpS0SUuHHhQiI/Etpm
+                       +XHCqb2znXO8R556nov3+fTteRn+qwmyt7u3f2Z4YHhcL5aYpNjM
+                       nb3U5tH56eonDNUy8r/1RavqlxeSE2PxkSSoUoGhcbRzb4d29ZA4
+                       4JlZYT4kMagZFdNrxvycvyGMklpA+Y2jcPkO17MdyYvmeNo8yon0
+                       MyvZ1gXPqFMXafcqWKYA5lbAvoowOIf13gnv6BNeV/5wPTxhb6AG
+                       1FgLm2KHpMjQ3WWUihzGs4HgiycMvNosTHnGs85GZ8BsqdbJ72Jw
+                       V4B+ypDVb5QzGuXyxSxMEAlsHJv59DU9QO6pk6jNTRR2kBR1kNwT
+                       oCt+hxOxt5z1QaZBz96wXKQ31BqpTYQAvwNwEM639jG1u5i619Ul
+                       4d6YdYZoVzAYjPkk++RQrG9QZmZI183HjcPtVL70vS72t6K1XwEG
+                       AGShiOMS6uVKAAAAAElFTkSuQmCC
+                       </data>
+                       <key>WebResourceMIMEType</key>
+                       <string>image/png</string>
+                       <key>WebResourceResponse</key>
+                       <data>
+                       YnBsaXN0MDDUAQIDBAUGCQpYJHZlcnNpb25UJHRvcFkkYXJjaGl2
+                       ZXJYJG9iamVjdHMSAAGGoNEHCF8QE1dlYlJlc291cmNlUmVzcG9u
+                       c2WAAV8QD05TS2V5ZWRBcmNoaXZlcq8QHAsMJiwtMx40NUtMTU5P
+                       UFFSU1RVVldYWU1aXl9VJG51bGzdDQ4PEBESExQVFhcYGRobHB0e
+                       HyAhIiMkHSVWJGNsYXNzUiQzUiQ4UyQxMFMkMTFSJDVSJDZSJDRS
+                       JDdSJDJSJDlSJDBSJDGAG4ACgBoQCBAAgAaAB4AFgAgQB4AAEAHT
+                       DScoKSQrV05TLmJhc2VbTlMucmVsYXRpdmWABIAAgANfEBlodHRw
+                       Oi8vbG9jYWxob3N0L3Rlc3QucGhw0i4vMDFYJGNsYXNzZXNaJGNs
+                       YXNzbmFtZaIxMlVOU1VSTFhOU09iamVjdCNBqupvX+dELBDI0w02
+                       Nzg5QldOUy5rZXlzWk5TLm9iamVjdHOAGag6Ozw9Pj9AQYAJgAqA
+                       C4AMgA2ADoAPgBCoQ0RFRkdISUqAEYASgBOAFIAVgBaAF4AYXFgt
+                       UG93ZXJlZC1CeV1DYWNoZS1Db250cm9sWktlZXAtQWxpdmVWU2Vy
+                       dmVyXENvbnRlbnQtVHlwZVREYXRlXkNvbnRlbnQtTGVuZ3RoWkNv
+                       bm5lY3Rpb25ZUEhQLzUuMi40WW1heC1hZ2U9MF8QEnRpbWVvdXQ9
+                       NSwgbWF4PTEwMF8QQEFwYWNoZS8yLjIuNiAoVW5peCkgbW9kX3Nz
+                       bC8yLjIuNiBPcGVuU1NMLzAuOS43bCBEQVYvMiBQSFAvNS4yLjRZ
+                       aW1hZ2UvcG5nXxAdV2VkLCAyNyBGZWIgMjAwOCAwNjoxNjoxNSBH
+                       TVRTNDkw0i4vW1yjXF0yXxATTlNNdXRhYmxlRGljdGlvbmFyeVxO
+                       U0RpY3Rpb25hcnkRAerSLi9gYaNhYjJfEBFOU0hUVFBVUkxSZXNw
+                       b25zZV1OU1VSTFJlc3BvbnNlAAgAEQAaAB8AKQAyADcAOgBQAFIA
+                       ZACDAIkApACrAK4AsQC1ALkAvAC/AMIAxQDIAMsAzgDRANMA1QDX
+                       ANkA2wDdAN8A4QDjAOUA5wDpAPAA+AEEAQYBCAEKASYBKwE0AT8B
+                       QgFIAVEBWgFcAWMBawF2AXgBgQGDAYUBhwGJAYsBjQGPAZEBmgGc
+                       AZ4BoAGiAaQBpgGoAaoBtwHFAdAB1wHkAekB+AIDAg0CFwIsAm8C
+                       eQKZAp0CogKmArwCyQLMAtEC1QLpAAAAAAAAAgEAAAAAAAAAYwAA
+                       AAAAAAAAAAAAAAAAAvc=
+                       </data>
+                       <key>WebResourceURL</key>
+                       <string>http://localhost/pink-bullet.png</string>
+               </dict>
+       </array>
+</dict>
+</plist>
index 1099220..a4af12c 100644 (file)
@@ -1,3 +1,50 @@
+2008-02-27  Brady Eidson  <beidson@apple.com>
+
+        Reviewed by Mark Rowe (code) and Darin (concept)
+
+        Much better fix for <rdar://problem/4930688> (see r19549)
+        Original fix for <rdar://problem/3947312> (and 14 dupes)
+        
+        Let me tell you a story:
+        A long time ago, in a cvs repository far, far away, loader code was almost all up in WebKit.
+        WebArchive code was intertwined with that code in bizarre and complex ways.
+        During the months long loader re-factoring where we pushed much loader code down into WebCore,
+        many portions of the WebKit loader were thinned out until they ceased to exist.  Others remained
+        with a sole purpose.
+
+        One such section of code whose lineage traces back from WebFrameLoaderClient to WebFrameLoader
+        to WebLoader was originally rooted in the method [WebLoader loadRequest:].  This method was the 
+        single entry point for almost all loading (network or web archives)
+
+        This method would check various headers and other fields on the NSURLRequest and NSURLResponse 
+        to make decisions about the load.  If the cache control fields were expired or other conditions 
+        in the headers were met, the load would be forced to go out to the network.
+
+        As the loader was moved and tweaked repeatedly, most of this code was pruned or re-factored.  
+        At some point, all that remained was the special cases for loading WebArchives.  
+        
+        Somewhere in the r16,000s, this remaining responsibility was noticed and related methods we renamed
+        to be WebArchive specific, further cementing the assumed design.
+
+        Problem is, the design was bad.  A WebArchive is meant to be a static snapshot of a WebPage at a
+        specific point in time.  Referring to the request to see if the resource should be reloaded seems
+        nonsensical, as does referring to the response headers to see if the resource is "expired".  In the 
+        context of loading a WebArchive, available data should *always* be loaded from the WebArchive, at least
+        during the initial load!
+
+        After discovering the secret to reproducing all of these bugs is both emptying our your Foundation 
+        cache and disconnecting your network, it was easy to reproduce the 16 individually reported cases 
+        that were all symptoms of this bug, and easy to verify that they are fixed with this patch.
+
+        * WebCoreSupport/WebFrameLoaderClient.h:
+        * WebCoreSupport/WebFrameLoaderClient.mm:
+        (WebFrameLoaderClient::willUseArchive): Do not call either form of "canUseArchivedResource()" that
+          inspect the request or response objects - We are loading from a WebArchive, and we should never
+          make the decision to go out to the network when we actually have the resource available.
+
+        * WebCoreSupport/WebSystemInterface.m:
+        (InitWebCoreSystemInterface):  Remove two methods that are no longer used anywhere in WebKit
+
 2008-02-27  Matt Lilek  <webkit@mattlilek.com>
 
         Reviewed by Adam Roben.
index 0df9f04..12bcc51 100644 (file)
@@ -199,8 +199,6 @@ private:
     virtual void registerForIconNotification(bool listen);
 
     void deliverArchivedResourcesAfterDelay() const;
-    bool canUseArchivedResource(NSURLRequest *) const;
-    bool canUseArchivedResource(NSURLResponse *) const;
     void deliverArchivedResources(WebCore::Timer<WebFrameLoaderClient>*);
 
     void setOriginalURLForDownload(WebDownload *, const WebCore::ResourceRequest&) const;
index cbe95e2..4bcfeaf 100644 (file)
@@ -778,16 +778,15 @@ bool WebFrameLoaderClient::willUseArchive(ResourceLoader* loader, const Resource
 {
     if (request.url() != originalURL)
         return false;
-    if (!canUseArchivedResource(request.nsURLRequest()))
-        return false;
+
     WebResource *resource = [dataSource(core(m_webFrame.get())->loader()->activeDocumentLoader()) _archivedSubresourceForURL:originalURL];
     if (!resource)
         return false;
-    if (!canUseArchivedResource([resource _response]))
-        return false;
+
     m_pendingArchivedResources.set(loader, resource);
     // Deliver the resource after a delay because callers don't expect to receive callbacks while calling this method.
     deliverArchivedResourcesAfterDelay();
+
     return true;
 }
 
@@ -940,37 +939,6 @@ void WebFrameLoaderClient::setTitle(const String& title, const KURL& URL)
     [[[WebHistory optionalSharedHistory] itemForURL:nsURL] setTitle:titleNSString];
 }
 
-// The following 2 functions are copied from [NSHTTPURLProtocol _cachedResponsePassesValidityChecks] and modified for our needs.
-// FIXME: It would be nice to eventually to share this logic somehow.
-bool WebFrameLoaderClient::canUseArchivedResource(NSURLRequest *request) const
-{
-    NSURLRequestCachePolicy policy = [request cachePolicy];
-    if (policy == NSURLRequestReturnCacheDataElseLoad)
-        return true;
-    if (policy == NSURLRequestReturnCacheDataDontLoad)
-        return true;
-    if ([request valueForHTTPHeaderField:@"must-revalidate"] != nil)
-        return false;
-    if ([request valueForHTTPHeaderField:@"proxy-revalidate"] != nil)
-        return false;
-    if ([request valueForHTTPHeaderField:@"If-Modified-Since"] != nil)
-        return false;
-    if ([request valueForHTTPHeaderField:@"Cache-Control"] != nil)
-        return false;
-    if ([@"POST" _webkit_isCaseInsensitiveEqualToString:[request HTTPMethod]])
-        return false;
-    return true;
-}
-
-bool WebFrameLoaderClient::canUseArchivedResource(NSURLResponse *response) const
-{
-    if (WKGetNSURLResponseMustRevalidate(response))
-        return false;
-    if (WKGetNSURLResponseCalculatedExpiration(response) - CFAbsoluteTimeGetCurrent() < 1)
-        return false;
-    return true;
-}
-
 void WebFrameLoaderClient::deliverArchivedResourcesAfterDelay() const
 {
     if (m_pendingArchivedResources.isEmpty())
index de9b1ac..fd237a6 100644 (file)
@@ -71,9 +71,7 @@ void InitWebCoreSystemInterface(void)
     INIT(GetGlyphVectorRecordSize);
     INIT(GetMIMETypeForExtension);
     INIT(GetNSFontATSUFontId);
-    INIT(GetNSURLResponseCalculatedExpiration);
     INIT(GetNSURLResponseLastModifiedDate);
-    INIT(GetNSURLResponseMustRevalidate);
     INIT(GetPreferredExtensionForMIMEType);
     INIT(GetWheelEventDeltas);
     INIT(InitializeGlyphVector);