REGRESSION (r141136): Wiki "Random article" function very broken

• Component: Page Loading
• Status: RESOLVED FIXED

[Brady Eidson]

REGRESSION (r141136): Wiki "Random article" function very broken

When main resources are in the memory cache, they are keyed off http://en.wikipedia.org/wiki/Special:Random
Even though http://en.wikipedia.org/wiki/Special:Random redirects to a random article each time.

So we start to revalidate the resource with wikipedia, and it redirects the revalidation request.

In our new redirect request, we still issue a conditional GET to try to revalidate the new request, and that gives us back a 304.  So we update our URLs but use the main resource text from the previous article.

If the redirect URL differs from the CachedResource we're trying to revalidate, we should stop trying to revalidate and issue an unconditional GET.

In radar as <rdar://problem/13229985>

[Brady Eidson]

Created attachment 197910 [details]
Patch v1

Still making sure I didn't break any other layout tests, but this is my plan.

[Alexey Proskuryakov]

Comment on attachment 197910 [details]
Patch v1

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

Does SubresourceLoader cover all the resources that may be affected?

> Source/WebCore/loader/SubresourceLoader.cpp:137
> +        // If this redirect takes us somewhere besides the final response URL of the resource we're revalidating,
> +        // then make this an unconditional GET request.

I think that this "what" comment adds some value, but a "why" one would be even better.

[Brady Eidson]

(In reply to comment #2)
> (From update of attachment 197910 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=197910&action=review
> 
> Does SubresourceLoader cover all the resources that may be affected?

SubresourceLoader is the only thing that revalidates CachedResources, so it's an appropriate catch-all for accidentally reusing previously loaded resources.

> > Source/WebCore/loader/SubresourceLoader.cpp:137
> > +        // If this redirect takes us somewhere besides the final response URL of the resource we're revalidating,
> > +        // then make this an unconditional GET request.
> 
> I think that this "what" comment adds some value, but a "why" one would be even better.

I can definitely improve this.

[Brady Eidson]

EWS never churned all the way through this, and running tests locally is proving to be a hit-or-miss PITA.

I need to move forward with this one and watch the actual testers afterwards.

[Brady Eidson]

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

[Brady Eidson]

(In reply to comment #3)
> (In reply to comment #2)
> > > Source/WebCore/loader/SubresourceLoader.cpp:137
> > > +        // If this redirect takes us somewhere besides the final response URL of the resource we're revalidating,
> > > +        // then make this an unconditional GET request.
> > 
> > I think that this "what" comment adds some value, but a "why" one would be even better.
> 
> I can definitely improve this.

Since I'd worked on other things for ~9 days, I forgot to address this comment.  My apologies.  Will fix in a followup

[Brady Eidson]

Followed up in http://trac.webkit.org/changeset/148932