Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2160403002:
Dont preload from link headers if the response came from MemoryCache (Closed)
|
Created:
4 years, 5 months ago by Charlie Harrison Modified:
4 years, 5 months ago CC:
chromium-reviews, gavinp+prerender_chromium.org, tyoshino+watch_chromium.org, loading-reviews_chromium.org, gavinp+loader_chromium.org, blink-reviews, Nate Chapin Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDont preload from link headers if the response came from MemoryCache
This patch ensures that in we don't end up in a stack overflow if a
resource in memory cache has link headers directing to preload itself.
BUG=628904
Committed: https://crrev.com/ef9c921d6ff92819b8d91b604a2255fbe2fef068
Cr-Commit-Position: refs/heads/master@{#406534}
Patch Set 1 #
Total comments: 8
Messages
Total messages: 21 (8 generated)
The CQ bit was checked by csharrison@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...
csharrison@chromium.org changed reviewers: + japhet@chromium.org
japhet@, can you take a look? I had trouble replicating this bug in a layout test due to the resource always being in m_validatedURLs. Also cc Yoav, if you want to take a look.
yoav@yoav.ws changed reviewers: + yoav@yoav.ws
https://codereview.chromium.org/2160403002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/2160403002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:789: void FrameFetchContext::dispatchDidReceiveResponseInternal(unsigned long identifier, const ResourceResponse& response, WebURLRequest::FrameType frameType, WebURLRequest::RequestContext requestContext, Resource* resource, LinkLoader::CanLoadResources resourceLoadingPolicy) Is there a functional change here? Or is this just a refactoring?
https://codereview.chromium.org/2160403002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/2160403002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:789: void FrameFetchContext::dispatchDidReceiveResponseInternal(unsigned long identifier, const ResourceResponse& response, WebURLRequest::FrameType frameType, WebURLRequest::RequestContext requestContext, Resource* resource, LinkLoader::CanLoadResources resourceLoadingPolicy) On 2016/07/19 21:04:03, Yoav Weiss wrote: > Is there a functional change here? Or is this just a refactoring? No functional change, except to pull resourceLoadingPolicy into an argument. I didn't want to do this in the base class because core/fetch should not know about LinkLoader.
https://codereview.chromium.org/2160403002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/2160403002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:789: void FrameFetchContext::dispatchDidReceiveResponseInternal(unsigned long identifier, const ResourceResponse& response, WebURLRequest::FrameType frameType, WebURLRequest::RequestContext requestContext, Resource* resource, LinkLoader::CanLoadResources resourceLoadingPolicy) On 2016/07/19 21:11:02, csharrison wrote: > On 2016/07/19 21:04:03, Yoav Weiss wrote: > > Is there a functional change here? Or is this just a refactoring? > > No functional change, except to pull resourceLoadingPolicy into an argument. I > didn't want to do this in the base class because core/fetch should not know > about LinkLoader. I'm totally fine with this refactoring, just wasn't sure how it's related to this patch. https://codereview.chromium.org/2160403002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right): https://codereview.chromium.org/2160403002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/LinkLoader.cpp:318: continue; Is this specific to MemoryCache? Seems like if I'm serving a response with a link preload for the same URL, this would protect me from that as well (regardless of MemoryCache)
https://codereview.chromium.org/2160403002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right): https://codereview.chromium.org/2160403002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/LinkLoader.cpp:318: continue; On 2016/07/19 21:14:22, Yoav Weiss wrote: > Is this specific to MemoryCache? Seems like if I'm serving a response with a > link preload for the same URL, this would protect me from that as well > (regardless of MemoryCache) Ah no this isn't specific to memory cache. I added this as a sanity check to make sure that this kind of problem doesn't come back in a slightly different form. Happy to remove it but I feel like we might as well abort early and avoid looking up the resource in memory cache, etc.
https://codereview.chromium.org/2160403002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right): https://codereview.chromium.org/2160403002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/LinkLoader.cpp:318: continue; On 2016/07/19 21:23:52, csharrison wrote: > On 2016/07/19 21:14:22, Yoav Weiss wrote: > > Is this specific to MemoryCache? Seems like if I'm serving a response with a > > link preload for the same URL, this would protect me from that as well > > (regardless of MemoryCache) > > Ah no this isn't specific to memory cache. I added this as a sanity check to > make sure that this kind of problem doesn't come back in a slightly different > form. Happy to remove it but I feel like we might as well abort early and avoid > looking up the resource in memory cache, etc. I don't think you should remove it. I'm probably missing something, but I guess my main trouble with this CL is to figure out which parts of the code do what the title says it does. (so "Dont preload from link headers if the response came from MemoryCache") Is there code in the CL that's doing something MemoryCache specific? If not, maybe a title change is appropriate
https://codereview.chromium.org/2160403002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/2160403002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:371: dispatchDidReceiveResponseInternal(identifier, resource->response(), frameType, requestContext, resource, LinkLoader::DoNotLoadResources); To answer your last comment: This line is the functional change besides for the sanity checking of "url == baseURL". We forward calls from dispatchDidLoadResourceFromMemory cache to dispatchDidReceiveResponseInternal with the policy that we shouldn't be loading anything from links.
LGTM. On the one hand, I'm not sure the baseURL != url check wouldn't have been enough to solve this issue. At the same time, if a resource is in MemoryCache, there's a good chance that the resources preloaded from its headers are also there. https://codereview.chromium.org/2160403002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/2160403002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:371: dispatchDidReceiveResponseInternal(identifier, resource->response(), frameType, requestContext, resource, LinkLoader::DoNotLoadResources); On 2016/07/19 21:35:39, csharrison wrote: > To answer your last comment: This line is the functional change besides for the > sanity checking of "url == baseURL". We forward calls from > dispatchDidLoadResourceFromMemory cache to dispatchDidReceiveResponseInternal > with the policy that we shouldn't be loading anything from links. OK, so that's what I missed! :)
You're right the sanity check would have been enough to solve this specific issue, but I was worried about the A -> B -> A -> B loop.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/07/19 21:48:53, csharrison wrote: > You're right the sanity check would have been enough to solve this specific > issue, but I was worried about the A -> B -> A -> B loop. makes sense. ship it! :)
The CQ bit was checked by csharrison@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 #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Dont preload from link headers if the response came from MemoryCache This patch ensures that in we don't end up in a stack overflow if a resource in memory cache has link headers directing to preload itself. BUG=628904 ========== to ========== Dont preload from link headers if the response came from MemoryCache This patch ensures that in we don't end up in a stack overflow if a resource in memory cache has link headers directing to preload itself. BUG=628904 Committed: https://crrev.com/ef9c921d6ff92819b8d91b604a2255fbe2fef068 Cr-Commit-Position: refs/heads/master@{#406534} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/ef9c921d6ff92819b8d91b604a2255fbe2fef068 Cr-Commit-Position: refs/heads/master@{#406534} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
