Don't reuse cached stylesheet with failed or canceled resource loads

https://bugs.webkit.org/show_bug.cgi?id=93203

Reviewed by Simon Fraser.

1) Go to apple.com
2) Reload repeatedly

Eventually you can get into state where some images don't load.

The problem is that a cached stylesheet may end up pointing to image resources that have been canceled (by the reload).
If this happens they stay in the canceled state even when the stylesheet is applied to a new document.

Fix by checking if all loads are complete (or pending) when restoring a cached stylesheet. The sheet is only used
if there are no failed or canceled loads. There are potential more sophisticated fixes but this is simple and safe.
Walking the sheet is fast and since it is only done on cache restore the cost is minimal.

No regression test yet though the new code does get exercised by the existing tests.

* css/CSSCrossfadeValue.cpp:
(WebCore::CSSCrossfadeValue::hasFailedOrCanceledSubresources):
(WebCore):
* css/CSSCrossfadeValue.h:
(CSSCrossfadeValue):
* css/CSSFontFaceSrcValue.cpp:
(WebCore::CSSFontFaceSrcValue::hasFailedOrCanceledSubresources):
(WebCore):
* css/CSSFontFaceSrcValue.h:
(CSSFontFaceSrcValue):
* css/CSSImageSetValue.cpp:
(WebCore::CSSImageSetValue::hasFailedOrCanceledSubresources):
(WebCore):
* css/CSSImageSetValue.h:
(CSSImageSetValue):
* css/CSSImageValue.cpp:
(WebCore::CSSImageValue::hasFailedOrCanceledSubresources):
(WebCore):
* css/CSSImageValue.h:
(CSSImageValue):
* css/CSSValue.cpp:
(WebCore::CSSValue::hasFailedOrCanceledSubresources):
(WebCore):
* css/CSSValue.h:
(CSSValue):
* css/CSSValueList.cpp:
(WebCore::CSSValueList::hasFailedOrCanceledSubresources):
(WebCore):
* css/CSSValueList.h:
(CSSValueList):
* css/StylePropertySet.cpp:
(WebCore::StylePropertySet::hasFailedOrCanceledSubresources):
(WebCore):
* css/StylePropertySet.h:
(StylePropertySet):
* css/StyleSheetContents.cpp:
(WebCore::childRulesHaveFailedOrCanceledSubresources):
(WebCore):
(WebCore::StyleSheetContents::hasFailedOrCanceledSubresources):
* css/StyleSheetContents.h:
(StyleSheetContents):
* loader/cache/CachedCSSStyleSheet.cpp:
(WebCore::CachedCSSStyleSheet::restoreParsedStyleSheet):
* loader/cache/CachedResource.h:
(WebCore::CachedResource::loadFailedOrCanceled):

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

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 7b8fe3e..5dd5fa0 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,70 @@
+2012-08-05  Antti Koivisto  <antti@apple.com>
+
+        Don't reuse cached stylesheet with failed or canceled resource loads
+        https://bugs.webkit.org/show_bug.cgi?id=93203
+
+        Reviewed by Simon Fraser.
+
+        1) Go to apple.com
+        2) Reload repeatedly
+
+        Eventually you can get into state where some images don't load.
+        
+        The problem is that a cached stylesheet may end up pointing to image resources that have been canceled (by the reload).
+        If this happens they stay in the canceled state even when the stylesheet is applied to a new document.
+        
+        Fix by checking if all loads are complete (or pending) when restoring a cached stylesheet. The sheet is only used
+        if there are no failed or canceled loads. There are potential more sophisticated fixes but this is simple and safe.
+        Walking the sheet is fast and since it is only done on cache restore the cost is minimal.
+
+        No regression test yet though the new code does get exercised by the existing tests.
+
+        * css/CSSCrossfadeValue.cpp:
+        (WebCore::CSSCrossfadeValue::hasFailedOrCanceledSubresources):
+        (WebCore):
+        * css/CSSCrossfadeValue.h:
+        (CSSCrossfadeValue):
+        * css/CSSFontFaceSrcValue.cpp:
+        (WebCore::CSSFontFaceSrcValue::hasFailedOrCanceledSubresources):
+        (WebCore):
+        * css/CSSFontFaceSrcValue.h:
+        (CSSFontFaceSrcValue):
+        * css/CSSImageSetValue.cpp:
+        (WebCore::CSSImageSetValue::hasFailedOrCanceledSubresources):
+        (WebCore):
+        * css/CSSImageSetValue.h:
+        (CSSImageSetValue):
+        * css/CSSImageValue.cpp:
+        (WebCore::CSSImageValue::hasFailedOrCanceledSubresources):
+        (WebCore):
+        * css/CSSImageValue.h:
+        (CSSImageValue):
+        * css/CSSValue.cpp:
+        (WebCore::CSSValue::hasFailedOrCanceledSubresources):
+        (WebCore):
+        * css/CSSValue.h:
+        (CSSValue):
+        * css/CSSValueList.cpp:
+        (WebCore::CSSValueList::hasFailedOrCanceledSubresources):
+        (WebCore):
+        * css/CSSValueList.h:
+        (CSSValueList):
+        * css/StylePropertySet.cpp:
+        (WebCore::StylePropertySet::hasFailedOrCanceledSubresources):
+        (WebCore):
+        * css/StylePropertySet.h:
+        (StylePropertySet):
+        * css/StyleSheetContents.cpp:
+        (WebCore::childRulesHaveFailedOrCanceledSubresources):
+        (WebCore):
+        (WebCore::StyleSheetContents::hasFailedOrCanceledSubresources):
+        * css/StyleSheetContents.h:
+        (StyleSheetContents):
+        * loader/cache/CachedCSSStyleSheet.cpp:
+        (WebCore::CachedCSSStyleSheet::restoreParsedStyleSheet):
+        * loader/cache/CachedResource.h:
+        (WebCore::CachedResource::loadFailedOrCanceled):
+
 2012-08-05  Kentaro Hara  <haraken@chromium.org>
 
         [V8] Move V8Proxy methods that set DOM attributes/callbacks to V8Binding

