Bug 93203

Summary: Don't reuse cached stylesheet with failed or canceled resource loads
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, gustavo, japhet, jonlee, kling, macpherson, menard, thorton, webkit.review.bot, xan.lopez
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
webkit-ews: commit-queue-
better patch
webkit-ews: commit-queue-
update patch simon.fraser: review+

Description Antti Koivisto 2012-08-05 11:14:17 PDT
1) Go to apple.com
2) Reload and repeatedly

Eventually you can get into state where some images don't load.
Comment 1 Antti Koivisto 2012-08-05 11:20:46 PDT
<rdar://problem/11559765>
Comment 2 Antti Koivisto 2012-08-05 11:24:32 PDT
Created attachment 156566 [details]
patch
Comment 3 Early Warning System Bot 2012-08-05 11:41:37 PDT
Comment on attachment 156566 [details]
patch

Attachment 156566 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13436501
Comment 4 Tim Horton 2012-08-05 11:43:08 PDT
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)
Comment 5 Antti Koivisto 2012-08-05 11:46:47 PDT
Created attachment 156567 [details]
better patch
Comment 6 Antti Koivisto 2012-08-05 11:50:02 PDT
(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.
Comment 7 Tim Horton 2012-08-05 11:51:41 PDT
(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.
Comment 8 Early Warning System Bot 2012-08-05 12:09:28 PDT
Comment on attachment 156567 [details]
better patch

Attachment 156567 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13439404
Comment 9 Gustavo Noronha (kov) 2012-08-05 12:10:42 PDT
Comment on attachment 156567 [details]
better patch

Attachment 156567 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13442259
Comment 10 Early Warning System Bot 2012-08-05 12:13:40 PDT
Comment on attachment 156567 [details]
better patch

Attachment 156567 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13443180
Comment 11 Build Bot 2012-08-05 13:25:20 PDT
Comment on attachment 156567 [details]
better patch

Attachment 156567 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13447051
Comment 12 Antti Koivisto 2012-08-05 13:30:28 PDT
Created attachment 156568 [details]
update patch
Comment 13 Simon Fraser (smfr) 2012-08-05 13:45:52 PDT
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.
Comment 14 Antti Koivisto 2012-08-05 14:04:49 PDT
http://trac.webkit.org/changeset/124720