Changeset 20178 in webkit

Timestamp:

Mar 13, 2007, 8:59:08 PM (18 years ago)

Author:

beidson

Message:

Reviewed by Anders

<rdar://problem/5048818> - REGRESSION: Incompletely loaded resources being saved to the object cache

Due to a subtle change in loader behavior back in 10904, we would stop all loaders before calling
didFail() on them in the Cache loader. As a result, we basically cleared all of the Subresource Loaders
out of the Cache loader before more properly failing them as errored out. The result? Partially loaded
resources being cached.

Since Loader::didFail() both calls error() on the object *and* removes the loader, the solution is to call
didFail() for all cancelled loaders instead of *only* removing them from the set of active loaders.

In addition, pages that didn't completely load were being saved to the back/forward cache. To fix that,
I added a null check on the DocumentLoader's error to see if the page ended in an error, or did indeed
completely load.

Note that the layout test for this - if possible - will require other enhancements including possibly adding
support for window.stop(). That task is documented in <rdar://problem/5061826>

• loader/FrameLoader.cpp: (WebCore::FrameLoader::provisionalLoadStarted): Fixed a few bugs relating to my original BFCache rewrite to more perfectly restore the original behavior - including only caching HTML documents via the m_client->canCachePage() call (WebCore::FrameLoader::canCachePage): Don't make the call to m_client->canCachePage() as that serves a different purpose
  • Check the mainDocumentError to see if the load ended in error as a further criteria in determining the cachability of a page

• loader/loader.cpp: (WebCore::Loader::cancelRequests): Call didFail(cancelledError()) instead of just removing the loaders from the loaders-in-progress set. This adds the effect of properly cleaning up the cached object.

Location:

trunk/WebCore

Files:

• ChangeLog (modified) (1 diff)
• loader/FrameLoader.cpp (modified) (2 diffs)
• loader/loader.cpp (modified) (1 diff)

trunk/WebCore/ChangeLog

@@ +1 @@
+2007-03-13  Brady Eidson  <beidson@apple.com>
+
+        Reviewed by Anders
+
+        <rdar://problem/5048818> - REGRESSION: Incompletely loaded resources being saved to the object cache
+
+        Due to a subtle change in loader behavior back in 10904, we would stop all loaders before calling 
+        didFail() on them in the Cache loader.  As a result, we basically cleared all of the Subresource Loaders 
+        out of the Cache loader before more properly failing them as errored out.  The result?  Partially loaded
+        resources being cached.
+        
+        Since Loader::didFail() both calls error() on the object *and* removes the loader, the solution is to call 
+        didFail() for all cancelled loaders instead of *only* removing them from the set of active loaders.
+        
+        In addition, pages that didn't completely load were being saved to the back/forward cache.  To fix that,
+        I added a null check on the DocumentLoader's error to see if the page ended in an error, or did indeed 
+        completely load.
+
+        Note that the layout test for this - if possible - will require other enhancements including possibly adding
+        support for window.stop().  That task is documented in <rdar://problem/5061826>
+
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::provisionalLoadStarted): Fixed a few bugs relating to my original BFCache rewrite to
+          more perfectly restore the original behavior - including only caching HTML documents via the 
+          m_client->canCachePage() call
+        (WebCore::FrameLoader::canCachePage): Don't make the call to m_client->canCachePage() as that serves a different
+          purpose
+          - Check the mainDocumentError to see if the load ended in error as a further criteria in determining the 
+          cachability of a page
+
+        * loader/loader.cpp:
+        (WebCore::Loader::cancelRequests): Call didFail(cancelledError()) instead of just removing the loaders from the 
+          loaders-in-progress set.  This adds the effect of properly cleaning up the cached object.
+
 2007-03-13  Beth Dakin  <bdakin@apple.com>
 

trunk/WebCore/loader/FrameLoader.cpp

@@ -1479 +1479 @@
         && loadType != FrameLoadTypeReloadAllowingStaleData
         && loadType != FrameLoadTypeSame
-        && !documentLoader()->isLoading()
+        && !documentLoader()->isLoadingInAPISense()
         && !documentLoader()->isStopping()) {
-        
-        if (!m_currentHistoryItem->pageCache()) {
-            // Add the items to this page's cache.
-            if (createPageCache(m_currentHistoryItem.get())) {
-                // See if any page caches need to be purged after the addition of this new page cache.
-                purgePageCache();
+        if (m_client->canCachePage()) {
+            if (!m_currentHistoryItem->pageCache()) {
+                // Add the items to this page's cache.
+                if (createPageCache(m_currentHistoryItem.get())) {
+                    // See if any page caches need to be purged after the addition of this new page cache.
+                    purgePageCache();
+                }
             }
+        } else {
+            // Put the document into a null state, so it can be restored correctly. 
+            clear();
         }
     }
@@ -1532 +1536 @@
 
 bool FrameLoader::canCachePage()
-{
-    if (!m_client->canCachePage())
+{    
+    if (documentLoader() && !documentLoader()->mainDocumentError().isNull())
         return false;
-        
+
     return m_frame->document()
         && !m_frame->tree()->childCount()

trunk/WebCore/loader/loader.cpp

@@ -224 +224 @@
     for (unsigned i = 0; i < loadersToCancel.size(); ++i) {
         SubresourceLoader* loader = loadersToCancel[i];
-        Request* r = m_requestsLoading.get(loader);
-        m_requestsLoading.remove(loader);
-        cache()->remove(r->cachedResource());
+        didFail(loader, loader->cancelledError());
     }
 }