Bug 69877 – Make CachedResourceClientWalker templated and have it do checked casts.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

Followup to https://bugs.webkit.org/show_bug.cgi?id=69790, which introduced a bunch of unchecked casts that rely on CachedResource subtypes and CachedResourceClient subtypes matching each other correctly.  As written, it will fail dangerously in the event of a programmer error.

[Nate Chapin]

Created attachment 110597 [details]
patch

[WebKit Review Bot]

Comment on attachment 110597 [details]
patch

Clearing flags on attachment: 110597

Committed r97199: <http://trac.webkit.org/changeset/97199>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Antti Koivisto]

Comment on attachment 110597 [details]
patch

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

> Source/WebCore/loader/cache/CachedResourceClientWalker.h:54
> +            if (m_clientSet.contains(next) && (T::expectedType() == CachedResourceClient::expectedType() || next->type() == T::expectedType()))
> +                return static_cast<T*>(next);

What is the purpose of T::expectedType() == CachedResourceClient::expectedType()?

Could this assert the correct type instead?

[Nate Chapin]

(In reply to comment #4)
> (From update of attachment 110597 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110597&action=review
> 
> > Source/WebCore/loader/cache/CachedResourceClientWalker.h:54
> > +            if (m_clientSet.contains(next) && (T::expectedType() == CachedResourceClient::expectedType() || next->type() == T::expectedType()))
> > +                return static_cast<T*>(next);
> 
> What is the purpose of T::expectedType() == CachedResourceClient::expectedType()?
> 
> Could this assert the correct type instead?

The purpose is to handle the notifyFinished() callback correctly.  notifyFinished() is called for all CachedResourceClient types, and is templated with the base class rather than a subclass.

I suppose we could assert instead of just skipping clients of the wrong type.  I don't have a strong opinion.

[Antti Koivisto]

I suspect that all cases where there are unexpected types on the list are bugs so assert would be the right choice.