Summary: | Entity-header extension headers honored on 304 responses. | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Thomas Sepez <tsepez> | ||||||||||||||||||||
Component: | Page Loading | Assignee: | Mike West <mkwst> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | abarth, ap, japhet, jochen, koivisto, mkwst, mkwst+watchlist, webkit.review.bot | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||
Bug Blocks: | 71851 | ||||||||||||||||||||||
Attachments: |
|
Description
Thomas Sepez
2011-11-15 14:00:22 PST
Created attachment 115239 [details]
Flunking testcases
Maybe do all X-WebKit-* headers? Anything we put in that namespace is going to be an entity header. X-Frame-Options as well. Created attachment 115255 [details]
Flunking testcases
Fix cut-n-paste, extend expires to near the end of time.
Created attachment 115416 [details]
Flunking testcases
Add XFO header test as well just for completeness.
I'm not going to be able to look at this right now. I'm trying to address the Chromium side at https://codereview.chromium.org/12224008/, and Jochen's suggested a direction for the rest of WebCore. Let's see how it goes. Created attachment 186598 [details]
Patch
The attached patch works locally for the Mac port. I've skipped the tests for Chromium for the moment, as it looks like that will require a Chromium-side change that's currently up for review. Nate: Jochen suggested that you'd be a good reviewer for this change. Would you mind taking a look? Created attachment 186604 [details]
Patch
Hrm. I ran the tests through the chromium port locally, and they passed, which surprised me because I'd misunderstood the network stack's role here. So, looks like I can run the tests everywhere. Yay. :) Comment on attachment 186604 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186604&action=review > Source/WebCore/loader/cache/CachedResource.cpp:740 > DEFINE_STATIC_LOCAL(const AtomicString, contentHeaderPrefix, ("content-", AtomicString::ConstructFromLiteral)); > + DEFINE_STATIC_LOCAL(const AtomicString, xContentHeaderPrefix, ("x-content-", AtomicString::ConstructFromLiteral)); > + DEFINE_STATIC_LOCAL(const AtomicString, xFrameHeaderPrefix, ("x-frame-", AtomicString::ConstructFromLiteral)); > + DEFINE_STATIC_LOCAL(const AtomicString, xWebKitHeaderPrefix, ("x-webkit-", AtomicString::ConstructFromLiteral)); > + DEFINE_STATIC_LOCAL(const AtomicString, xXSSHeaderPrefix, ("x-xss-", AtomicString::ConstructFromLiteral)); What's the benefit of having AtomicStrings here? These constants are only used as prefixes AFAICT. (In reply to comment #12) > (From update of attachment 186604 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186604&action=review > > > Source/WebCore/loader/cache/CachedResource.cpp:740 > > DEFINE_STATIC_LOCAL(const AtomicString, contentHeaderPrefix, ("content-", AtomicString::ConstructFromLiteral)); > > + DEFINE_STATIC_LOCAL(const AtomicString, xContentHeaderPrefix, ("x-content-", AtomicString::ConstructFromLiteral)); > > + DEFINE_STATIC_LOCAL(const AtomicString, xFrameHeaderPrefix, ("x-frame-", AtomicString::ConstructFromLiteral)); > > + DEFINE_STATIC_LOCAL(const AtomicString, xWebKitHeaderPrefix, ("x-webkit-", AtomicString::ConstructFromLiteral)); > > + DEFINE_STATIC_LOCAL(const AtomicString, xXSSHeaderPrefix, ("x-xss-", AtomicString::ConstructFromLiteral)); > > What's the benefit of having AtomicStrings here? These constants are only used as prefixes AFAICT. They're only used as prefixes, correct. AtomicString is probably overkill. I'm not even sure this is going to be called often enough to worry about defining static locals. Created attachment 186674 [details]
Patch
(In reply to comment #14) > Created an attachment (id=186674) [details] > Patch This patch drops the static locals in favor of string literals. I don't think it'll end up making a performance difference; if it does, I'll happily put thought into optimizing. Comment on attachment 186674 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186674&action=review Looks good to me. > Source/WebCore/loader/cache/CachedResource.cpp:741 > // Don't allow 304 response to update content headers, these can't change but some servers send wrong values. This comment seems incomplete now. In fact, I'd be pretty surprised to see this list of prefixes in code without reading this bug first. Some things to explain would be: (1) entity header fields should not be sent with 304 responses, but when they are, we ignore them, and (2) per RFC 2612 paragraph 7.1, all extension header fields are treated as entity header fields, and (3) why we have specific prefixes instead of just "x-". Possibly fourth thing to explain, or maybe a flaw with this patch: why are other entity headers not here, namely Expires and Last-Modified? (In reply to comment #16) > (From update of attachment 186674 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186674&action=review > > Looks good to me. > > > Source/WebCore/loader/cache/CachedResource.cpp:741 > > // Don't allow 304 response to update content headers, these can't change but some servers send wrong values. > > This comment seems incomplete now. > > In fact, I'd be pretty surprised to see this list of prefixes in code without reading this bug first. > > Some things to explain would be: (1) entity header fields should not be sent with 304 responses, but when they are, we ignore them, and (2) per RFC 2612 paragraph 7.1, all extension header fields are treated as entity header fields, and (3) why we have specific prefixes instead of just "x-". > > Possibly fourth thing to explain, or maybe a flaw with this patch: why are other entity headers not here, namely Expires and Last-Modified? You're right. I originally wanted to keep this change as small and simple as possible, but it would be better to be complete. I'll change this method to match Chromium's network stack which filters more completely. Created attachment 186782 [details]
Patch
Created attachment 186824 [details]
Patch for landing?
Comment on attachment 186824 [details] Patch for landing? (In reply to comment #19) > Created an attachment (id=186824) [details] > Patch for landing? I've addressed your feedback, and I'm carrying over your r+, ap. I'd appreciate, however, it if you took another look at the patch since it's changed quite a bit. If you like it, cq+ it, please. If not, let me know and I'll take another pass. Comment on attachment 186824 [details] Patch for landing? View in context: https://bugs.webkit.org/attachment.cgi?id=186824&action=review > Source/WebCore/ChangeLog:30 > + In an anonymous namespace, this patch adds two arrays containing the We don't use anonymous namespaces, because they make debugging harder (can't easily reference symbols in debugger command line), and also as a coding style matter for consistency with existing code. Constant global variables already have the right linkage in C++. > Source/WebCore/loader/cache/CachedResource.cpp:65 > +const char* const headersToIgnoreAfterRevalidation[] = { This is actual meaningful code for this file, I'd put it below WTF forward declaration. > Source/WebCore/loader/cache/CachedResource.cpp:108 > template<> struct SequenceMemoryInstrumentationTraits<WebCore::CachedResourceClient*> { I wonder why we have to forward declare something from WTF. This does not look safe. > Source/WebCore/loader/cache/CachedResource.cpp:790 > + // such headers to update the original request. To match Chromium's > + // network stack, we'll base this on the list defined by RFC2616 7.1, I'd phrase this a little differently. It's not a goal to match Chromium's network stack. We do that because we think that its behavior makes sense, not because it's critical to have the same list here and there. Created attachment 186892 [details]
Patch
(In reply to comment #21) > (From update of attachment 186824 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186824&action=review > > > Source/WebCore/ChangeLog:30 > > + In an anonymous namespace, this patch adds two arrays containing the > > We don't use anonymous namespaces, because they make debugging harder (can't easily reference symbols in debugger command line), and also as a coding style matter for consistency with existing code. > > Constant global variables already have the right linkage in C++. Makes sense, thanks. > > Source/WebCore/loader/cache/CachedResource.cpp:790 > > + // such headers to update the original request. To match Chromium's > > + // network stack, we'll base this on the list defined by RFC2616 7.1, > > I'd phrase this a little differently. It's not a goal to match Chromium's network stack. We do that because we think that its behavior makes sense, not because it's critical to have the same list here and there. I've adjusted this comment to drop the reference to Chromium. The ChangeLog still refers to Chromium, but only in the context of referring to it as the source of this list. Thanks for taking another look. Comment on attachment 186892 [details]
Patch
Looks good to me.
Comment on attachment 186892 [details] Patch Clearing flags on attachment: 186892 Committed r142068: <http://trac.webkit.org/changeset/142068> All reviewed patches have been landed. Closing bug. |