Changeset 42896 in webkit

Timestamp:

Apr 27, 2009 9:57:13 AM (15 years ago)

Author:

ap@webkit.org

Message:

Reviewed by Darin Adler.

​https://bugs.webkit.org/show_bug.cgi?id=25399
<rdar://problem/6633943> REGRESSION: Many crashes reported accessing Lexis/Nexis database,
beneath WebCore::Cache::evict

The crash happened because a cached resource handle was removed from a document's cached
resources map twice recursively, so a destructor was called for a value in a deleted bucket.
The first call was from Cache::evict, and when destroying CachedResourceHandle destroyed
CachedResource, DocLoader::removeCachedResource() was called again, with HashMap being in
an inconsistent state.

I couldn't fully reconstruct the loading sequence to make a test.

• loader/Cache.cpp: (WebCore::Cache::revalidateResource): Assert that the resource being revalidated is in cache (it makes no sense to revalidate one that isn't). (WebCore::Cache::evict): Don't remove the resource from document's map. Removing a resource from the cache in no way implies that documents no longer use the old version. This fixes the crash, and also fixes many cases of resource content being unavailable in Web Inspector.

• loader/CachedResource.h: (WebCore::CachedResource::setInCache): When bringing a revalidated resource back to cache, reset m_isBeingRevalidated to maintain the invariant of resources being revalidated never being in cache. This fixes another assertion I saw on LexisNexis search: in rare cases, switchClientsToRevalidatedResource() results in the same resource being requested again, but we were only enforcing CachedResource invariants after calling this function. (WebCore::CachedResource::unregisterHandle): Assert that the counter doesn't underflow.

• loader/DocLoader.cpp: (WebCore::DocLoader::removeCachedResource): Assert that the passed resource is removed, not some other resource that happens to have the same URL (this used to fail on LexisNexis search before this patch).

• loader/ImageDocument.cpp: (WebCore::ImageTokenizer::write): Replaced ASSERT_NOT_REACHED with notImplemented(). This method can be legally called via document.write(), and should work. LexisNexis takes this code path, but apparently has a fallback for Safari, so it doesn't affect site functionality.

• loader/CachedResource.cpp: (WebCore::CachedResource::clearResourceToRevalidate): Don't assert that m_resourceToRevalidate is being revalidated - this may no longer be true, because we now reset this member in CachedResource::setInCache().

Location:

trunk/WebCore

Files:

• ChangeLog (modified) (1 diff)
• loader/Cache.cpp (modified) (2 diffs)
• loader/CachedResource.cpp (modified) (2 diffs)
• loader/CachedResource.h (modified) (2 diffs)
• loader/DocLoader.cpp (modified) (1 diff)
• loader/ImageDocument.cpp (modified) (2 diffs)

trunk/WebCore/ChangeLog

@@ +1 @@
+2009-04-27  Alexey Proskuryakov  <ap@webkit.org>
+
+        Reviewed by Darin Adler.
+
+        https://bugs.webkit.org/show_bug.cgi?id=25399
+        <rdar://problem/6633943> REGRESSION: Many crashes reported accessing Lexis/Nexis database,
+        beneath WebCore::Cache::evict
+
+        The crash happened because a cached resource handle was removed from a document's cached
+        resources map twice recursively, so a destructor was called for a value in a deleted bucket.
+        The first call was from Cache::evict, and when destroying CachedResourceHandle destroyed
+        CachedResource, DocLoader::removeCachedResource() was called again, with HashMap being in
+        an inconsistent state.
+
+        I couldn't fully reconstruct the loading sequence to make a test.
+
+        * loader/Cache.cpp:
+        (WebCore::Cache::revalidateResource): Assert that the resource being revalidated is in cache
+        (it makes no sense to revalidate one that isn't).
+        (WebCore::Cache::evict): Don't remove the resource from document's map. Removing a resource
+        from the cache in no way implies that documents no longer use the old version. This fixes the
+        crash, and also fixes many cases of resource content being unavailable in Web Inspector.
+
+        * loader/CachedResource.h:
+        (WebCore::CachedResource::setInCache): When bringing a revalidated resource back to cache,
+        reset m_isBeingRevalidated to maintain the invariant of resources being revalidated never
+        being in cache. This fixes another assertion I saw on LexisNexis search: in rare cases,
+        switchClientsToRevalidatedResource() results in the same resource being requested again,
+        but we were only enforcing CachedResource invariants after calling this function.
+        (WebCore::CachedResource::unregisterHandle): Assert that the counter doesn't underflow.
+
+        * loader/DocLoader.cpp: (WebCore::DocLoader::removeCachedResource): Assert that the passed
+        resource is removed, not some other resource that happens to have the same URL (this used to
+        fail on LexisNexis search before this patch).
+
+        * loader/ImageDocument.cpp: (WebCore::ImageTokenizer::write): Replaced ASSERT_NOT_REACHED
+        with notImplemented(). This method can be legally called via document.write(), and should
+        work. LexisNexis takes this code path, but apparently has a fallback for Safari, so it
+        doesn't affect site functionality.
+
+        * loader/CachedResource.cpp:
+        (WebCore::CachedResource::clearResourceToRevalidate): Don't assert that m_resourceToRevalidate
+        is being revalidated - this may no longer be true, because we now reset this member in
+        CachedResource::setInCache().
+
 2009-04-27  Dan Bernstein  <mitz@apple.com>
 

trunk/WebCore/loader/Cache.cpp

@@ -182 +182 @@
 {
     ASSERT(resource);
+    ASSERT(resource->inCache());
     ASSERT(!disabled());
     if (resource->resourceToRevalidate())
@@ -391 +392 @@
     // who needed a fresh copy for a reload. See <http://bugs.webkit.org/show_bug.cgi?id=12479#c6>.
     if (resource->inCache()) {
-        if (!resource->isCacheValidator()) {
-            // Notify all doc loaders that might be observing this object still that it has been
-            // extracted from the set of resources.
-            // No need to do this for cache validator resources, they are replaced automatically by using CachedResourceHandles.
-            HashSet<DocLoader*>::iterator end = m_docLoaders.end();
-            for (HashSet<DocLoader*>::iterator itr = m_docLoaders.begin(); itr != end; ++itr)
-                (*itr)->removeCachedResource(resource);
-        }
-        
         // Remove from the resource map.
         m_resources.remove(resource->url());

trunk/WebCore/loader/CachedResource.cpp

@@ -259 +259 @@
 { 
     ASSERT(m_resourceToRevalidate);
-    ASSERT(m_resourceToRevalidate->m_isBeingRevalidated);
     m_resourceToRevalidate->m_isBeingRevalidated = false;
     m_resourceToRevalidate->deleteIfPossible();
@@ -270 +269 @@
 {
     ASSERT(m_resourceToRevalidate);
+    ASSERT(m_resourceToRevalidate->inCache());
     ASSERT(!inCache());
 

trunk/WebCore/loader/CachedResource.h

@@ -128 +128 @@
     // while still being referenced. This means the object should delete itself
     // if the number of clients observing it ever drops to 0.
-    void setInCache(bool b) { m_inCache = b; }
+    // The resource can be brought back to cache after successful revalidation.
+    void setInCache(bool b) { m_inCache = b; if (b) m_isBeingRevalidated = false; }
     bool inCache() const { return m_inCache; }
     
@@ -164 +165 @@
     
     void registerHandle(CachedResourceHandleBase* h) { ++m_handleCount; if (m_resourceToRevalidate) m_handlesToRevalidate.add(h); }
-    void unregisterHandle(CachedResourceHandleBase* h) { --m_handleCount; if (m_resourceToRevalidate) m_handlesToRevalidate.remove(h); if (!m_handleCount) deleteIfPossible(); }
+    void unregisterHandle(CachedResourceHandleBase* h) { ASSERT(m_handleCount > 0); --m_handleCount; if (m_resourceToRevalidate) m_handlesToRevalidate.remove(h); if (!m_handleCount) deleteIfPossible(); }
     
     bool canUseCacheValidator() const;

trunk/WebCore/loader/DocLoader.cpp

@@ -277 +277 @@
 void DocLoader::removeCachedResource(CachedResource* resource) const
 {
+#ifndef NDEBUG
+    DocumentResourceMap::iterator it = m_documentResources.find(resource->url());
+    if (it != m_documentResources.end())
+        ASSERT(it->second.get() == resource);
+#endif
     m_documentResources.remove(resource->url());
 }

trunk/WebCore/loader/ImageDocument.cpp

@@ -38 +38 @@
 #include "LocalizedStrings.h"
 #include "MouseEvent.h"
+#include "NotImplemented.h"
 #include "Page.h"
 #include "SegmentedString.h"
@@ -94 +95 @@
 void ImageTokenizer::write(const SegmentedString&, bool)
 {
-    ASSERT_NOT_REACHED();
+    // <https://bugs.webkit.org/show_bug.cgi?id=25397>: JS code can always call document.write, we need to handle it.
+    notImplemented();
 }