diff --git a/Source/WebCore/css/CSSCrossfadeValue.cpp b/Source/WebCore/css/CSSCrossfadeValue.cpp
index 4ff08af..a95d1e7 100644 (file)
--- a/Source/WebCore/css/CSSCrossfadeValue.cpp
+++ b/Source/WebCore/css/CSSCrossfadeValue.cpp
@@ -172,4 +172,13 @@ void CSSCrossfadeValue::CrossfadeSubimageObserverProxy::imageChanged(CachedImage
         m_ownerValue->crossfadeChanged(*rect);
 }
 
+bool CSSCrossfadeValue::hasFailedOrCanceledSubresources() const
+{
+    if (m_cachedFromImage && m_cachedFromImage->loadFailedOrCanceled())
+        return true;
+    if (m_cachedToImage && m_cachedToImage->loadFailedOrCanceled())
+        return true;
+    return false;
+}
+
 } // namespace WebCore

diff --git a/Source/WebCore/css/CSSCrossfadeValue.h b/Source/WebCore/css/CSSCrossfadeValue.h
index fb4c2e0..89a72c4 100644 (file)
--- a/Source/WebCore/css/CSSCrossfadeValue.h
+++ b/Source/WebCore/css/CSSCrossfadeValue.h
@@ -61,6 +61,8 @@ public:
 
     void setPercentage(PassRefPtr<CSSPrimitiveValue> percentageValue) { m_percentageValue = percentageValue; }
 
+    bool hasFailedOrCanceledSubresources() const;
+
 private:
     CSSCrossfadeValue(PassRefPtr<CSSValue> fromValue, PassRefPtr<CSSValue> toValue)
         : CSSImageGeneratorValue(CrossfadeClass)

diff --git a/Source/WebCore/css/CSSFontFaceSrcValue.cpp b/Source/WebCore/css/CSSFontFaceSrcValue.cpp
index 79ae0e7..605b2fc 100644 (file)
--- a/Source/WebCore/css/CSSFontFaceSrcValue.cpp
+++ b/Source/WebCore/css/CSSFontFaceSrcValue.cpp
@@ -79,6 +79,13 @@ void CSSFontFaceSrcValue::addSubresourceStyleURLs(ListHashSet<KURL>& urls, const
         addSubresourceURL(urls, styleSheet->completeURL(m_resource));
 }
 
