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

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Antti Koivisto]

1) Go to apple.com
2) Reload and repeatedly

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

[Antti Koivisto]

<rdar://problem/11559765>

[Antti Koivisto]

Created attachment 156566 [details]
patch

[Early Warning System Bot]

Comment on attachment 156566 [details]
patch

Attachment 156566 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13436501

[Tim Horton]

Comment on attachment 156566 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156566&action=review

> Source/WebCore/css/CSSImageValue.cpp:105
> +bool CSSImageValue::hasFailedOrCanceledSubresources() const

Do generated images need their own override too? (I'm thinking specifically for crossfade)

[Antti Koivisto]

Created attachment 156567 [details]
better patch

[Antti Koivisto]

(In reply to comment #4)
> Do generated images need their own override too? (I'm thinking specifically for crossfade)

I don't see code there that references a CachedResource. Or maybe I'm not looking the right place?

But it is definitely good to check this for completeness.

[Tim Horton]

(In reply to comment #6)
> (In reply to comment #4)
> > Do generated images need their own override too? (I'm thinking specifically for crossfade)
> 
> I don't see code there that references a CachedResource. Or maybe I'm not looking the right place?
> 
> But it is definitely good to check this for completeness.

CSSCrossfadeValue (a CSSImageGeneratorValue, not a CSSImageValue) has two CachedResourceHandles.

[Early Warning System Bot]

Comment on attachment 156567 [details]
better patch

Attachment 156567 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13439404

[Gustavo Noronha (kov)]

Comment on attachment 156567 [details]
better patch

Attachment 156567 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13442259

[Early Warning System Bot]

Comment on attachment 156567 [details]
better patch

Attachment 156567 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13443180

[Build Bot]

Comment on attachment 156567 [details]
better patch

Attachment 156567 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13447051

[Antti Koivisto]

Created attachment 156568 [details]
update patch

[Simon Fraser (smfr)]

Comment on attachment 156568 [details]
update patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156568&action=review

> Source/WebCore/css/CSSValue.cpp:140
> +    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

This would be one line if we used virtual functions :(

> Source/WebCore/css/StylePropertySet.cpp:901
> +        if (propertyAt(i).value()->hasFailedOrCanceledSubresources())

Is this guaranteed to be non-null?

> Source/WebCore/css/StyleSheetContents.cpp:433
> +        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;

We need casting functions like we have for renderers.

[Antti Koivisto]

http://trac.webkit.org/changeset/124720