Bug 162959 – [Fetch API] Memory cache should not bypass redirect mode

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

If a resource is fetched from the memory cache, we currently do not check the redirect mode.

[youenn fablet]

Created attachment 290697 [details]
Patch

[Darin Adler]

Comment on attachment 290697 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:554
> +static inline bool canUpdateFromResource(const CachedResource& resource, const CachedResourceRequest& request)

I don’t fully understand the name of this function. The name "can update from resource" explains mechanically what the caller should do, but doesn’t make it clear the kind of decision this function is making. Maybe this is about whether we can re-use an existing cached resource for a new request? And if we return "false" it means we should ignore the existing cached resource because it’s not suitable for this request? It would be good to give this a clearer name.

Think about how you would say this when talking to another person. I don’t think you would say, “This function answers the question: Can you update from this resource for this request?” What does “update from this resource” mean?

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:556
>      // FIXME: For being loaded requests, we currently do not use the same resource, as this may induce errors in the resource response tainting.

I don’t understand what "errors in the resource response tainting" means.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:561
> +    // If the cached resource has not followed redirections, we are best reloading the resource.

I don’t understand the phrase "we are best reloading the resource".

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:565
> +    // We could support redirect mode other than Follow in case of a redirected resource, but it is simpler to just reload.

Should be "redirect modes" not "redirect mode".

Saying it is "simplest" doesn’t give the full rationale for why this is OK. The real reason must be that these kinds of loads are unusual and it’s OK to not optimize these cases, not worth the complexity? I don’t know what the reason is, so I can’t tell you what to write.

[youenn fablet]

Created attachment 291093 [details]
Patch for landing

[youenn fablet]

Thanks for the review.

(In reply to comment #2)
> Comment on attachment 290697 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290697&action=review
> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:554
> > +static inline bool canUpdateFromResource(const CachedResource& resource, const CachedResourceRequest& request)
> 
> I don’t fully understand the name of this function. The name "can update
> from resource" explains mechanically what the caller should do, but doesn’t
> make it clear the kind of decision this function is making. Maybe this is
> about whether we can re-use an existing cached resource for a new request?
> And if we return "false" it means we should ignore the existing cached
> resource because it’s not suitable for this request? It would be good to
> give this a clearer name.
> 
> Think about how you would say this when talking to another person. I don’t
> think you would say, “This function answers the question: Can you update
> from this resource for this request?” What does “update from this resource”
> mean?

Renamed to: isResourceSuitableForDirectReuse, although the name is not perfect.

I agree its role is not very clear, especially with CachedResourceLoader:: determineRevalidationPolicy.
determineRevalidationPolicy is mostly (but not only...) targeted at checking whether the request can be reused as per HTTP cache policy

When a resource can be reused, in most cases, we can return the cached resource as is.
Sometimes, we need to recompute some internal fields, based on new request being slightly different from the cached resource own request.
canUpdateFromResource is about telling whether we can recompute those internal fields directly or whether we should reload.
We may want to not do recomputation when we do not have enough data or when this is too complex/too rare.

The difference between the two methods is not that well-defined though.
determineRevalidationPolicy asks to reload in case cookies are different or in case resource type is different for instance.
In both cases, we could try to be smarter and reuse them and in case it does not work, reload.

I am not satisfied with that model though.
Simplifying determineRevalidationPolicy would be great but it is difficult to evaluate the perf penalties or corner-cases we may miss.

> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:556
> >      // FIXME: For being loaded requests, we currently do not use the same resource, as this may induce errors in the resource response tainting.
> 
> I don’t understand what "errors in the resource response tainting" means.

Clarified to: 
// For being loaded requests, the response tainting may not be correctly computed if the fetch mode is not the same.
// Even if the fetch mode is the same, we are not sure that the resource can be reused (Vary: Origin header for instance).


> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:561
> > +    // If the cached resource has not followed redirections, we are best reloading the resource.
> 
> I don’t understand the phrase "we are best reloading the resource".

Clarified to:
// If the cached resource has not followed redirections, it is incomplete and we should not use it.

> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:565
> > +    // We could support redirect mode other than Follow in case of a redirected resource, but it is simpler to just reload.
> 
> Should be "redirect modes" not "redirect mode".

OK

> Saying it is "simplest" doesn’t give the full rationale for why this is OK.
> The real reason must be that these kinds of loads are unusual and it’s OK to
> not optimize these cases, not worth the complexity? I don’t know what the
> reason is, so I can’t tell you what to write.

Right, clarified to:
// We could support redirect modes other than Follow in case of a redirected resource.
// This case is rare and is not worth optimizing currently.

[WebKit Commit Bot]

Comment on attachment 291093 [details]
Patch for landing

Clearing flags on attachment: 291093

Committed r206994: <http://trac.webkit.org/changeset/206994>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.