Bug 70972 – [chromium] CachedResourceRequest clobbers the TargetType of ThreadableLoaderClients

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nate Chapin]

Originally reported at: http://code.google.com/p/chromium/issues/detail?id=101651

In http://trac.webkit.org/changeset/98380, ThreadableLoaderClients started being loaded through the CachedResource codepath. ThreadableLoaderClients have traditionally had to set their own ResourceRequest::TargetType, but CachedResourceRequest sets it for CachedResource loads. CachedResourceRequest currently clobbers TargetType without checking whether it had been previously set.  That has interesting (and bad) side effects in chromium-specific code.

[Nate Chapin]

Created attachment 112606 [details]
patch

No cq? because I want to ensure this passes the chromium trybots before landing.

[Adam Barth]

Comment on attachment 112606 [details]
patch

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

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:104
> -    resourceRequest.setTargetType(cachedResourceTypeToTargetType(resource->type()));
> +    if (resourceRequest.targetType() == ResourceRequest::TargetIsSubresource)
> +        resourceRequest.setTargetType(cachedResourceTypeToTargetType(resource->type()));

It's too bad there isn't an uninitialized value to compare against.  It's strange that TargetIsSubresource will magically behave differently.

[Adam Barth]

@jochen: Any thoughts on this patch?

[Darin Fisher (:fishd, Google)]

Comment on attachment 112606 [details]
patch

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

>> Source/WebCore/loader/cache/CachedResourceRequest.cpp:104
>> +        resourceRequest.setTargetType(cachedResourceTypeToTargetType(resource->type()));
> 
> It's too bad there isn't an uninitialized value to compare against.  It's strange that TargetIsSubresource will magically behave differently.

Agreed.  It seems like TargetIsUnspecified would be a reasonable value to define.

[Nate Chapin]

(In reply to comment #4)
> (From update of attachment 112606 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112606&action=review
> 
> >> Source/WebCore/loader/cache/CachedResourceRequest.cpp:104
> >> +        resourceRequest.setTargetType(cachedResourceTypeToTargetType(resource->type()));
> > 
> > It's too bad there isn't an uninitialized value to compare against.  It's strange that TargetIsSubresource will magically behave differently.
> 
> Agreed.  It seems like TargetIsUnspecified would be a reasonable value to define.

My main objection is that this proposal will turn this patch into a two-sided one.  TargetIsSubresource is the default, and changing the default to a not-yet-defined causes a bunch of assertion failures in browser_tests and interactive_ui_tests.

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 112606 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=112606&action=review
> > 
> > >> Source/WebCore/loader/cache/CachedResourceRequest.cpp:104
> > >> +        resourceRequest.setTargetType(cachedResourceTypeToTargetType(resource->type()));
> > > 
> > > It's too bad there isn't an uninitialized value to compare against.  It's strange that TargetIsSubresource will magically behave differently.
> > 
> > Agreed.  It seems like TargetIsUnspecified would be a reasonable value to define.
> 
> My main objection is that this proposal will turn this patch into a two-sided one.  TargetIsSubresource is the default, and changing the default to a not-yet-defined causes a bunch of assertion failures in browser_tests and interactive_ui_tests.

I don't think this needs to be two-sided. We can map TargetIsUnspecified to TargetIsSubresource when converting the ResourceRequest::TargetType to a WebURLRequest::TargetType

I think overall, the patch looks reasonable.

It's a pity that we have TargetType and CachedResourceType with slightly different definitions. I wonder whether it was possible to have all resource requests go through the cache, and replace TargetType by CachedResourceType entirely?

[Adam Barth]

The goal is to have almost all requests go through the cached resource pipeline.  Some (e.g., NetscapePluginStream) won't though.

[Nate Chapin]

Created attachment 112914 [details]
Add TargetIsUnspecified

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

Comment on attachment 112914 [details]
Add TargetIsUnspecified

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

OK

> Source/WebKit/chromium/public/WebURLRequest.h:75
> +        TargetIsCue = 15,

It took me a while to figure out what "Cue" refers too.  It seems like it might be
a name that makes sense in some other world?  Video something or other?

> Source/WebCore/ChangeLog:4
> +        ReosurceRequest::TargetType clobbered. They set their own

nit: "ReosurceRequest"

[Nate Chapin]

> It took me a while to figure out what "Cue" refers too.  It seems like it might be
> a name that makes sense in some other world?  Video something or other?

I believe it's related to http://trac.webkit.org/browser/trunk/Source/WebCore/loader/cache/CachedTextTrack.h.  I thought these names had been standardized, but I think the ones in CachedResourceRequest and ResourceRequest may have been missed?

[Darin Fisher (:fishd, Google)]

(In reply to comment #11)
> > It took me a while to figure out what "Cue" refers too.  It seems like it might be
> > a name that makes sense in some other world?  Video something or other?
> 
> I believe it's related to http://trac.webkit.org/browser/trunk/Source/WebCore/loader/cache/CachedTextTrack.h.  I thought these names had been standardized, but I think the ones in CachedResourceRequest and ResourceRequest may have been missed?

So, perhaps "Cue" should be renamed "TextTrack"?

[Nate Chapin]

Created attachment 113238 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 113238 [details]
Patch for landing

Clearing flags on attachment: 113238

Committed r99015: <http://trac.webkit.org/changeset/99015>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.