Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1558703002:
Synchronous requests that get a 304 clobber the cached response body. (Closed)
|
Created:
4 years, 11 months ago by Nate Chapin Modified:
4 years, 11 months ago Reviewers:
Yoav Weiss CC:
chromium-reviews, blink-reviews, loading-reviews+fetch_chromium.org, tyoshino+watch_chromium.org, gavinp+loader_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSynchronous requests that get a 304 clobber the cached response body.
ResourceLoader::requestSynchronously blindly copies the response data to
Resource, even if it's empty. In the 304 case, this results in overwriting the
correct data in Resource::m_data with an empty buffer.
BUG=570622
TEST=http/tests/cache/sync-xhr-304.html
Committed: https://crrev.com/a3e33238e770cc9543f89db94329fc5fdfadc2ef
Cr-Commit-Position: refs/heads/master@{#368497}
Patch Set 1 #
Total comments: 3
Patch Set 2 : #Patch Set 3 : Add comment #
Messages
Total messages: 21 (8 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/patch-status/1558703002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1558703002/1
japhet@chromium.org changed reviewers: + yoav@yoav.ws
yoav, would you mind reviewing this? A nice, small bugfix :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sure! :) https://codereview.chromium.org/1558703002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right): https://codereview.chromium.org/1558703002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:519: if (dataOut.size()) Isn't there a scenario where the response *has* changed to be a zero sized response and should be clobbered? Wouldn't it be better the special case 304s?
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
https://codereview.chromium.org/1558703002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right): https://codereview.chromium.org/1558703002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:519: if (dataOut.size()) On 2015/12/31 06:29:17, Yoav Weiss wrote: > Isn't there a scenario where the response *has* changed to be a zero sized > response and should be clobbered? Wouldn't it be better the special case 304s? In all non-revalidation cases, Resource::m_data will be empty at this point. If a revalidation attempt returns something other than a 304, Resource::revalidationFailed() will clear the buffer automatically, so calling setResourceBuffer() again with an empty buffer will be a noop. However, I just noticed that I accidnetally left the m_fetcher->didReceiveData() call outside of the if(), which seems wrong for the 304 case. Updated that.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1558703002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1558703002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The relationship between 304 and dataOut.size() is not entirely clear to me, sorry. I'll try to catch you on irc to cut down on the RTT. https://codereview.chromium.org/1558703002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right): https://codereview.chromium.org/1558703002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:519: if (dataOut.size()) On 2016/01/04 22:15:54, Nate Chapin wrote: > On 2015/12/31 06:29:17, Yoav Weiss wrote: > > Isn't there a scenario where the response *has* changed to be a zero sized > > response and should be clobbered? Wouldn't it be better the special case 304s? > > In all non-revalidation cases, Resource::m_data will be empty at this point. What's the relationship between Resource::m_data and dataOut? > If a revalidation attempt returns something other than a 304, > Resource::revalidationFailed() will clear the buffer automatically, so calling > setResourceBuffer() again with an empty buffer will be a noop. so `dataOut.size()` is non-zero in all cases other than 304? Could you add a comment making that explicit? > > However, I just noticed that I accidnetally left the m_fetcher->didReceiveData() > call outside of the if(), which seems wrong for the 304 case. Updated that.
On 2016/01/05 14:13:03, Yoav Weiss wrote: > The relationship between 304 and dataOut.size() is not entirely clear to me, > sorry. I'll try to catch you on irc to cut down on the RTT. > > https://codereview.chromium.org/1558703002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right): > > https://codereview.chromium.org/1558703002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:519: if (dataOut.size()) > On 2016/01/04 22:15:54, Nate Chapin wrote: > > On 2015/12/31 06:29:17, Yoav Weiss wrote: > > > Isn't there a scenario where the response *has* changed to be a zero sized > > > response and should be clobbered? Wouldn't it be better the special case > 304s? > > > > In all non-revalidation cases, Resource::m_data will be empty at this point. > > What's the relationship between Resource::m_data and dataOut? > > > If a revalidation attempt returns something other than a 304, > > Resource::revalidationFailed() will clear the buffer automatically, so calling > > setResourceBuffer() again with an empty buffer will be a noop. > > so `dataOut.size()` is non-zero in all cases other than 304? > Could you add a comment making that explicit? dataOut should be empty any time the http response body is empty. 304s *have* to have an empty body, but other requests can, too. In terms of the relation between the buffers and the various cases we need to handle, hopefully the wall of text below is clearer than the little bits I've written before. Let me know if it makes sense :) In the synchronous request case, there are two buffers. dataOut is the buffer that will be populated by WebURLLoader with the http response body. Resource::m_data is the final destination for the bytes, and is what will be stored in MemoryCache. If we are attempting a normal, non-revalidation request: * Resource::m_data is null when ResourceLoader::requestSynchronously is called. * dataOut is populated as a result of the network request. * If dataOut is non-empty (i.e., the http response has a body), it will be transferred to Resource::m_data by the setResourceBuffer() call. * Otherwise, dataOut is empty (i.e., an empty http response body), it is a noop to transfer to Resource::m_data. This patch changes our behavior from performing the noop to skipping the noop. If we are attempting a revalidation: * Resource::m_data is non-null and contains the data we previously cached when ResourceLoader::requestSynchronously is called. * dataOut is populated as a result of the network request. * If the http response was a 304, the body will be empty per the http spec. So dataOut will be empty, and we want to make sure we don't clobber the valid data in Resource::m_data, so setResourceBuffer() must not be called. This is the case we're trying to fix. * Otherwise, the http response is not a 304. Clear Resource::m_data, since its contents are no longer fresh (this is done in Resource::revalidationFailed). * If the http response is not a 304 and dataOut is non-empty, write dataOut to Resource::m_data. * Otherwise, the http response is not a 304 and dataOut is empty, it is a noop to transfer to Resource::m_data. This patch changes our behavior from performing the noop to skipping the noop.
On 2016/01/05 22:53:02, Nate Chapin wrote: > On 2016/01/05 14:13:03, Yoav Weiss wrote: > > The relationship between 304 and dataOut.size() is not entirely clear to me, > > sorry. I'll try to catch you on irc to cut down on the RTT. > > > > > https://codereview.chromium.org/1558703002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right): > > > > > https://codereview.chromium.org/1558703002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:519: if > (dataOut.size()) > > On 2016/01/04 22:15:54, Nate Chapin wrote: > > > On 2015/12/31 06:29:17, Yoav Weiss wrote: > > > > Isn't there a scenario where the response *has* changed to be a zero sized > > > > response and should be clobbered? Wouldn't it be better the special case > > 304s? > > > > > > In all non-revalidation cases, Resource::m_data will be empty at this point. > > > > What's the relationship between Resource::m_data and dataOut? > > > > > If a revalidation attempt returns something other than a 304, > > > Resource::revalidationFailed() will clear the buffer automatically, so > calling > > > setResourceBuffer() again with an empty buffer will be a noop. > > > > so `dataOut.size()` is non-zero in all cases other than 304? > > Could you add a comment making that explicit? > > dataOut should be empty any time the http response body is empty. 304s *have* to > have an empty body, but other requests can, too. > > > In terms of the relation between the buffers and the various cases we need to > handle, hopefully the wall of text below is clearer than the little bits I've > written before. Let me know if it makes sense :) > > In the synchronous request case, there are two buffers. dataOut is the buffer > that will be populated by WebURLLoader with the http response body. > Resource::m_data is the final destination for the bytes, and is what will be > stored in MemoryCache. > > If we are attempting a normal, non-revalidation request: > * Resource::m_data is null when ResourceLoader::requestSynchronously is called. > * dataOut is populated as a result of the network request. > * If dataOut is non-empty (i.e., the http response has a body), it will be > transferred to Resource::m_data by the setResourceBuffer() call. > * Otherwise, dataOut is empty (i.e., an empty http response body), it is a noop > to transfer to Resource::m_data. This patch changes our behavior from performing > the noop to skipping the noop. > > If we are attempting a revalidation: > * Resource::m_data is non-null and contains the data we previously cached when > ResourceLoader::requestSynchronously is called. > * dataOut is populated as a result of the network request. > * If the http response was a 304, the body will be empty per the http spec. So > dataOut will be empty, and we want to make sure we don't clobber the valid data > in Resource::m_data, so setResourceBuffer() must not be called. This is the case > we're trying to fix. > * Otherwise, the http response is not a 304. Clear Resource::m_data, since its > contents are no longer fresh (this is done in Resource::revalidationFailed). > * If the http response is not a 304 and dataOut is non-empty, write dataOut to > Resource::m_data. > * Otherwise, the http response is not a 304 and dataOut is empty, it is a noop > to transfer to Resource::m_data. This patch changes our behavior from performing > the noop to skipping the noop. Makes sense :) Maybe worthwhile adding a short comment explaining the reasons for this condition? In any case, LGTM!
The CQ bit was checked by japhet@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yoav@yoav.ws Link to the patchset: https://codereview.chromium.org/1558703002/#ps40001 (title: "Add comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1558703002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1558703002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Synchronous requests that get a 304 clobber the cached response body. ResourceLoader::requestSynchronously blindly copies the response data to Resource, even if it's empty. In the 304 case, this results in overwriting the correct data in Resource::m_data with an empty buffer. BUG=570622 TEST=http/tests/cache/sync-xhr-304.html ========== to ========== Synchronous requests that get a 304 clobber the cached response body. ResourceLoader::requestSynchronously blindly copies the response data to Resource, even if it's empty. In the 304 case, this results in overwriting the correct data in Resource::m_data with an empty buffer. BUG=570622 TEST=http/tests/cache/sync-xhr-304.html Committed: https://crrev.com/a3e33238e770cc9543f89db94329fc5fdfadc2ef Cr-Commit-Position: refs/heads/master@{#368497} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a3e33238e770cc9543f89db94329fc5fdfadc2ef Cr-Commit-Position: refs/heads/master@{#368497} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