+bool CSSFontFaceSrcValue::hasFailedOrCanceledSubresources() const
+{
+    if (!m_cachedFont)
+        return false;
+    return m_cachedFont->loadFailedOrCanceled();
+}
+
 CachedFont* CSSFontFaceSrcValue::cachedFont(Document* document)
 {
     if (!m_cachedFont) {

diff --git a/Source/WebCore/css/CSSFontFaceSrcValue.h b/Source/WebCore/css/CSSFontFaceSrcValue.h
index 94607fe..06ef4b3 100644 (file)
--- a/Source/WebCore/css/CSSFontFaceSrcValue.h
+++ b/Source/WebCore/css/CSSFontFaceSrcValue.h
@@ -67,6 +67,8 @@ public:
 
     void addSubresourceStyleURLs(ListHashSet<KURL>&, const StyleSheetContents*) const;
 
+    bool hasFailedOrCanceledSubresources() const;
+
     CachedFont* cachedFont(Document*);
 
 private:

diff --git a/Source/WebCore/css/CSSImageSetValue.cpp b/Source/WebCore/css/CSSImageSetValue.cpp
index 0f48d01..89a6462 100644 (file)
--- a/Source/WebCore/css/CSSImageSetValue.cpp
+++ b/Source/WebCore/css/CSSImageSetValue.cpp
@@ -139,6 +139,16 @@ String CSSImageSetValue::customCssText() const
     return "-webkit-image-set(" + CSSValueList::customCssText() + ")";
 }
 
+bool CSSImageSetValue::hasFailedOrCanceledSubresources() const
+{
+    if (!m_imageSet || !m_imageSet->isCachedImageSet())
+        return false;
+    CachedResource* cachedResource = static_cast<StyleCachedImageSet*>(m_imageSet.get())->cachedImage();
+    if (!cachedResource)
+        return true;
+    return cachedResource->loadFailedOrCanceled();
+}
+
 CSSImageSetValue::CSSImageSetValue(const CSSImageSetValue& cloneFrom)
     : CSSValueList(cloneFrom)
     , m_accessedBestFitImage(false)

diff --git a/Source/WebCore/css/CSSImageSetValue.h b/Source/WebCore/css/CSSImageSetValue.h
index 404d3f9..95270a6 100644 (file)
--- a/Source/WebCore/css/CSSImageSetValue.h
+++ b/Source/WebCore/css/CSSImageSetValue.h
@@ -60,6 +60,8 @@ public:
         float scaleFactor;
     };
 
+    bool hasFailedOrCanceledSubresources() const;
+
     PassRefPtr<CSSImageSetValue> cloneForCSSOM() const;
 
 protected:

diff --git a/Source/WebCore/css/CSSImageValue.cpp b/Source/WebCore/css/CSSImageValue.cpp
index 6196463..07bd4b3 100644 (file)
--- a/Source/WebCore/css/CSSImageValue.cpp
+++ b/Source/WebCore/css/CSSImageValue.cpp
@@ -102,6 +102,16 @@ void CSSImageValue::clearCachedImage()
     m_accessedImage = false;
 }
 
