|
|
Created:
4 years, 3 months ago by Nate Chapin Modified:
4 years, 3 months ago Reviewers:
hiroshige CC:
chromium-reviews, blink-reviews, loading-reviews+fetch_chromium.org, tyoshino+watch_chromium.org, gavinp+loader_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't attempt to revalidate a Resource with a 304 response.
...which is counter-intuitive. Normally, a revalidation is started on a Resource
with a 2xx response. If the revalidation is successful (i.e, a 304 response),
Resource updates it's m_response with the headers that were received on the
304 response, but leaves the original 2xx response status code. A Resource with
a 304 status code on its m_response therefore received the 304 without thinking
it issued a revaldation. This could happen as a result of a server behaving
badly, or it could happen because an XHR was created with manually added
revalidation headers. In either case, the Resource is not guaranteed to have
sufficient information to perform a successful revalidtion, so don't attempt a
revalidation with it.
BUG=643659
TEST=ResourceTest.Revalidate304
Committed: https://crrev.com/d343f801f8cd63f985180bb4acb9cd4949e4c4b9
Cr-Commit-Position: refs/heads/master@{#420771}
Patch Set 1 #Patch Set 2 : Move fix to ResourceFetcher::determineRevalidationPolicy, test to ResourceFetcherTest #
Total comments: 6
Patch Set 3 : Fix comment issues #
Messages
Total messages: 25 (14 generated)
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
japhet@chromium.org changed reviewers: + hiroshige@chromium.org
hiroshige, does this seem reasonable to you?
On 2016/09/14 21:09:23, Nate Chapin wrote: > hiroshige, does this seem reasonable to you? Thanks for fixing! 1. This CL forbids |Revalidate| of 304 responses, but I think we should also forbid |Use| of such responses. (I think we should return |Reload| before Line 724 of ResourceFetcher.cpp, e.g. by adding an if statement around |if (request.isConditional())| at Line 706 which is also related to manually-set request headers, or adding a condition to Resource::canReuse() etc.) 2. In general, we might have to avoid caching Resource with manually set cache-related headers. Fetch spec says (Step 10 of https://fetch.spec.whatwg.org/#http-network-or-cache-fetch): > If httpRequest's cache mode is "default" and httpRequest's header list contains a header named `If-Modified-Since`, `If-None-Match`, `If-Unmodified-Since`, `If-Match`, or `If-Range`, set httpRequest's cache mode to "no-store". So making determineRevalidationPolicy() to return Reload when existingResource has manually set cache-related request headers might be better. In current implementation, Resource::m_resourceRequest contains both manually-set and non-manually-set request headers so changes might be larger though. To make the change small and mergeable to stable, disabling cache only for 304 responses will be an acceptable fix, if we can assume setting manually-set cache-related request headers only affects responses by changing response to 304.
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/15 06:27:25, hiroshige wrote: > On 2016/09/14 21:09:23, Nate Chapin wrote: > > hiroshige, does this seem reasonable to you? > > Thanks for fixing! > > 1. > This CL forbids |Revalidate| of 304 responses, but I think we should also forbid > |Use| of such responses. > (I think we should return |Reload| before Line 724 of ResourceFetcher.cpp, e.g. > by adding an if statement around |if (request.isConditional())| at Line 706 > which is also related to manually-set request headers, or adding a condition to > Resource::canReuse() etc.) Done. Combined with request.isConditional() clause, since they're strongly related. > > 2. > In general, we might have to avoid caching Resource with manually set > cache-related headers. > Fetch spec says (Step 10 of > https://fetch.spec.whatwg.org/#http-network-or-cache-fetch): > > If httpRequest's cache mode is "default" and httpRequest's header list > contains a header named `If-Modified-Since`, `If-None-Match`, > `If-Unmodified-Since`, `If-Match`, or `If-Range`, set httpRequest's cache mode > to "no-store". > So making determineRevalidationPolicy() to return Reload when existingResource > has manually set cache-related request headers might be better. > > In current implementation, Resource::m_resourceRequest contains both > manually-set and non-manually-set request headers so changes might be larger > though. > To make the change small and mergeable to stable, disabling cache only for 304 > responses will be an acceptable fix, if we can assume setting manually-set > cache-related request headers only affects responses by changing response to > 304. Agreed on all counts. Will try to address this in a follow up CL once this is landed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/19 23:31:48, Nate Chapin wrote: > On 2016/09/15 06:27:25, hiroshige wrote: > > On 2016/09/14 21:09:23, Nate Chapin wrote: > > > hiroshige, does this seem reasonable to you? > > > > Thanks for fixing! > > > > 1. > > This CL forbids |Revalidate| of 304 responses, but I think we should also > forbid > > |Use| of such responses. > > (I think we should return |Reload| before Line 724 of ResourceFetcher.cpp, > e.g. > > by adding an if statement around |if (request.isConditional())| at Line 706 > > which is also related to manually-set request headers, or adding a condition > to > > Resource::canReuse() etc.) > > Done. Combined with request.isConditional() clause, since they're strongly > related. > > > > > 2. > > In general, we might have to avoid caching Resource with manually set > > cache-related headers. > > Fetch spec says (Step 10 of > > https://fetch.spec.whatwg.org/#http-network-or-cache-fetch): > > > If httpRequest's cache mode is "default" and httpRequest's header list > > contains a header named `If-Modified-Since`, `If-None-Match`, > > `If-Unmodified-Since`, `If-Match`, or `If-Range`, set httpRequest's cache mode > > to "no-store". > > So making determineRevalidationPolicy() to return Reload when existingResource > > has manually set cache-related request headers might be better. > > > > In current implementation, Resource::m_resourceRequest contains both > > manually-set and non-manually-set request headers so changes might be larger > > though. > > To make the change small and mergeable to stable, disabling cache only for 304 > > responses will be an acceptable fix, if we can assume setting manually-set > > cache-related request headers only affects responses by changing response to > > 304. > > Agreed on all counts. Will try to address this in a follow up CL once this is > landed. Friendly ping :)
lgtm with comments. https://codereview.chromium.org/2341893002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2341893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:715: // aremains 304. In this case, the Resource likely has insufficient context nit: s/aremains/remains/? https://codereview.chromium.org/2341893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:716: // to provide a useful revalidation. > "context to a useful revalidation" This looks like this condition is only for preventing Revalidation, but actually also we should prevent Use. https://codereview.chromium.org/2341893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:716: // to provide a useful revalidation. Could you refer to the crbug entry to provide more context?
https://codereview.chromium.org/2341893002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2341893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:715: // aremains 304. In this case, the Resource likely has insufficient context On 2016/09/23 16:00:19, hiroshige wrote: > nit: s/aremains/remains/? Done. https://codereview.chromium.org/2341893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:716: // to provide a useful revalidation. On 2016/09/23 16:00:19, hiroshige wrote: > > "context to a useful revalidation" > This looks like this condition is only for preventing Revalidation, but actually > also we should prevent Use. Done. https://codereview.chromium.org/2341893002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:716: // to provide a useful revalidation. On 2016/09/23 16:00:19, hiroshige wrote: > Could you refer to the crbug entry to provide more context? Done.
The CQ bit was checked by japhet@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hiroshige@chromium.org Link to the patchset: https://codereview.chromium.org/2341893002/#ps40001 (title: "Fix comment issues")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by japhet@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Don't attempt to revalidate a Resource with a 304 response. ...which is counter-intuitive. Normally, a revalidation is started on a Resource with a 2xx response. If the revalidation is successful (i.e, a 304 response), Resource updates it's m_response with the headers that were received on the 304 response, but leaves the original 2xx response status code. A Resource with a 304 status code on its m_response therefore received the 304 without thinking it issued a revaldation. This could happen as a result of a server behaving badly, or it could happen because an XHR was created with manually added revalidation headers. In either case, the Resource is not guaranteed to have sufficient information to perform a successful revalidtion, so don't attempt a revalidation with it. BUG=643659 TEST=ResourceTest.Revalidate304 ========== to ========== Don't attempt to revalidate a Resource with a 304 response. ...which is counter-intuitive. Normally, a revalidation is started on a Resource with a 2xx response. If the revalidation is successful (i.e, a 304 response), Resource updates it's m_response with the headers that were received on the 304 response, but leaves the original 2xx response status code. A Resource with a 304 status code on its m_response therefore received the 304 without thinking it issued a revaldation. This could happen as a result of a server behaving badly, or it could happen because an XHR was created with manually added revalidation headers. In either case, the Resource is not guaranteed to have sufficient information to perform a successful revalidtion, so don't attempt a revalidation with it. BUG=643659 TEST=ResourceTest.Revalidate304 Committed: https://crrev.com/d343f801f8cd63f985180bb4acb9cd4949e4c4b9 Cr-Commit-Position: refs/heads/master@{#420771} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/d343f801f8cd63f985180bb4acb9cd4949e4c4b9 Cr-Commit-Position: refs/heads/master@{#420771} |