+bool CSSImageValue::hasFailedOrCanceledSubresources() const
+{
+    if (!m_image || !m_image->isCachedImage())
+        return false;
+    CachedResource* cachedResource = static_cast<StyleCachedImage*>(m_image.get())->cachedImage();
+    if (!cachedResource)
+        return true;
+    return cachedResource->loadFailedOrCanceled();
+}
+
 String CSSImageValue::customCssText() const
 {
     return "url(" + quoteCSSURLIfNeeded(m_url) + ")";

diff --git a/Source/WebCore/css/CSSImageValue.h b/Source/WebCore/css/CSSImageValue.h
index 5b2ce47..75063d2 100644 (file)
--- a/Source/WebCore/css/CSSImageValue.h
+++ b/Source/WebCore/css/CSSImageValue.h
@@ -46,6 +46,8 @@ public:
 
     PassRefPtr<CSSValue> cloneForCSSOM() const;
 
+    bool hasFailedOrCanceledSubresources() const;
+
 protected:
     CSSImageValue(ClassType, const String& url);
 

diff --git a/Source/WebCore/css/CSSValue.cpp b/Source/WebCore/css/CSSValue.cpp
index cda43b9..5ffb156 100644 (file)
--- a/Source/WebCore/css/CSSValue.cpp
+++ b/Source/WebCore/css/CSSValue.cpp
@@ -121,6 +121,26 @@ void CSSValue::addSubresourceStyleURLs(ListHashSet<KURL>& urls, const StyleSheet
         static_cast<const CSSReflectValue*>(this)->addSubresourceStyleURLs(urls, styleSheet);
 }
 
+bool CSSValue::hasFailedOrCanceledSubresources() const
+{
+    // This should get called for internal instances only.
+    ASSERT(!isCSSOMSafe());
+
+    if (isValueList())
+        return static_cast<const CSSValueList*>(this)->hasFailedOrCanceledSubresources();
+    if (classType() == FontFaceSrcClass)
+        return static_cast<const CSSFontFaceSrcValue*>(this)->hasFailedOrCanceledSubresources();
+    if (classType() == ImageClass)
+        return static_cast<const CSSImageValue*>(this)->hasFailedOrCanceledSubresources();
+    if (classType() == CrossfadeClass)
+        return static_cast<const CSSCrossfadeValue*>(this)->hasFailedOrCanceledSubresources();
+#if ENABLE(CSS_IMAGE_SET)
+    if (classType() == ImageSetClass)
+        return static_cast<const CSSImageSetValue*>(this)->hasFailedOrCanceledSubresources();
+#endif
+    return false;
+}
+
 String CSSValue::cssText() const
 {
     if (m_isTextClone) {

diff --git a/Source/WebCore/css/CSSValue.h b/Source/WebCore/css/CSSValue.h
index 93a6d4a..83c2e8e 100644 (file)
--- a/Source/WebCore/css/CSSValue.h
+++ b/Source/WebCore/css/CSSValue.h
@@ -117,6 +117,8 @@ public:
 
     void addSubresourceStyleURLs(ListHashSet<KURL>&, const StyleSheetContents*) const;
 
+    bool hasFailedOrCanceledSubresources() const;
+
 protected:
 
     static const size_t ClassTypeBits = 5;

diff --git a/Source/WebCore/css/CSSValueList.cpp b/Source/WebCore/css/CSSValueList.cpp
index 06f093e..b8d36a4 100644 (file)
--- a/Source/WebCore/css/CSSValueList.cpp
+++ b/Source/WebCore/css/CSSValueList.cpp
@@ -173,6 +173,15 @@ void CSSValueList::addSubresourceStyleURLs(ListHashSet<KURL>& urls, const StyleS
         m_values[i]->addSubresourceStyleURLs(urls, styleSheet);
 }
 
+bool CSSValueList::hasFailedOrCanceledSubresources() const
+{
+    for (unsigned i = 0; i < m_values.size(); ++i) {
+        if (m_values[i]->hasFailedOrCanceledSubresources())
+            return true;
+    }
+    return false;
+}
+
 CSSValueList::CSSValueList(const CSSValueList& cloneFrom)
     : CSSValue(cloneFrom.classType(), /* isCSSOMSafe */ true)
 {

diff --git a/Source/WebCore/css/CSSValueList.h b/Source/WebCore/css/CSSValueList.h
index 94643da..40bb437 100644 (file)
--- a/Source/WebCore/css/CSSValueList.h
+++ b/Source/WebCore/css/CSSValueList.h
@@ -64,6 +64,8 @@ public:
 #endif
 
     void addSubresourceStyleURLs(ListHashSet<KURL>&, const StyleSheetContents*) const;
+
+    bool hasFailedOrCanceledSubresources() const;
     
     PassRefPtr<CSSValueList> cloneForCSSOM() const;
 

diff --git a/Source/WebCore/css/StylePropertySet.cpp b/Source/WebCore/css/StylePropertySet.cpp
index 79ab043..1e78aab 100644 (file)
--- a/Source/WebCore/css/StylePropertySet.cpp
+++ b/Source/WebCore/css/StylePropertySet.cpp
@@ -894,6 +894,16 @@ void StylePropertySet::addSubresourceStyleURLs(ListHashSet<KURL>& urls, StyleShe
         propertyAt(i).value()->addSubresourceStyleURLs(urls, contextStyleSheet);
 }
 
+bool StylePropertySet::hasFailedOrCanceledSubresources() const
+{
+    unsigned size = propertyCount();
+    for (unsigned i = 0; i < size; ++i) {
+        if (propertyAt(i).value()->hasFailedOrCanceledSubresources())
+            return true;
+    }
+    return false;
+}
+
 // This is the list of properties we want to copy in the copyBlockProperties() function.
 // It is the list of CSS properties that apply specially to block-level elements.
 static const CSSPropertyID blockProperties[] = {

diff --git a/Source/WebCore/css/StylePropertySet.h b/Source/WebCore/css/StylePropertySet.h
index a6d5819..945dde6 100644 (file)
--- a/Source/WebCore/css/StylePropertySet.h
+++ b/Source/WebCore/css/StylePropertySet.h
@@ -110,6 +110,8 @@ public:
 
     bool isMutable() const { return m_isMutable; }
 
+    bool hasFailedOrCanceledSubresources() const;
+
     static unsigned averageSizeInBytes();
     void reportMemoryUsage(MemoryObjectInfo*) const;
 

diff --git a/Source/WebCore/css/StyleSheetContents.cpp b/Source/WebCore/css/StyleSheetContents.cpp
index c2244c6..0c9dfbd 100644 (file)
--- a/Source/WebCore/css/StyleSheetContents.cpp
+++ b/Source/WebCore/css/StyleSheetContents.cpp
@@ -410,6 +410,46 @@ void StyleSheetContents::addSubresourceStyleURLs(ListHashSet<KURL>& urls)
     }
 }
 
+static bool childRulesHaveFailedOrCanceledSubresources(const Vector<RefPtr<StyleRuleBase> >& rules)
+{
+    for (unsigned i = 0; i < rules.size(); ++i) {
+        const StyleRuleBase* rule = rules[i].get();
+        switch (rule->type()) {
+        case StyleRuleBase::Style:
+            if (static_cast<const StyleRule*>(rule)->properties()->hasFailedOrCanceledSubresources())
+                return true;
+            break;
+        case StyleRuleBase::FontFace:
+            if (static_cast<const StyleRuleFontFace*>(rule)->properties()->hasFailedOrCanceledSubresources())
+                return true;
+            break;
+        case StyleRuleBase::Media:
+            if (childRulesHaveFailedOrCanceledSubresources(static_cast<const StyleRuleMedia*>(rule)->childRules()))
+                return true;
+            break;
+        case StyleRuleBase::Region:
+            if (childRulesHaveFailedOrCanceledSubresources(static_cast<const StyleRuleRegion*>(rule)->childRules()))
+                return true;
+            break;
+        case StyleRuleBase::Import:
+            ASSERT_NOT_REACHED();
+        case StyleRuleBase::Page:
+        case StyleRuleBase::Keyframes:
+        case StyleRuleBase::Unknown:
+        case StyleRuleBase::Charset:
+        case StyleRuleBase::Keyframe:
+            break;
+        }
+    }
+    return false;
+}
+
+bool StyleSheetContents::hasFailedOrCanceledSubresources() const
+{
+    ASSERT(isCacheable());
+    return childRulesHaveFailedOrCanceledSubresources(m_childRules);
+}
+
 StyleSheetContents* StyleSheetContents::parentStyleSheet() const
 {
     return m_ownerRule ? m_ownerRule->parentStyleSheet() : 0;

diff --git a/Source/WebCore/css/StyleSheetContents.h b/Source/WebCore/css/StyleSheetContents.h
index e2cce2f..b1ddb8c 100644 (file)
--- a/Source/WebCore/css/StyleSheetContents.h
+++ b/Source/WebCore/css/StyleSheetContents.h
@@ -79,6 +79,7 @@ public:
     const String& charset() const { return m_parserContext.charset; }
 
     bool loadCompleted() const { return m_loadCompleted; }
+    bool hasFailedOrCanceledSubresources() const;
 
     KURL completeURL(const String& url) const;
     void addSubresourceStyleURLs(ListHashSet<KURL>&);

diff --git a/Source/WebCore/loader/cache/CachedCSSStyleSheet.cpp b/Source/WebCore/loader/cache/CachedCSSStyleSheet.cpp
index e1fb02c..c05d730 100644 (file)
--- a/Source/WebCore/loader/cache/CachedCSSStyleSheet.cpp
+++ b/Source/WebCore/loader/cache/CachedCSSStyleSheet.cpp
@@ -168,6 +168,12 @@ PassRefPtr<StyleSheetContents> CachedCSSStyleSheet::restoreParsedStyleSheet(cons
 {
     if (!m_parsedStyleSheetCache)
         return 0;
+    if (m_parsedStyleSheetCache->hasFailedOrCanceledSubresources()) {
+        m_parsedStyleSheetCache->removedFromMemoryCache();
+        m_parsedStyleSheetCache.clear();
+        return 0;
+    }
+
     ASSERT(m_parsedStyleSheetCache->isCacheable());
     ASSERT(m_parsedStyleSheetCache->isInMemoryCache());
 

diff --git a/Source/WebCore/loader/cache/CachedResource.h b/Source/WebCore/loader/cache/CachedResource.h
index fe3e1fc..ea63f93 100644 (file)
--- a/Source/WebCore/loader/cache/CachedResource.h
+++ b/Source/WebCore/loader/cache/CachedResource.h
@@ -205,6 +205,7 @@ public:
 
     bool wasCanceled() const { return m_status == Canceled; }
     bool errorOccurred() const { return (m_status == LoadError || m_status == DecodeError); }
+    bool loadFailedOrCanceled() { return m_status == Canceled || m_status == LoadError; }
 
     bool sendResourceLoadCallbacks() const { return m_options.sendLoadCallbacks == SendCallbacks; }