Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
25772002:
Allows prefetch requests to live beyond the renderer by delaying (Closed)
|
Created:
7 years, 2 months ago by jkarlin2 Modified:
7 years, 1 month ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, cbentzel, tonyg Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAllows prefetch and other detachable requests to live beyond the renderer by delaying LinkLoader::Cancel requests by a few seconds and through the use of a new DetachedResourceHandler.
The general concept of a detachable resource should be useful for <a ping> as well.
BUG=286186
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231910
Patch Set 1 #Patch Set 2 : Added a delay-prefetch-cancellation flag so that the delay is default off. #Patch Set 3 : Change delay from 3s to 60s for initial timing. Obey the browser process when it wants to cancel. #Patch Set 4 : Change delay from 3s to 60s for initial timing. Obey the browser process when it wants to cancel. #
Total comments: 11
Patch Set 5 : Addressed mmenke's comments. Fixed a unit test breakage caused by not delaying cancel when not cal… #Patch Set 6 : Cancellation delay now settable at command line #
Total comments: 11
Patch Set 7 : Remove unnecessary timer check. #
Total comments: 2
Patch Set 8 : Rebase. Sorry! #Patch Set 9 : Adds a DetachedResourceHandler. #Patch Set 10 : Added DetachableType function for generality. #
Total comments: 38
Patch Set 11 : Addressing great comments from mmenke. #
Total comments: 18
Patch Set 12 : Asanka fixes. Made more general for future <a ping> support. #
Total comments: 3
Patch Set 13 : Possible AsyncResourceHandler support for detached resources. #
Total comments: 29
Patch Set 14 : Addressed all comments. Lots of tests. Using AsyncResourceHandler. #
Total comments: 22
Patch Set 15 : Addressed mmenke's comments #
Total comments: 7
Patch Set 16 : Created a detached state. Data is sent to renderer until detached. Fixed up tests. #
Total comments: 4
Patch Set 17 : Addressed simonjam's comments. #Patch Set 18 : Rebased. Changed from 60 second timeout to 30 seconds. #Patch Set 19 : Rebase. #Messages
Total messages: 58 (0 generated)
Ping?
Drive-by comments. https://codereview.chromium.org/25772002/diff/12001/content/browser/loader/as... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/25772002/diff/12001/content/browser/loader/as... content/browser/loader/async_resource_handler.cc:395: CHECK(!(detached_reads_ && detached_reads)); Unless there's some reason to be particularly concerned about this happening, this should be a DCHECK. Also, if we don't support moving from detached to not detached, should we even be taking in a bool? https://codereview.chromium.org/25772002/diff/12001/content/browser/loader/re... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/25772002/diff/12001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_impl.cc:411: // aren't cancelled when the associated processes go away. It may be OK for Update comment. https://codereview.chromium.org/25772002/diff/12001/content/browser/loader/re... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/25772002/diff/12001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_unittest.cc:617: const GURL& url); I think it's a bad idea to have two ways to set the resource type. I'd suggest getting rid of the old method, updating old callers and getting rid of resource_type_. Old one is only used 8 or 9 times, so not a huge effort. https://codereview.chromium.org/25772002/diff/12001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_unittest.cc:617: const GURL& url); Google style guide discourages operator overloading. I'd suggest just renaming this to MakeTestRequestWithResourceType. Also suggest making type the last parameter, to keep consistent parameter ordering between functions. https://codereview.chromium.org/25772002/diff/12001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_unittest.cc:619: nit: Remove extra blank line.
https://codereview.chromium.org/25772002/diff/12001/content/browser/loader/re... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/25772002/diff/12001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_unittest.cc:617: const GURL& url); On 2013/10/07 19:16:24, mmenke wrote: > Google style guide discourages operator overloading. I'd suggest just renaming > this to MakeTestRequestWithResourceType. Also suggest making type the last > parameter, to keep consistent parameter ordering between functions. -operator + function
https://codereview.chromium.org/25772002/diff/12001/content/browser/loader/as... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/25772002/diff/12001/content/browser/loader/as... content/browser/loader/async_resource_handler.cc:395: CHECK(!(detached_reads_ && detached_reads)); On 2013/10/07 19:16:24, mmenke wrote: > Unless there's some reason to be particularly concerned about this happening, > this should be a DCHECK. > > Also, if we don't support moving from detached to not detached, should we even > be taking in a bool? Done. https://codereview.chromium.org/25772002/diff/12001/content/browser/loader/re... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/25772002/diff/12001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_impl.cc:411: // aren't cancelled when the associated processes go away. It may be OK for On 2013/10/07 19:16:24, mmenke wrote: > Update comment. Done. https://codereview.chromium.org/25772002/diff/12001/content/browser/loader/re... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/25772002/diff/12001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_unittest.cc:617: const GURL& url); On 2013/10/07 19:16:24, mmenke wrote: > I think it's a bad idea to have two ways to set the resource type. I'd suggest > getting rid of the old method, updating old callers and getting rid of > resource_type_. Old one is only used 8 or 9 times, so not a huge effort. Done. https://codereview.chromium.org/25772002/diff/12001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_unittest.cc:617: const GURL& url); On 2013/10/07 19:16:24, mmenke wrote: > Google style guide discourages operator overloading. I'd suggest just renaming > this to MakeTestRequestWithResourceType. Also suggest making type the last > parameter, to keep consistent parameter ordering between functions. Done, though the precedent is already set in the file to overload that function. I think the code was cleaner before, though it is probably a little clearer now. https://codereview.chromium.org/25772002/diff/12001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_unittest.cc:619: On 2013/10/07 19:16:24, mmenke wrote: > nit: Remove extra blank line. Done.
https://codereview.chromium.org/25772002/diff/23001/content/browser/loader/as... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/25772002/diff/23001/content/browser/loader/as... content/browser/loader/async_resource_handler.cc:182: return false; So we still cancel prefetches if the renderer is destroyed before the response is started, though we only do it once we start receiving a response? And the same thing for redirects? I'm not hugely concerned about this, just want to make sure we understand what this patch does and doesn't do. https://codereview.chromium.org/25772002/diff/23001/content/browser/loader/re... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/25772002/diff/23001/content/browser/loader/re... content/browser/loader/resource_loader.cc:469: info.GetResourceType() == ResourceType::PREFETCH) { Know it doesn't really matter, but seems like checking the resource type should go before doing a search through the map of command line flags, for performance reasons. https://codereview.chromium.org/25772002/diff/23001/content/browser/loader/re... content/browser/loader/resource_loader.cc:470: if (prefetch_timer_.get() && prefetch_timer_->IsRunning()) { If something is impossible, should we be checking it? Seems like we should just DCHECK(prefetch_timer_->IsRunning()) here instead. https://codereview.chromium.org/25772002/diff/23001/content/browser/loader/re... content/browser/loader/resource_loader.cc:473: } else if (!prefetch_timer_.get()) { nit: Here and just above, believe "if (!prefetch_timer_)" is the preferred form (No get). Since we support it, may as well use it. https://codereview.chromium.org/25772002/diff/23001/content/browser/loader/re... File content/browser/loader/resource_loader.h (right): https://codereview.chromium.org/25772002/diff/23001/content/browser/loader/re... content/browser/loader/resource_loader.h:143: base::WeakPtrFactory<ResourceLoader> weak_ptr_factory_; WeakPtrFactories should always go last. The static won't cause a problem, of course, but it may cause someone else who adds something to this class to overlook the factory.
https://codereview.chromium.org/25772002/diff/23001/content/browser/loader/as... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/25772002/diff/23001/content/browser/loader/as... content/browser/loader/async_resource_handler.cc:182: return false; On 2013/10/08 14:50:54, mmenke wrote: > So we still cancel prefetches if the renderer is destroyed before the response > is started, though we only do it once we start receiving a response? And the > same thing for redirects? Are you saying that the 'return false' above will cause a cancel? The ResourceLoader seems to ignore the result of this, so it shouldn't cancel the prefetch. https://codereview.chromium.org/25772002/diff/23001/content/browser/loader/re... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/25772002/diff/23001/content/browser/loader/re... content/browser/loader/resource_loader.cc:469: info.GetResourceType() == ResourceType::PREFETCH) { On 2013/10/08 14:50:54, mmenke wrote: > Know it doesn't really matter, but seems like checking the resource type should > go before doing a search through the map of command line flags, for performance > reasons. Done. https://codereview.chromium.org/25772002/diff/23001/content/browser/loader/re... content/browser/loader/resource_loader.cc:470: if (prefetch_timer_.get() && prefetch_timer_->IsRunning()) { On 2013/10/08 14:50:54, mmenke wrote: > If something is impossible, should we be checking it? Seems like we should just > DCHECK(prefetch_timer_->IsRunning()) here instead. Done. https://codereview.chromium.org/25772002/diff/23001/content/browser/loader/re... File content/browser/loader/resource_loader.h (right): https://codereview.chromium.org/25772002/diff/23001/content/browser/loader/re... content/browser/loader/resource_loader.h:143: base::WeakPtrFactory<ResourceLoader> weak_ptr_factory_; On 2013/10/08 14:50:54, mmenke wrote: > WeakPtrFactories should always go last. The static won't cause a problem, of > course, but it may cause someone else who adds something to this class to > overlook the factory. Done.
https://codereview.chromium.org/25772002/diff/23001/content/browser/loader/as... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/25772002/diff/23001/content/browser/loader/as... content/browser/loader/async_resource_handler.cc:182: return false; On 2013/10/09 14:44:33, jkarlin wrote: > On 2013/10/08 14:50:54, mmenke wrote: > > So we still cancel prefetches if the renderer is destroyed before the response > > is started, though we only do it once we start receiving a response? And the > > same thing for redirects? > > Are you saying that the 'return false' above will cause a cancel? The > ResourceLoader seems to ignore the result of this, so it shouldn't cancel the > prefetch. > The return false should indeed result in a cancel. https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo... Now that I think about it more, I'm not sure if info->filter() is NULL in that case, just assumed it would be.
[Sorry if I'm bringing up a design discussion we've already had...] How is <a ping> handled? Could prefetch use the same logic?
On 2013/10/09 15:46:23, cbentzel wrote: > [Sorry if I'm bringing up a design discussion we've already had...] > > How is <a ping> handled? Could prefetch use the same logic? According to davidben, it suffers from the same problem (Which is one of the two reasons it doesn't work with prerenders). He pointed me to this CL, thinking we'd made <a ping> use the same code path.
On 2013/10/09 15:52:41, mmenke wrote: > On 2013/10/09 15:46:23, cbentzel wrote: > > [Sorry if I'm bringing up a design discussion we've already had...] > > > > How is <a ping> handled? Could prefetch use the same logic? > > According to davidben, it suffers from the same problem (Which is one of the two > reasons it doesn't work with prerenders). He pointed me to this CL, thinking > we'd made <a ping> use the same code path. <a ping> is just a resource load, take a peek at the PingLoader.cpp in blink. So yeah, it'll suffer the same races. I wonder if we want these "resource loads which aren't resource loads" to be considered resource loads at all. Why not treat prefetches exactly as we do prerenders now? We'd have to create the prefetch request in the PrerenderLinkManager, which would mean finding our profile to get a NetworkDelegate, and we'd have to sink the bytes somewhere, but we could also remove a lot of code in blink that currently special cases prerenders and subresource loads. Josh, I know we talked about that approach earlier. Why did you decide against it, and keep with the "prefetches are url requests" model?
On 2013/10/09 16:16:22, gavinp wrote: > On 2013/10/09 15:52:41, mmenke wrote: > > On 2013/10/09 15:46:23, cbentzel wrote: > > > [Sorry if I'm bringing up a design discussion we've already had...] > > > > > > How is <a ping> handled? Could prefetch use the same logic? > > > > According to davidben, it suffers from the same problem (Which is one of the > two > > reasons it doesn't work with prerenders). He pointed me to this CL, thinking > > we'd made <a ping> use the same code path. > > <a ping> is just a resource load, take a peek at the PingLoader.cpp in blink. So > yeah, it'll suffer the same races. > > I wonder if we want these "resource loads which aren't resource loads" to be > considered resource loads at all. Why not treat prefetches exactly as we do > prerenders now? We'd have to create the prefetch request in the > PrerenderLinkManager, which would mean finding our profile to get a > NetworkDelegate, and we'd have to sink the bytes somewhere, but we could also > remove a lot of code in blink that currently special cases prerenders and > subresource loads. > > Josh, I know we talked about that approach earlier. Why did you decide against > it, and keep with the "prefetches are url requests" model? Not sure we'd want PrerenderLinkManager to handle them - just doesn't seem like they need all that infrastructure, and don't think we want to add yet more complexity to the prerendering code. Also, isn't it better to have the requests to go through the ResourceScheduler, like normal requests, so it can defer them as needed.
Yes, as noted in the bug, we need the ResourceScheduler. On Wed, Oct 9, 2013 at 12:22 PM, <mmenke@chromium.org> wrote: > On 2013/10/09 16:16:22, gavinp wrote: > >> On 2013/10/09 15:52:41, mmenke wrote: >> > On 2013/10/09 15:46:23, cbentzel wrote: >> > > [Sorry if I'm bringing up a design discussion we've already had...] >> > > >> > > How is <a ping> handled? Could prefetch use the same logic? >> > >> > According to davidben, it suffers from the same problem (Which is one >> of the >> two >> > reasons it doesn't work with prerenders). He pointed me to this CL, >> > thinking > >> > we'd made <a ping> use the same code path. >> > > <a ping> is just a resource load, take a peek at the PingLoader.cpp in >> blink. >> > So > >> yeah, it'll suffer the same races. >> > > I wonder if we want these "resource loads which aren't resource loads" to >> be >> considered resource loads at all. Why not treat prefetches exactly as we >> do >> prerenders now? We'd have to create the prefetch request in the >> PrerenderLinkManager, which would mean finding our profile to get a >> NetworkDelegate, and we'd have to sink the bytes somewhere, but we could >> also >> remove a lot of code in blink that currently special cases prerenders and >> subresource loads. >> > > Josh, I know we talked about that approach earlier. Why did you decide >> against >> it, and keep with the "prefetches are url requests" model? >> > > Not sure we'd want PrerenderLinkManager to handle them - just doesn't seem > like > they need all that infrastructure, and don't think we want to add yet more > complexity to the prerendering code. Also, isn't it better to have the > requests > to go through the ResourceScheduler, like normal requests, so it can defer > them > as needed. > > https://codereview.chromium.**org/25772002/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/25772002/diff/23001/content/browser/loader/as... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/25772002/diff/23001/content/browser/loader/as... content/browser/loader/async_resource_handler.cc:182: return false; On 2013/10/09 14:59:08, mmenke wrote: > On 2013/10/09 14:44:33, jkarlin wrote: > > On 2013/10/08 14:50:54, mmenke wrote: > > > So we still cancel prefetches if the renderer is destroyed before the > response > > > is started, though we only do it once we start receiving a response? And > the > > > same thing for redirects? > > > > Are you saying that the 'return false' above will cause a cancel? The > > ResourceLoader seems to ignore the result of this, so it shouldn't cancel the > > prefetch. > > > > The return false should indeed result in a cancel. > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo... > > Now that I think about it more, I'm not sure if info->filter() is NULL in that > case, just assumed it would be. Right, not sure how I didn't see that. It seems reasonable to expect the filter to be gone if the renderer is gone, but we don't want these prefetches (and potentially <a ping>'s later) to be canceled. It seems somewhat icky to hack in a guard at the beginning of each function and ignore the setup that say OnResponseStarted goes through. I think a new ResourceHandler, say, DetachedResourceHandler, is required that only signals close and error signals to the renderer, and loads regardless of the filter's existence.
Flushing nits, as it sounds like a refactoring is in the pipe. https://codereview.chromium.org/25772002/diff/33001/content/browser/loader/as... File content/browser/loader/async_resource_handler.h (right): https://codereview.chromium.org/25772002/diff/33001/content/browser/loader/as... content/browser/loader/async_resource_handler.h:69: void DetachReads(); Nit: I vote for inlining this. https://codereview.chromium.org/25772002/diff/33001/content/browser/loader/as... content/browser/loader/async_resource_handler.h:96: bool detached_reads_; Naming nit: I like totally unambiguous names for bools, so how about "reads_are_detached_" instead?
On 2013/10/09 17:28:29, jkarlin wrote: > https://codereview.chromium.org/25772002/diff/23001/content/browser/loader/as... > File content/browser/loader/async_resource_handler.cc (right): > > https://codereview.chromium.org/25772002/diff/23001/content/browser/loader/as... > content/browser/loader/async_resource_handler.cc:182: return false; > On 2013/10/09 14:59:08, mmenke wrote: > > On 2013/10/09 14:44:33, jkarlin wrote: > > > On 2013/10/08 14:50:54, mmenke wrote: > > > > So we still cancel prefetches if the renderer is destroyed before the > > response > > > > is started, though we only do it once we start receiving a response? And > > the > > > > same thing for redirects? > > > > > > Are you saying that the 'return false' above will cause a cancel? The > > > ResourceLoader seems to ignore the result of this, so it shouldn't cancel > the > > > prefetch. > > > > > > > The return false should indeed result in a cancel. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo... > > > > Now that I think about it more, I'm not sure if info->filter() is NULL in that > > case, just assumed it would be. > > Right, not sure how I didn't see that. It seems reasonable to expect the filter > to be gone if the renderer is gone, but we don't want these prefetches (and > potentially <a ping>'s later) to be canceled. > > It seems somewhat icky to hack in a guard at the beginning of each function and > ignore the setup that say OnResponseStarted goes through. I think a new > ResourceHandler, say, DetachedResourceHandler, is required that only signals > close and error signals to the renderer, and loads regardless of the filter's > existence. I agree that a new resource handler is probably the way to go - I was thinking along the same lines myself.
On 2013/10/09 19:38:44, mmenke wrote: > On 2013/10/09 17:28:29, jkarlin wrote: > > > https://codereview.chromium.org/25772002/diff/23001/content/browser/loader/as... > > File content/browser/loader/async_resource_handler.cc (right): > > > > > https://codereview.chromium.org/25772002/diff/23001/content/browser/loader/as... > > content/browser/loader/async_resource_handler.cc:182: return false; > > On 2013/10/09 14:59:08, mmenke wrote: > > > On 2013/10/09 14:44:33, jkarlin wrote: > > > > On 2013/10/08 14:50:54, mmenke wrote: > > > > > So we still cancel prefetches if the renderer is destroyed before the > > > response > > > > > is started, though we only do it once we start receiving a response? > And > > > the > > > > > same thing for redirects? > > > > > > > > Are you saying that the 'return false' above will cause a cancel? The > > > > ResourceLoader seems to ignore the result of this, so it shouldn't cancel > > the > > > > prefetch. > > > > > > > > > > The return false should indeed result in a cancel. > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo... > > > > > > Now that I think about it more, I'm not sure if info->filter() is NULL in > that > > > case, just assumed it would be. > > > > Right, not sure how I didn't see that. It seems reasonable to expect the > filter > > to be gone if the renderer is gone, but we don't want these prefetches (and > > potentially <a ping>'s later) to be canceled. > > > > It seems somewhat icky to hack in a guard at the beginning of each function > and > > ignore the setup that say OnResponseStarted goes through. I think a new > > ResourceHandler, say, DetachedResourceHandler, is required that only signals > > close and error signals to the renderer, and loads regardless of the filter's > > existence. > > I agree that a new resource handler is probably the way to go - I was thinking > along the same lines myself. Done.
On 2013/10/10 17:16:11, jkarlin wrote: > On 2013/10/09 19:38:44, mmenke wrote: > > On 2013/10/09 17:28:29, jkarlin wrote: > > > > > > https://codereview.chromium.org/25772002/diff/23001/content/browser/loader/as... > > > File content/browser/loader/async_resource_handler.cc (right): > > > > > > > > > https://codereview.chromium.org/25772002/diff/23001/content/browser/loader/as... > > > content/browser/loader/async_resource_handler.cc:182: return false; > > > On 2013/10/09 14:59:08, mmenke wrote: > > > > On 2013/10/09 14:44:33, jkarlin wrote: > > > > > On 2013/10/08 14:50:54, mmenke wrote: > > > > > > So we still cancel prefetches if the renderer is destroyed before the > > > > response > > > > > > is started, though we only do it once we start receiving a response? > > And > > > > the > > > > > > same thing for redirects? > > > > > > > > > > Are you saying that the 'return false' above will cause a cancel? The > > > > > ResourceLoader seems to ignore the result of this, so it shouldn't > cancel > > > the > > > > > prefetch. > > > > > > > > > > > > > The return false should indeed result in a cancel. > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo... > > > > > > > > Now that I think about it more, I'm not sure if info->filter() is NULL in > > that > > > > case, just assumed it would be. > > > > > > Right, not sure how I didn't see that. It seems reasonable to expect the > > filter > > > to be gone if the renderer is gone, but we don't want these prefetches (and > > > potentially <a ping>'s later) to be canceled. > > > > > > It seems somewhat icky to hack in a guard at the beginning of each function > > and > > > ignore the setup that say OnResponseStarted goes through. I think a new > > > ResourceHandler, say, DetachedResourceHandler, is required that only signals > > > close and error signals to the renderer, and loads regardless of the > filter's > > > existence. > > > > I agree that a new resource handler is probably the way to go - I was thinking > > along the same lines myself. > > Done. Great! I don't think I'll have time to go over this carefully until tomorrow, unfortunately.
Did you look into how downloads handle this case? My main concerns are the potential for unintended consequences, and things that were broken before and this doesn't end up fixing. I'll want to dig around before I'm comfortable signing off on anything, though also should definitely be signed off on by someone more familiar with the ResourceLoader and RDH. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/as... File content/browser/loader/async_resource_handler.h (right): https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/as... content/browser/loader/async_resource_handler.h:69: void DetachReads(); This is no longer used, right? https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... File content/browser/loader/detached_resource_handler.cc (right): https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... content/browser/loader/detached_resource_handler.cc:30: read_buffer_(new net::IOBuffer(kReadBufSize)) {} Reading may be delayed by the ResourceThrottle. Think we should only allocate the read buffer when we first need it, so we don't have a bunch of these hanging around in memory. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... content/browser/loader/detached_resource_handler.cc:39: if (!info->filter()) return false; Put return false on next line. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... content/browser/loader/detached_resource_handler.cc:39: if (!info->filter()) return false; Should we be returning true? Looks like that's what we need to do to be destroyed, looking at ResourceLoader. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... content/browser/loader/detached_resource_handler.cc:45: base::debug::Alias(url_buf); The referenced bug was long since fixed. Do we need this here? https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... content/browser/loader/detached_resource_handler.cc:79: if (!info->filter()) return true; nit: Move return onto another line. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... content/browser/loader/detached_resource_handler.cc:91: if (!info->filter()) return true; nit: Move return onto another line. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... content/browser/loader/detached_resource_handler.cc:95: info->filter()); Hmm...Wonder about some of the stuff this does, like AutoLoginPrompter::ShowInfoBarIfPossible, and OneClickSigninHelper::ShowInfoBarIfPossible. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... content/browser/loader/detached_resource_handler.cc:123: // Give the loader something to write its data to, but we'll never read it. nit: Don't use "we" in comments, since it's ambiguous. One option is to use passive voice. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... File content/browser/loader/detached_resource_handler.h (right): https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... content/browser/loader/detached_resource_handler.h:54: const std::string& security_info) OVERRIDE; nit: Add blank line between methods. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... content/browser/loader/detached_resource_handler.h:56: } Suggest not inlining this. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... content/browser/loader/detached_resource_handler.h:60: kReadBufSize = 3840 Is there a motivation for making it this size? We generally use 32k buffers. I believe network reads correspond to reads we request here (In the non-SSL/non-SPDY/non-QUIC case), so each read results in a round trip to the cache thread, among other things. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... content/browser/loader/detached_resource_handler.h:61: }; Why not just use static const int? This also doesn't need to be defined or declared in the header. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... content/browser/loader/detached_resource_handler.h:65: DISALLOW_COPY_AND_ASSIGN(DetachedResourceHandler); nit: There's generally a blank line before DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/re... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_impl.cc:704: return; Should this only protect against messing with the offline_policy_map? Seems like passing in the ResourceRequestDetails may be important/relevant, though I'm not sure what it actually does. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_impl.cc:1161: bool ResourceDispatcherHostImpl::DetachableType( Suggest IsDetachableResouceType, or maybe IsResouceTypeDetachable. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_impl.cc:1161: bool ResourceDispatcherHostImpl::DetachableType( Since this is only used in this file and doesn't depend on |this|, suggest moving it into a private namespace. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/re... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_unittest.cc:2211: EXPECT_EQ(size_t(2), msgs[0].size()); C-style casts aren't allowed. Just use 2u here. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/re... File content/browser/loader/resource_loader.h (right): https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/re... content/browser/loader/resource_loader.h:145: static int default_delay_prefetch_cancel_ms_; This doesn't need to be in the header, and should be const, using the const naming scheme.
+asanka as reviewer. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/as... File content/browser/loader/async_resource_handler.h (right): https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/as... content/browser/loader/async_resource_handler.h:69: void DetachReads(); On 2013/10/11 16:39:07, mmenke wrote: > This is no longer used, right? Done. And removed other changes to the file. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... File content/browser/loader/detached_resource_handler.cc (right): https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... content/browser/loader/detached_resource_handler.cc:30: read_buffer_(new net::IOBuffer(kReadBufSize)) {} On 2013/10/11 16:39:07, mmenke wrote: > Reading may be delayed by the ResourceThrottle. Think we should only allocate > the read buffer when we first need it, so we don't have a bunch of these hanging > around in memory. Done. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... content/browser/loader/detached_resource_handler.cc:39: if (!info->filter()) return false; On 2013/10/11 16:39:07, mmenke wrote: > Put return false on next line. My auto-google-formatter did that. Done. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... content/browser/loader/detached_resource_handler.cc:39: if (!info->filter()) return false; On 2013/10/11 16:39:07, mmenke wrote: > Should we be returning true? Looks like that's what we need to do to be > destroyed, looking at ResourceLoader. You're right. Done. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... content/browser/loader/detached_resource_handler.cc:45: base::debug::Alias(url_buf); On 2013/10/11 16:39:07, mmenke wrote: > The referenced bug was long since fixed. Do we need this here? Done. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... content/browser/loader/detached_resource_handler.cc:79: if (!info->filter()) return true; On 2013/10/11 16:39:07, mmenke wrote: > nit: Move return onto another line. Auto formatter again. Done. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... content/browser/loader/detached_resource_handler.cc:91: if (!info->filter()) return true; On 2013/10/11 16:39:07, mmenke wrote: > nit: Move return onto another line. Done. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... content/browser/loader/detached_resource_handler.cc:95: info->filter()); On 2013/10/11 16:39:07, mmenke wrote: > Hmm...Wonder about some of the stuff this does, like > AutoLoginPrompter::ShowInfoBarIfPossible, and > OneClickSigninHelper::ShowInfoBarIfPossible. This seems to be an existing issue with prefetch and unrelated to this CL. But perhaps a new bug is in order? https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... content/browser/loader/detached_resource_handler.cc:123: // Give the loader something to write its data to, but we'll never read it. On 2013/10/11 16:39:07, mmenke wrote: > nit: Don't use "we" in comments, since it's ambiguous. One option is to use > passive voice. Done. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... File content/browser/loader/detached_resource_handler.h (right): https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... content/browser/loader/detached_resource_handler.h:54: const std::string& security_info) OVERRIDE; On 2013/10/11 16:39:07, mmenke wrote: > nit: Add blank line between methods. Done. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... content/browser/loader/detached_resource_handler.h:56: } On 2013/10/11 16:39:07, mmenke wrote: > Suggest not inlining this. Done. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... content/browser/loader/detached_resource_handler.h:60: kReadBufSize = 3840 On 2013/10/11 16:39:07, mmenke wrote: > Is there a motivation for making it this size? We generally use 32k buffers. > > I believe network reads correspond to reads we request here (In the > non-SSL/non-SPDY/non-QUIC case), so each read results in a round trip to the > cache thread, among other things. Done. I copied it from the SyncResourceHandler. 32K seems fine with me. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... content/browser/loader/detached_resource_handler.h:61: }; On 2013/10/11 16:39:07, mmenke wrote: > Why not just use static const int? This also doesn't need to be defined or > declared in the header. Done. I was copying SyncResourceHandler. static const int it is. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/de... content/browser/loader/detached_resource_handler.h:65: DISALLOW_COPY_AND_ASSIGN(DetachedResourceHandler); On 2013/10/11 16:39:07, mmenke wrote: > nit: There's generally a blank line before DISALLOW_COPY_AND_ASSIGN. Done. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/re... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_impl.cc:704: return; On 2013/10/11 16:39:07, mmenke wrote: > Should this only protect against messing with the offline_policy_map? Seems > like passing in the ResourceRequestDetails may be important/relevant, though I'm > not sure what it actually does. Done. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_impl.cc:1161: bool ResourceDispatcherHostImpl::DetachableType( On 2013/10/11 16:39:07, mmenke wrote: > Since this is only used in this file and doesn't depend on |this|, suggest > moving it into a private namespace. Done. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_impl.cc:1161: bool ResourceDispatcherHostImpl::DetachableType( On 2013/10/11 16:39:07, mmenke wrote: > Suggest IsDetachableResouceType, or maybe IsResouceTypeDetachable. Done. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/re... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_unittest.cc:2211: EXPECT_EQ(size_t(2), msgs[0].size()); On 2013/10/11 16:39:07, mmenke wrote: > C-style casts aren't allowed. Just use 2u here. Done. Thanks. https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/re... File content/browser/loader/resource_loader.h (right): https://codereview.chromium.org/25772002/diff/54001/content/browser/loader/re... content/browser/loader/resource_loader.h:145: static int default_delay_prefetch_cancel_ms_; On 2013/10/11 16:39:07, mmenke wrote: > This doesn't need to be in the header, and should be const, using the const > naming scheme. Done. Ah right, I changed the meaning of this. Thanks.
How close would we be to DetachedResourceHandler if AsyncResourceHandler just didn't send data back for PREFETCH requests? https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/de... File content/browser/loader/detached_resource_handler.cc (right): https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/de... content/browser/loader/detached_resource_handler.cc:28: static const int kReadBufSize = 3840; Nit: Explain where this comes from? Why is it this value? https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/de... content/browser/loader/detached_resource_handler.cc:54: DCHECK(status.status() != net::URLRequestStatus::IO_PENDING); I'm curious why you are checking this in response to OnResponseCompleted()? https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/de... content/browser/loader/detached_resource_handler.cc:126: // Give the loader something to write its data to, but never reference it. hm? But you are keeping a reference to the buffer until the next OnWillRead() or until the resource handler is destroyed. https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/de... content/browser/loader/detached_resource_handler.cc:127: if (!read_buffer_) { Nit: No braces for single line conditional. https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/de... File content/browser/loader/detached_resource_handler.h (right): https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/de... content/browser/loader/detached_resource_handler.h:25: // handler will continue if the renderer dissappears. Nit: s/ss/s/ https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/re... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_impl.cc:713: // request unless it's been detached. This isn't strictly correct, is it? The entry would only be removed if the associated renderer has died. It's not necessarily true that the entry would be absent if the request is detachable. This check could prevent UpdateStateForSuccessfullyStartedRequest() to be called for an entry that exists. https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_impl.cc:1119: // put in detached handler Nit: Remove comment. https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/re... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_unittest.cc:607: void MakeTestRequest(ResourceMessageFilter* filter, Nit: Can you get rid of MakeTestRequest and resource_type_? https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/re... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/re... content/browser/loader/resource_loader.cc:467: bool ResourceLoader::DelayCancelForPrefetch(const ResourceRequestInfoImpl& info, Can you explain the rationale for this logic? Why is the cancel delayed?
On Fri, Oct 11, 2013 at 5:01 PM, <asanka@chromium.org> wrote: > How close would we be to DetachedResourceHandler if AsyncResourceHandler > just > didn't send data back for PREFETCH requests? > That's exactly what it was a couple of uploads ago, but then I found a few more places I needed to guard, and it started to get a bit hackish. If you feel strongly I can go back to that and remove the new handler. Josh > > > https://codereview.chromium.**org/25772002/diff/62001/** > content/browser/loader/**detached_resource_handler.cc<https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/detached_resource_handler.cc> > File content/browser/loader/**detached_resource_handler.cc (right): > > https://codereview.chromium.**org/25772002/diff/62001/** > content/browser/loader/**detached_resource_handler.cc#**newcode28<https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/detached_resource_handler.cc#newcode28> > content/browser/loader/**detached_resource_handler.cc:**28: static const > int > kReadBufSize = 3840; > Nit: Explain where this comes from? Why is it this value? > > https://codereview.chromium.**org/25772002/diff/62001/** > content/browser/loader/**detached_resource_handler.cc#**newcode54<https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/detached_resource_handler.cc#newcode54> > content/browser/loader/**detached_resource_handler.cc:**54: > DCHECK(status.status() != net::URLRequestStatus::IO_**PENDING); > I'm curious why you are checking this in response to > OnResponseCompleted()? > > https://codereview.chromium.**org/25772002/diff/62001/** > content/browser/loader/**detached_resource_handler.cc#**newcode126<https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/detached_resource_handler.cc#newcode126> > content/browser/loader/**detached_resource_handler.cc:**126: // Give the > loader something to write its data to, but never reference it. > hm? But you are keeping a reference to the buffer until the next > OnWillRead() or until the resource handler is destroyed. > > https://codereview.chromium.**org/25772002/diff/62001/** > content/browser/loader/**detached_resource_handler.cc#**newcode127<https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/detached_resource_handler.cc#newcode127> > content/browser/loader/**detached_resource_handler.cc:**127: if > (!read_buffer_) { > Nit: No braces for single line conditional. > > https://codereview.chromium.**org/25772002/diff/62001/** > content/browser/loader/**detached_resource_handler.h<https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/detached_resource_handler.h> > File content/browser/loader/**detached_resource_handler.h (right): > > https://codereview.chromium.**org/25772002/diff/62001/** > content/browser/loader/**detached_resource_handler.h#**newcode25<https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/detached_resource_handler.h#newcode25> > content/browser/loader/**detached_resource_handler.h:**25: // handler will > continue if the renderer dissappears. > Nit: s/ss/s/ > > https://codereview.chromium.**org/25772002/diff/62001/** > content/browser/loader/**resource_dispatcher_host_impl.**cc<https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/resource_dispatcher_host_impl.cc> > File content/browser/loader/**resource_dispatcher_host_impl.**cc (right): > > https://codereview.chromium.**org/25772002/diff/62001/** > content/browser/loader/**resource_dispatcher_host_impl.**cc#newcode713<https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode713> > content/browser/loader/**resource_dispatcher_host_impl.**cc:713: // > request > unless it's been detached. > This isn't strictly correct, is it? The entry would only be removed if > the associated renderer has died. It's not necessarily true that the > entry would be absent if the request is detachable. This check could > prevent UpdateStateForSuccessfullyStar**tedRequest() to be called for an > entry that exists. > > https://codereview.chromium.**org/25772002/diff/62001/** > content/browser/loader/**resource_dispatcher_host_impl.**cc#newcode1119<https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode1119> > content/browser/loader/**resource_dispatcher_host_impl.**cc:1119: // put > in > detached handler > Nit: Remove comment. > > https://codereview.chromium.**org/25772002/diff/62001/** > content/browser/loader/**resource_dispatcher_host_**unittest.cc<https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/resource_dispatcher_host_unittest.cc> > File content/browser/loader/**resource_dispatcher_host_**unittest.cc > (right): > > https://codereview.chromium.**org/25772002/diff/62001/** > content/browser/loader/**resource_dispatcher_host_**unittest.cc#newcode607<https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/resource_dispatcher_host_unittest.cc#newcode607> > content/browser/loader/**resource_dispatcher_host_**unittest.cc:607: void > MakeTestRequest(**ResourceMessageFilter* filter, > Nit: Can you get rid of MakeTestRequest and resource_type_? > > https://codereview.chromium.**org/25772002/diff/62001/** > content/browser/loader/**resource_loader.cc<https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/resource_loader.cc> > File content/browser/loader/**resource_loader.cc (right): > > https://codereview.chromium.**org/25772002/diff/62001/** > content/browser/loader/**resource_loader.cc#newcode467<https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/resource_loader.cc#newcode467> > content/browser/loader/**resource_loader.cc:467: bool > ResourceLoader::**DelayCancelForPrefetch(const ResourceRequestInfoImpl& > info, > Can you explain the rationale for this logic? Why is the cancel delayed? > > https://codereview.chromium.**org/25772002/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Just one little nit (didn't do full review): I'd prefer not to add a command line switch for this behavior. We should just land this as is since there isn't much need to toggle behavior. If you want to have an easy bit to flip to disable the behavior just create a boolean in the code base.
Just one little nit (didn't do full review): I'd prefer not to add a command line switch for this behavior. We should just land this as is since there isn't much need to toggle behavior. If you want to have an easy bit to flip to disable the behavior just create a boolean in the code base.
cbentzel: In terms of the flag, it'll be useful for field trials to determine a reasonable timeout and to determine its goodness. https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/de... File content/browser/loader/detached_resource_handler.cc (right): https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/de... content/browser/loader/detached_resource_handler.cc:28: static const int kReadBufSize = 3840; On 2013/10/11 21:01:20, asanka wrote: > Nit: Explain where this comes from? Why is it this value? Done. Ah, this was supposed to be fixed earlier. Set to 32K now. https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/de... content/browser/loader/detached_resource_handler.cc:54: DCHECK(status.status() != net::URLRequestStatus::IO_PENDING); On 2013/10/11 21:01:20, asanka wrote: > I'm curious why you are checking this in response to OnResponseCompleted()? Done. Copied from AsyncResourceHandler::OnResponseCompleted. Seemed like if it was a concern there at one point it could be here as well, but I'll take it out. https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/de... content/browser/loader/detached_resource_handler.cc:126: // Give the loader something to write its data to, but never reference it. On 2013/10/11 21:01:20, asanka wrote: > hm? But you are keeping a reference to the buffer until the next OnWillRead() or > until the resource handler is destroyed. Done. https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/de... content/browser/loader/detached_resource_handler.cc:127: if (!read_buffer_) { On 2013/10/11 21:01:20, asanka wrote: > Nit: No braces for single line conditional. Done. https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/de... File content/browser/loader/detached_resource_handler.h (right): https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/de... content/browser/loader/detached_resource_handler.h:25: // handler will continue if the renderer dissappears. On 2013/10/11 21:01:20, asanka wrote: > Nit: s/ss/s/ Done. https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/re... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_impl.cc:713: // request unless it's been detached. On 2013/10/11 21:01:20, asanka wrote: > This isn't strictly correct, is it? The entry would only be removed if the > associated renderer has died. It's not necessarily true that the entry would be > absent if the request is detachable. This check could prevent > UpdateStateForSuccessfullyStartedRequest() to be called for an entry that > exists. Done. https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_impl.cc:1119: // put in detached handler On 2013/10/11 21:01:20, asanka wrote: > Nit: Remove comment. Done. https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/re... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_unittest.cc:607: void MakeTestRequest(ResourceMessageFilter* filter, On 2013/10/11 21:01:20, asanka wrote: > Nit: Can you get rid of MakeTestRequest and resource_type_? Done. https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/re... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/25772002/diff/62001/content/browser/loader/re... content/browser/loader/resource_loader.cc:467: bool ResourceLoader::DelayCancelForPrefetch(const ResourceRequestInfoImpl& info, On 2013/10/11 21:01:20, asanka wrote: > Can you explain the rationale for this logic? Why is the cancel delayed? Done.
Nits only. However, I'm a bit apprehensive about the new resource handler due to code duplication. It seems that now the decision to detach reads could be based on ResourceRequestInfo::is_detached() in AsyncResourceHandler. Would that alleviate the concerns regarding code complexity? Also, +1 to cbentzel's suggestion. I'm also under the impression that additional flags aren't required for field trials. https://codereview.chromium.org/25772002/diff/72001/content/browser/loader/re... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/25772002/diff/72001/content/browser/loader/re... content/browser/loader/resource_loader.cc:476: command_line.HasSwitch(switches::kDelayDetachableCancellation)) { Is it possible to get two cancels from the renderer? So far that would have been "idempotent". But it seems this code would delay one but not the other. https://codereview.chromium.org/25772002/diff/72001/content/browser/loader/re... File content/browser/loader/resource_request_info_impl.h (right): https://codereview.chromium.org/25772002/diff/72001/content/browser/loader/re... content/browser/loader/resource_request_info_impl.h:128: // live beyond the life of the renderer. Nit: In practice this flag means the request is already detached and not just whether the request can be detached. I'd go with is_detached(). https://codereview.chromium.org/25772002/diff/72001/content/public/common/con... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/25772002/diff/72001/content/public/common/con... content/public/common/content_switches.cc:67: // don't immediately drop an in-flight prefetch. Nit: Update comment. Probably want to explicitly mention the <a ping> case.
On 2013/10/14 21:06:49, asanka wrote: > Nits only. > > However, I'm a bit apprehensive about the new resource handler due to code > duplication. It seems that now the decision to detach reads could be based on > ResourceRequestInfo::is_detached() in AsyncResourceHandler. Would that alleviate > the concerns regarding code complexity? I'm also a bit apprehensive about this, though I'm not a big fan of peppering AsyncResourceHandler with tests everywhere to make sure this works - that seems more complicated to me, and about as regression prone. One other option is to layer the new class (through composition) on top of an AsyncResourceHandler, and give it a bogus ResourceController which ignores cancellation messages. Then we just always call into the AsyncResourceHandler, until it tries to cancel, at which point we delete it and stop calling into it. This is complicated, and raises similar worries about regressions as the other two cases - I'd argue it may be slightly less regression prone than the two alternatives, but more complicated. > Also, +1 to cbentzel's suggestion. I'm also under the impression that additional > flags aren't required for field trials. > > https://codereview.chromium.org/25772002/diff/72001/content/browser/loader/re... > File content/browser/loader/resource_loader.cc (right): > > https://codereview.chromium.org/25772002/diff/72001/content/browser/loader/re... > content/browser/loader/resource_loader.cc:476: > command_line.HasSwitch(switches::kDelayDetachableCancellation)) { > Is it possible to get two cancels from the renderer? So far that would have been > "idempotent". But it seems this code would delay one but not the other. > > https://codereview.chromium.org/25772002/diff/72001/content/browser/loader/re... > File content/browser/loader/resource_request_info_impl.h (right): > > https://codereview.chromium.org/25772002/diff/72001/content/browser/loader/re... > content/browser/loader/resource_request_info_impl.h:128: // live beyond the life > of the renderer. > Nit: In practice this flag means the request is already detached and not just > whether the request can be detached. I'd go with is_detached(). > > https://codereview.chromium.org/25772002/diff/72001/content/public/common/con... > File content/public/common/content_switches.cc (right): > > https://codereview.chromium.org/25772002/diff/72001/content/public/common/con... > content/public/common/content_switches.cc:67: // don't immediately drop an > in-flight prefetch. > Nit: Update comment. Probably want to explicitly mention the <a ping> case.
On 2013/10/14 21:22:32, mmenke wrote: > On 2013/10/14 21:06:49, asanka wrote: > > Nits only. > > > > However, I'm a bit apprehensive about the new resource handler due to code > > duplication. It seems that now the decision to detach reads could be based on > > ResourceRequestInfo::is_detached() in AsyncResourceHandler. Would that > alleviate > > the concerns regarding code complexity? > > I'm also a bit apprehensive about this, though I'm not a big fan of peppering > AsyncResourceHandler with tests everywhere to make sure this works - that seems > more complicated to me, and about as regression prone. > > One other option is to layer the new class (through composition) on top of an > AsyncResourceHandler, and give it a bogus ResourceController which ignores > cancellation messages. Then we just always call into the AsyncResourceHandler, > until it tries to cancel, at which point we delete it and stop calling into it. > This is complicated, and raises similar worries about regressions as the other > two cases - I'd argue it may be slightly less regression prone than the two > alternatives, but more complicated. jkarlin: Not suggesting you implement this at the moment, just to be clear. Want to try and come with some consensus as to which is the best alternative to go with.
I can go either way in terms of the new handler vs sprinkling in changes in AsyncResourceHandler. Up to you guys. What's the verdict? Josh On Mon, Oct 14, 2013 at 5:24 PM, <mmenke@chromium.org> wrote: > On 2013/10/14 21:22:32, mmenke wrote: > >> On 2013/10/14 21:06:49, asanka wrote: >> > Nits only. >> > >> > However, I'm a bit apprehensive about the new resource handler due to >> code >> > duplication. It seems that now the decision to detach reads could be >> based >> > on > >> > ResourceRequestInfo::is_**detached() in AsyncResourceHandler. Would >> that >> alleviate >> > the concerns regarding code complexity? >> > > I'm also a bit apprehensive about this, though I'm not a big fan of >> peppering >> AsyncResourceHandler with tests everywhere to make sure this works - that >> > seems > >> more complicated to me, and about as regression prone. >> > > One other option is to layer the new class (through composition) on top >> of an >> AsyncResourceHandler, and give it a bogus ResourceController which ignores >> cancellation messages. Then we just always call into the >> > AsyncResourceHandler, > >> until it tries to cancel, at which point we delete it and stop calling >> into >> > it. > >> This is complicated, and raises similar worries about regressions as the >> other >> two cases - I'd argue it may be slightly less regression prone than the >> two >> alternatives, but more complicated. >> > > jkarlin: Not suggesting you implement this at the moment, just to be > clear. > Want to try and come with some consensus as to which is the best > alternative to > go with. > > https://codereview.chromium.**org/25772002/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/10/15 21:38:45, jkarlin wrote: > I can go either way in terms of the new handler vs sprinkling in changes in > AsyncResourceHandler. Up to you guys. What's the verdict? > > Josh > > > On Mon, Oct 14, 2013 at 5:24 PM, <mailto:mmenke@chromium.org> wrote: > > > On 2013/10/14 21:22:32, mmenke wrote: > > > >> On 2013/10/14 21:06:49, asanka wrote: > >> > Nits only. > >> > > >> > However, I'm a bit apprehensive about the new resource handler due to > >> code > >> > duplication. It seems that now the decision to detach reads could be > >> based > >> > > on > > > >> > ResourceRequestInfo::is_**detached() in AsyncResourceHandler. Would > >> that > >> alleviate > >> > the concerns regarding code complexity? > >> > > > > I'm also a bit apprehensive about this, though I'm not a big fan of > >> peppering > >> AsyncResourceHandler with tests everywhere to make sure this works - that > >> > > seems > > > >> more complicated to me, and about as regression prone. > >> > > > > One other option is to layer the new class (through composition) on top > >> of an > >> AsyncResourceHandler, and give it a bogus ResourceController which ignores > >> cancellation messages. Then we just always call into the > >> > > AsyncResourceHandler, > > > >> until it tries to cancel, at which point we delete it and stop calling > >> into > >> > > it. > > > >> This is complicated, and raises similar worries about regressions as the > >> other > >> two cases - I'd argue it may be slightly less regression prone than the > >> two > >> alternatives, but more complicated. > >> > > > > jkarlin: Not suggesting you implement this at the moment, just to be > > clear. > > Want to try and come with some consensus as to which is the best > > alternative to > > go with. If there's going to be more than a couple of conditionals on is_*_detached() in AsyncResourceHandler, then yeah. I think composition/inheritance could be cleaner. However I feel we shouldn't break the ResourceHandler contract from the perspective of AsyncResourceHandler. I.e. don't depend on omitting calls to OnReadCompleted(). Doing so would make it hard to reason about the behavior of AsyncResourceHandler. Another option is to make AsyncResourceHandler a suitable base class for this purpose and use inheritance. I.e. by separating out the code paths that are involved in buffer sharing into methods that can be overridden. It's still only superficially different from not calling OnReadCompleted(), but would make it possible to preserve the RH contract, but at the same time separating out the overridable behavior. Does the renderer actively use the resource events for a prefetch or <a ping> resource request? Developer tools? > https://codereview.chromium.**org/25772002/%3Chttps://codereview.chromium.org...> > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
I've uploaded a new AsyncResourceHandler with 4 minor changes that supports detached resources. What do you guys think? Josh On Tue, Oct 15, 2013 at 6:16 PM, <asanka@chromium.org> wrote: > On 2013/10/15 21:38:45, jkarlin wrote: > >> I can go either way in terms of the new handler vs sprinkling in changes >> in >> AsyncResourceHandler. Up to you guys. What's the verdict? >> > > Josh >> > > > On Mon, Oct 14, 2013 at 5:24 PM, <mailto:mmenke@chromium.org> wrote: >> > > > On 2013/10/14 21:22:32, mmenke wrote: >> > >> >> On 2013/10/14 21:06:49, asanka wrote: >> >> > Nits only. >> >> > >> >> > However, I'm a bit apprehensive about the new resource handler due to >> >> code >> >> > duplication. It seems that now the decision to detach reads could be >> >> based >> >> >> > on >> > >> >> > ResourceRequestInfo::is_****detached() in AsyncResourceHandler. >> Would >> >> >> that >> >> alleviate >> >> > the concerns regarding code complexity? >> >> >> > >> > I'm also a bit apprehensive about this, though I'm not a big fan of >> >> peppering >> >> AsyncResourceHandler with tests everywhere to make sure this works - >> that >> >> >> > seems >> > >> >> more complicated to me, and about as regression prone. >> >> >> > >> > One other option is to layer the new class (through composition) on top >> >> of an >> >> AsyncResourceHandler, and give it a bogus ResourceController which >> ignores >> >> cancellation messages. Then we just always call into the >> >> >> > AsyncResourceHandler, >> > >> >> until it tries to cancel, at which point we delete it and stop calling >> >> into >> >> >> > it. >> > >> >> This is complicated, and raises similar worries about regressions as >> the >> >> other >> >> two cases - I'd argue it may be slightly less regression prone than the >> >> two >> >> alternatives, but more complicated. >> >> >> > >> > jkarlin: Not suggesting you implement this at the moment, just to be >> > clear. >> > Want to try and come with some consensus as to which is the best >> > alternative to >> > go with. >> > > If there's going to be more than a couple of conditionals on > is_*_detached() in > AsyncResourceHandler, then yeah. I think composition/inheritance could be > cleaner. However I feel we shouldn't break the ResourceHandler contract > from the > perspective of AsyncResourceHandler. I.e. don't depend on omitting calls to > OnReadCompleted(). Doing so would make it hard to reason about the > behavior of > AsyncResourceHandler. > > Another option is to make AsyncResourceHandler a suitable base class for > this > purpose and use inheritance. I.e. by separating out the code paths that are > involved in buffer sharing into methods that can be overridden. It's still > only > superficially different from not calling OnReadCompleted(), but would make > it > possible to preserve the RH contract, but at the same time separating out > the > overridable behavior. > > Does the renderer actively use the resource events for a prefetch or <a > ping> > resource request? Developer tools? > > > https://codereview.chromium.****org/25772002/%3Chttps://codere** > view.chromium.org/25772002/ <http://codereview.chromium.org/25772002/>> > >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+**unsubscribe@chromium.org<chromium-reviews%2Bunsubscribe@chromium.org> >> . >> > > > https://codereview.chromium.**org/25772002/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Matt: WDYT? The code is okay, but I see the problem with maintenance overhead: any change to AsyncResourceHandler would need to be made with the consideration that it could be used with a detached request. https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/as... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/as... content/browser/loader/async_resource_handler.cc:141: return false; Should this also return info->is_detachable()? <a ping> uses POST, right?
On 2013/10/16 20:42:36, asanka wrote: > Matt: WDYT? The code is okay, but I see the problem with maintenance overhead: > any change to AsyncResourceHandler would need to be made with the consideration > that it could be used with a detached request. I'm fine with any of the three approaches. The new (non-layered) ResourceHandler approach does raise serious concerns about code duplication, so I now think either layering on top of the default one, or this approach are less likely to break things badly (Sorry for having you run around in circles, Josh). I do think the current code is quite hard to follow, so I'll be suggesting some cleanups/comments. I think we could use a test that redirects and a test that does a post (With upload data). Doesn't look to me like any other case stands out as needing to be tested, except we should have a test that makes sure that prefetches normally do send data back to the renderer if it's not closed, if we don't have such a test already. Could optionally also have a failed URL request, though it doesn't look like the ResourceHandler cares much about that.
Meant to get these to you earlier. https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/as... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/as... content/browser/loader/async_resource_handler.cc:141: return false; On 2013/10/16 20:42:37, asanka wrote: > Should this also return info->is_detachable()? > > <a ping> uses POST, right? Yea, <a ping>'s are POSTs with 4-character bodies. https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/as... content/browser/loader/async_resource_handler.cc:153: return info->is_detachable(); I think all of these need a comment along the lines of "If detachable, just continue with the request. Otherwise, cancel it." https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/as... content/browser/loader/async_resource_handler.cc:252: if (info->is_detachable()) { Again, comment that renderers don't care about reads from detachable requests (Does devtools care?) https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/as... content/browser/loader/async_resource_handler.cc:319: return info->is_detachable(); Hmm....Wonder about returning false in this case in the first place. https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/de... File content/browser/loader/detached_resource_handler.h (right): https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/de... content/browser/loader/detached_resource_handler.h:6: #define CONTENT_BROWSER_LOADER_DETACHED_RESOURCE_HANDLER_H_ No longer need these two files. https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/re... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_impl.cc:35: #include "content/browser/loader/detached_resource_handler.h" Remove. https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_impl.cc:427: (*i)->GetRequestInfo()->is_detachable() || Wait...what's stopping us from putting detachable requests in this list? https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_impl.cc:1123: nit: Remove extra line break. https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_impl.cc:1366: if (!info->is_download() && !info->is_stream() && !info->is_detachable() && We aren't calling into DelayCancelForDetachable here, are we? So we don't start the timer, and don't listen to the command line flag? Think we need more testing of this. https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/re... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/re... content/browser/loader/resource_loader.cc:71: static const int kDefaultDelayDetachableCancelMs = 60000; nit: Just remove the static and move this up into the namespace above. https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/re... content/browser/loader/resource_loader.cc:468: const ResourceRequestInfoImpl& info, bool from_renderer) { If a profile, along with its network stack, are deleted...How are these destroyed before the profile's URLRequest context? https://codereview.chromium.org/25772002/diff/99001/content/public/common/con... File content/public/common/content_switches.h (right): https://codereview.chromium.org/25772002/diff/99001/content/public/common/con... content/public/common/content_switches.h:33: CONTENT_EXPORT extern const char kDelayDetachableCancellation[]; We don't need a switch for a field trial.
Okay, I'm going to go with the AsyncResourceHandler updates, add some tests, and clean everything up. Josh On Thu, Oct 17, 2013 at 2:33 PM, <mmenke@chromium.org> wrote: > Meant to get these to you earlier. > > > > https://codereview.chromium.**org/25772002/diff/99001/** > content/browser/loader/async_**resource_handler.cc<https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/async_resource_handler.cc> > File content/browser/loader/async_**resource_handler.cc (right): > > https://codereview.chromium.**org/25772002/diff/99001/** > content/browser/loader/async_**resource_handler.cc#newcode141<https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/async_resource_handler.cc#newcode141> > content/browser/loader/async_**resource_handler.cc:141: return false; > On 2013/10/16 20:42:37, asanka wrote: > >> Should this also return info->is_detachable()? >> > > <a ping> uses POST, right? >> > > Yea, <a ping>'s are POSTs with 4-character bodies. > > https://codereview.chromium.**org/25772002/diff/99001/** > content/browser/loader/async_**resource_handler.cc#newcode153<https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/async_resource_handler.cc#newcode153> > content/browser/loader/async_**resource_handler.cc:153: return > info->is_detachable(); > I think all of these need a comment along the lines of "If detachable, > just continue with the request. Otherwise, cancel it." > > https://codereview.chromium.**org/25772002/diff/99001/** > content/browser/loader/async_**resource_handler.cc#newcode252<https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/async_resource_handler.cc#newcode252> > content/browser/loader/async_**resource_handler.cc:252: if > (info->is_detachable()) { > Again, comment that renderers don't care about reads from detachable > requests (Does devtools care?) > > https://codereview.chromium.**org/25772002/diff/99001/** > content/browser/loader/async_**resource_handler.cc#newcode319<https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/async_resource_handler.cc#newcode319> > content/browser/loader/async_**resource_handler.cc:319: return > info->is_detachable(); > Hmm....Wonder about returning false in this case in the first place. > > https://codereview.chromium.**org/25772002/diff/99001/** > content/browser/loader/**detached_resource_handler.h<https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/detached_resource_handler.h> > File content/browser/loader/**detached_resource_handler.h (right): > > https://codereview.chromium.**org/25772002/diff/99001/** > content/browser/loader/**detached_resource_handler.h#**newcode6<https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/detached_resource_handler.h#newcode6> > content/browser/loader/**detached_resource_handler.h:6: #define > CONTENT_BROWSER_LOADER_**DETACHED_RESOURCE_HANDLER_H_ > No longer need these two files. > > https://codereview.chromium.**org/25772002/diff/99001/** > content/browser/loader/**resource_dispatcher_host_impl.**cc<https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/resource_dispatcher_host_impl.cc> > File content/browser/loader/**resource_dispatcher_host_impl.**cc (right): > > https://codereview.chromium.**org/25772002/diff/99001/** > content/browser/loader/**resource_dispatcher_host_impl.**cc#newcode35<https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode35> > content/browser/loader/**resource_dispatcher_host_impl.**cc:35: #include > "content/browser/loader/**detached_resource_handler.h" > Remove. > > https://codereview.chromium.**org/25772002/diff/99001/** > content/browser/loader/**resource_dispatcher_host_impl.**cc#newcode427<https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode427> > content/browser/loader/**resource_dispatcher_host_impl.**cc:427: > (*i)->GetRequestInfo()->is_**detachable() || > Wait...what's stopping us from putting detachable requests in this list? > > https://codereview.chromium.**org/25772002/diff/99001/** > content/browser/loader/**resource_dispatcher_host_impl.**cc#newcode1123<https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode1123> > content/browser/loader/**resource_dispatcher_host_impl.**cc:1123: > nit: Remove extra line break. > > https://codereview.chromium.**org/25772002/diff/99001/** > content/browser/loader/**resource_dispatcher_host_impl.**cc#newcode1366<https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode1366> > content/browser/loader/**resource_dispatcher_host_impl.**cc:1366: if > (!info->is_download() && !info->is_stream() && !info->is_detachable() && > We aren't calling into DelayCancelForDetachable here, are we? So we > don't start the timer, and don't listen to the command line flag? Think > we need more testing of this. > > https://codereview.chromium.**org/25772002/diff/99001/** > content/browser/loader/**resource_loader.cc<https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/resource_loader.cc> > File content/browser/loader/**resource_loader.cc (right): > > https://codereview.chromium.**org/25772002/diff/99001/** > content/browser/loader/**resource_loader.cc#newcode71<https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/resource_loader.cc#newcode71> > content/browser/loader/**resource_loader.cc:71: static const int > kDefaultDelayDetachableCancelM**s = 60000; > nit: Just remove the static and move this up into the namespace above. > > https://codereview.chromium.**org/25772002/diff/99001/** > content/browser/loader/**resource_loader.cc#newcode468<https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/resource_loader.cc#newcode468> > content/browser/loader/**resource_loader.cc:468: const > ResourceRequestInfoImpl& info, bool from_renderer) { > If a profile, along with its network stack, are deleted...How are these > destroyed before the profile's URLRequest context? > > https://codereview.chromium.**org/25772002/diff/99001/** > content/public/common/content_**switches.h<https://codereview.chromium.org/25772002/diff/99001/content/public/common/content_switches.h> > File content/public/common/content_**switches.h (right): > > https://codereview.chromium.**org/25772002/diff/99001/** > content/public/common/content_**switches.h#newcode33<https://codereview.chromium.org/25772002/diff/99001/content/public/common/content_switches.h#newcode33> > content/public/common/content_**switches.h:33: CONTENT_EXPORT extern const > char kDelayDetachableCancellation[]**; > We don't need a switch for a field trial. > > https://codereview.chromium.**org/25772002/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/as... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/as... content/browser/loader/async_resource_handler.cc:141: return false; On 2013/10/16 20:42:37, asanka wrote: > Should this also return info->is_detachable()? > > <a ping> uses POST, right? Done. https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/as... content/browser/loader/async_resource_handler.cc:141: return false; On 2013/10/17 18:33:48, mmenke wrote: > On 2013/10/16 20:42:37, asanka wrote: > > Should this also return info->is_detachable()? > > > > <a ping> uses POST, right? > > Yea, <a ping>'s are POSTs with 4-character bodies. Done. https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/as... content/browser/loader/async_resource_handler.cc:153: return info->is_detachable(); On 2013/10/17 18:33:48, mmenke wrote: > I think all of these need a comment along the lines of "If detachable, just > continue with the request. Otherwise, cancel it." Done. https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/as... content/browser/loader/async_resource_handler.cc:252: if (info->is_detachable()) { On 2013/10/17 18:33:48, mmenke wrote: > Again, comment that renderers don't care about reads from detachable requests > (Does devtools care?) Done. https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/as... content/browser/loader/async_resource_handler.cc:319: return info->is_detachable(); On 2013/10/17 18:33:48, mmenke wrote: > Hmm....Wonder about returning false in this case in the first place. Noted. It's unclear to me. The loader seems to interpret false as a need to jump into a DEFERRED_FINISH state. I want to change as little existing behavior as possible in this CL though so will leave the same. https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/de... File content/browser/loader/detached_resource_handler.h (right): https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/de... content/browser/loader/detached_resource_handler.h:6: #define CONTENT_BROWSER_LOADER_DETACHED_RESOURCE_HANDLER_H_ On 2013/10/17 18:33:48, mmenke wrote: > No longer need these two files. Done. https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/re... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_impl.cc:35: #include "content/browser/loader/detached_resource_handler.h" On 2013/10/17 18:33:48, mmenke wrote: > Remove. Done. https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_impl.cc:427: (*i)->GetRequestInfo()->is_detachable() || I don't understand, they are in the list? On 2013/10/17 18:33:48, mmenke wrote: > Wait...what's stopping us from putting detachable requests in this list? https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_impl.cc:1123: On 2013/10/17 18:33:48, mmenke wrote: > nit: Remove extra line break. Done. https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_impl.cc:1366: if (!info->is_download() && !info->is_stream() && !info->is_detachable() && On 2013/10/17 18:33:48, mmenke wrote: > We aren't calling into DelayCancelForDetachable here, are we? So we don't start > the timer, and don't listen to the command line flag? Think we need more > testing of this. Done. Great catch. Fixed and tests added. https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/re... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/re... content/browser/loader/resource_loader.cc:71: static const int kDefaultDelayDetachableCancelMs = 60000; On 2013/10/17 18:33:48, mmenke wrote: > nit: Just remove the static and move this up into the namespace above. I needed a settable delay for testing so I removed this and added a member variable. https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/re... content/browser/loader/resource_loader.cc:468: const ResourceRequestInfoImpl& info, bool from_renderer) { On 2013/10/17 18:33:48, mmenke wrote: > If a profile, along with its network stack, are deleted...How are these > destroyed before the profile's URLRequest context? ResourceContext::~ResourceContext() calls ResourceDispatcherHostImpl::CancelRequestsForContext I've added a test to verify that CancelRequestsForContext does immediately kill detachable requests. https://codereview.chromium.org/25772002/diff/99001/content/public/common/con... File content/public/common/content_switches.h (right): https://codereview.chromium.org/25772002/diff/99001/content/public/common/con... content/public/common/content_switches.h:33: CONTENT_EXPORT extern const char kDelayDetachableCancellation[]; On 2013/10/17 18:33:48, mmenke wrote: > We don't need a switch for a field trial. Done.
https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/re... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_impl.cc:427: (*i)->GetRequestInfo()->is_detachable() || On 2013/10/24 15:33:11, jkarlin wrote: > I don't understand, they are in the list? Sorry...What's stopping requests from being in the loaders_to_cancel list? https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_impl.cc:130: bool IsDetachableResourceType(const ResourceType::Type type) { nit: const not needed. https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_impl.cc:717: } else if (!info->is_detachable()) { optional nit: I suggest: } else { ... DCHECK(info->is_detachable()); } Think it's a little simpler. https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:826: const std::string& reference_data) { nit: Fix indentation https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:835: void CheckSuccessfulDetachableRequest( Maybe DetachedRequest instead? https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:972: host_.CancelRequest(filter_->child_id(), 1, true); Question: Should we consider timing out all prefetch requests after X seconds after start (Possibly for a larger X), rather than just doing so on cancel / RenderView destruction? Otherwise, still have the potential cache blocking problem when opening in another window/tab. https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:1305: TEST_F(ResourceDispatcherHostTest, TestProcessCancelDetachableTimesOut) { None of these tests exercises the cancel+timeout in CancelRequestsForRoute. Can we test that case? https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:1936: host_.OnMessageReceived(msg, filter_.get(), &msg_was_ok); Should the other tests cancel this way, rather than calling host_.CancelRequest? https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... content/browser/loader/resource_loader.cc:215: detachable_delay_ms_ = 60000; Wonder if we should reduce this... Regardless, this should be #defined as a constant at the top of the file. KDetaultDetachableDelayMs or somesuch. May also want to rename both variable and value to be more precise - detached_delay_on_cancel_ms_.
https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/re... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_impl.cc:427: (*i)->GetRequestInfo()->is_detachable() || At this point the renderers (and their attached requests) for the context should have already disappeared, so all that we're left with to clean up are detached requests, like downloads and detachable resources. On 2013/10/24 20:27:20, mmenke wrote: > On 2013/10/24 15:33:11, jkarlin wrote: > > I don't understand, they are in the list? > > Sorry...What's stopping requests from being in the loaders_to_cancel list? https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_impl.cc:130: bool IsDetachableResourceType(const ResourceType::Type type) { On 2013/10/24 20:27:21, mmenke wrote: > nit: const not needed. Done. https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_impl.cc:717: } else if (!info->is_detachable()) { On 2013/10/24 20:27:21, mmenke wrote: > optional nit: I suggest: > > } else { > ... > DCHECK(info->is_detachable()); > } > > Think it's a little simpler. Done. https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:826: const std::string& reference_data) { On 2013/10/24 20:27:21, mmenke wrote: > nit: Fix indentation Done. https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:835: void CheckSuccessfulDetachableRequest( On 2013/10/24 20:27:21, mmenke wrote: > Maybe DetachedRequest instead? Done. https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:972: host_.CancelRequest(filter_->child_id(), 1, true); That's an important issue that we need to address, but I don't think that this is the CL for it. I think the long-term resolution is to not block, or at least not block for long. On 2013/10/24 20:27:21, mmenke wrote: > Question: Should we consider timing out all prefetch requests after X seconds > after start (Possibly for a larger X), rather than just doing so on cancel / > RenderView destruction? Otherwise, still have the potential cache blocking > problem when opening in another window/tab. https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:1305: TEST_F(ResourceDispatcherHostTest, TestProcessCancelDetachableTimesOut) { CancelRequestsForProcess calls CancelRequestsForRoute. But if you'd like it to be more direct, I can add another test just for CancelRequestsForRoute. On 2013/10/24 20:27:21, mmenke wrote: > None of these tests exercises the cancel+timeout in CancelRequestsForRoute. Can > we test that case? https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:1936: host_.OnMessageReceived(msg, filter_.get(), &msg_was_ok); On 2013/10/24 20:27:21, mmenke wrote: > Should the other tests cancel this way, rather than calling host_.CancelRequest? Done. https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... content/browser/loader/resource_loader.cc:215: detachable_delay_ms_ = 60000; On 2013/10/24 20:27:21, mmenke wrote: > Wonder if we should reduce this... Regardless, this should be #defined as a > constant at the top of the file. KDetaultDetachableDelayMs or somesuch. > > May also want to rename both variable and value to be more precise - > detached_delay_on_cancel_ms_. Done. Yes, 60 seconds is long. It's just until we get a feel for what value it should be, and until we stop blocking subsequent requests for the same resource. I've added a comment on this to the new const (but it hasn't been uploaded yet).
LGTM. Make sure you get the signoff of someone more familiar with this stuff than I am (asanka). https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/re... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/25772002/diff/99001/content/browser/loader/re... content/browser/loader/resource_dispatcher_host_impl.cc:427: (*i)->GetRequestInfo()->is_detachable() || On 2013/10/25 14:10:21, jkarlin wrote: > At this point the renderers (and their attached requests) for the context should > have already disappeared, so all that we're left with to clean up are detached > requests, like downloads and detachable resources. > > On 2013/10/24 20:27:20, mmenke wrote: > > On 2013/10/24 15:33:11, jkarlin wrote: > > > I don't understand, they are in the list? > > > > Sorry...What's stopping requests from being in the loaders_to_cancel list? > Ah, right...I was misreading the code (I thought we were DCHECKing if this wasn't true, not if it was). https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:972: host_.CancelRequest(filter_->child_id(), 1, true); On 2013/10/25 14:10:21, jkarlin wrote: > That's an important issue that we need to address, but I don't think that this > is the CL for it. I think the long-term resolution is to not block, or at least > not block for long. > > On 2013/10/24 20:27:21, mmenke wrote: > > Question: Should we consider timing out all prefetch requests after X seconds > > after start (Possibly for a larger X), rather than just doing so on cancel / > > RenderView destruction? Otherwise, still have the potential cache blocking > > problem when opening in another window/tab. > I completely agree that it doesn't belong in this CL. https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:1305: TEST_F(ResourceDispatcherHostTest, TestProcessCancelDetachableTimesOut) { On 2013/10/25 14:10:21, jkarlin wrote: > CancelRequestsForProcess calls CancelRequestsForRoute. But if you'd like it to > be more direct, I can add another test just for CancelRequestsForRoute. Ah, I'm fine with that then. https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... content/browser/loader/resource_loader.cc:215: detachable_delay_ms_ = 60000; On 2013/10/25 14:10:21, jkarlin wrote: > On 2013/10/24 20:27:21, mmenke wrote: > > Wonder if we should reduce this... Regardless, this should be #defined as a > > constant at the top of the file. KDetaultDetachableDelayMs or somesuch. > > > > May also want to rename both variable and value to be more precise - > > detached_delay_on_cancel_ms_. > > Done. Yes, 60 seconds is long. It's just until we get a feel for what value it > should be, and until we stop blocking subsequent requests for the same resource. > I've added a comment on this to the new const (but it hasn't been uploaded > yet). I suggest starting short (Like the current behavior, which deletes it instantly) and then experimenting with lengthening it, rather than the other way around, as that has less chance of breaking things badly. We'll probably want to use a different timeout for <a ping>. https://codereview.chromium.org/25772002/diff/192001/content/browser/loader/r... File content/browser/loader/resource_loader.h (right): https://codereview.chromium.org/25772002/diff/192001/content/browser/loader/r... content/browser/loader/resource_loader.h:61: void set_detachable_delay_ms(int delay) { nit: set_detachable_delay_on_cancel_ms.
https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:1305: TEST_F(ResourceDispatcherHostTest, TestProcessCancelDetachableTimesOut) { On 2013/10/25 14:57:51, mmenke wrote: > On 2013/10/25 14:10:21, jkarlin wrote: > > CancelRequestsForProcess calls CancelRequestsForRoute. But if you'd like it > to > > be more direct, I can add another test just for CancelRequestsForRoute. > > Ah, I'm fine with that then. "Fine with that" meaning "Fine with it as it is"
https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... content/browser/loader/resource_loader.cc:215: detachable_delay_ms_ = 60000; I originally had it at seconds. cbentzel had recommended high so that: 1) We can find out what a good short measurement is. 2) We are unlikely to wind up fetching twice as it should load in 60 seconds. You're right in that we make the worst case 60 seconds worse, which is certainly bad. I can go either way. Thoughts from the crowd? On 2013/10/25 14:57:51, mmenke wrote: > On 2013/10/25 14:10:21, jkarlin wrote: > > On 2013/10/24 20:27:21, mmenke wrote: > > > Wonder if we should reduce this... Regardless, this should be #defined as a > > > constant at the top of the file. KDetaultDetachableDelayMs or somesuch. > > > > > > May also want to rename both variable and value to be more precise - > > > detached_delay_on_cancel_ms_. > > > > Done. Yes, 60 seconds is long. It's just until we get a feel for what value > it > > should be, and until we stop blocking subsequent requests for the same > resource. > > I've added a comment on this to the new const (but it hasn't been uploaded > > yet). > > I suggest starting short (Like the current behavior, which deletes it instantly) > and then experimenting with lengthening it, rather than the other way around, as > that has less chance of breaking things badly. We'll probably want to use a > different timeout for <a ping>.
https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/25772002/diff/112001/content/browser/loader/r... content/browser/loader/resource_loader.cc:215: detachable_delay_ms_ = 60000; I meant to say that I originally had it at 3 seconds. On 2013/10/25 18:24:54, jkarlin wrote: > I originally had it at seconds. > > cbentzel had recommended high so that: > 1) We can find out what a good short measurement is. > 2) We are unlikely to wind up fetching twice as it should load in 60 seconds. > > You're right in that we make the worst case 60 seconds worse, which is certainly > bad. > > I can go either way. Thoughts from the crowd? > > > On 2013/10/25 14:57:51, mmenke wrote: > > On 2013/10/25 14:10:21, jkarlin wrote: > > > On 2013/10/24 20:27:21, mmenke wrote: > > > > Wonder if we should reduce this... Regardless, this should be #defined as > a > > > > constant at the top of the file. KDetaultDetachableDelayMs or somesuch. > > > > > > > > May also want to rename both variable and value to be more precise - > > > > detached_delay_on_cancel_ms_. > > > > > > Done. Yes, 60 seconds is long. It's just until we get a feel for what > value > > it > > > should be, and until we stop blocking subsequent requests for the same > > resource. > > > I've added a comment on this to the new const (but it hasn't been uploaded > > > yet). > > > > I suggest starting short (Like the current behavior, which deletes it > instantly) > > and then experimenting with lengthening it, rather than the other way around, > as > > that has less chance of breaking things badly. We'll probably want to use a > > different timeout for <a ping>. >
https://codereview.chromium.org/25772002/diff/192001/content/browser/loader/a... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/25772002/diff/192001/content/browser/loader/a... content/browser/loader/async_resource_handler.cc:255: // Don't send any data for detachable requests. On face value, this doesn't sound right. It seems like this should be for _detached_ requests. Why don't you want to populate the MemoryCache? https://codereview.chromium.org/25772002/diff/192001/content/browser/loader/r... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/25772002/diff/192001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_impl.cc:1362: if (IsTransferredNavigation(id)) any_requests_transferring = true; nit: Split this into 2 lines. This file doesn't use this style. https://codereview.chromium.org/25772002/diff/192001/content/browser/loader/r... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/25772002/diff/192001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:969: EXPECT_TRUE(loader); Probably better to ASSERT. You'll crash if it's false.
There is now a distinct "detached" state and the AsyncResourceHandler acts normally for detachable resources until detached, at which point messages are no longer sent. This changed the unit tests a little bit. https://codereview.chromium.org/25772002/diff/192001/content/browser/loader/a... File content/browser/loader/async_resource_handler.cc (right): https://codereview.chromium.org/25772002/diff/192001/content/browser/loader/a... content/browser/loader/async_resource_handler.cc:255: // Don't send any data for detachable requests. On 2013/10/26 02:25:00, James Simonsen wrote: > On face value, this doesn't sound right. It seems like this should be for > _detached_ requests. Why don't you want to populate the MemoryCache? Done. There is now a distinct "detached" state and the AsyncResourceHandler acts normally for detachable resources until detached, at which point messages are no longer sent. This changed the unit tests a little bit. https://codereview.chromium.org/25772002/diff/192001/content/browser/loader/r... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/25772002/diff/192001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_impl.cc:1362: if (IsTransferredNavigation(id)) any_requests_transferring = true; On 2013/10/26 02:25:00, James Simonsen wrote: > nit: Split this into 2 lines. This file doesn't use this style. Done. https://codereview.chromium.org/25772002/diff/192001/content/browser/loader/r... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/25772002/diff/192001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:969: EXPECT_TRUE(loader); On 2013/10/26 02:25:00, James Simonsen wrote: > Probably better to ASSERT. You'll crash if it's false. Done.
lgtm https://codereview.chromium.org/25772002/diff/372001/content/browser/loader/r... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/25772002/diff/372001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:2388: ASSERT_LT(0, data_length); These should probably be EXPECTs. Basically, ASSERT means the test code is going to crash unless this is true, EXPECT means the test failed. https://codereview.chromium.org/25772002/diff/372001/content/browser/loader/r... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/25772002/diff/372001/content/browser/loader/r... content/browser/loader/resource_loader.cc:45: // same resource (see bugs 46104 and 31014). I think the worst case scenario here is that the other renderer is hung. The request will stay open, but the buffer will always be full and it'll stop reading. This request will then hang for the full duration. The real way out here is fixing the underlying problem and allow the cache entry to complete even if the renderer isn't reading. It might be a perf win, especially if the renderer is busy.
https://codereview.chromium.org/25772002/diff/372001/content/browser/loader/r... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/25772002/diff/372001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_unittest.cc:2388: ASSERT_LT(0, data_length); On 2013/10/28 21:16:40, James Simonsen wrote: > These should probably be EXPECTs. Basically, ASSERT means the test code is going > to crash unless this is true, EXPECT means the test failed. Done. https://codereview.chromium.org/25772002/diff/372001/content/browser/loader/r... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/25772002/diff/372001/content/browser/loader/r... content/browser/loader/resource_loader.cc:45: // same resource (see bugs 46104 and 31014). I agree that the second reader should be able to drive the cache entry if the first is hung/throttled. I also think that the second request should be able to read what's available from the cache and use it if it's cacheable. davidben and I are starting work on this problem. In the meanwhile, do you prefer the 60 second value or a short one (like 3 seconds)? On 2013/10/28 21:16:40, James Simonsen wrote: > I think the worst case scenario here is that the other renderer is hung. The > request will stay open, but the buffer will always be full and it'll stop > reading. This request will then hang for the full duration. > > The real way out here is fixing the underlying problem and allow the cache entry > to complete even if the renderer isn't reading. It might be a perf win, > especially if the renderer is busy.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jkarlin@google.com/25772002/552001
Failed to apply patch for content/browser/loader/async_resource_handler.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file content/browser/loader/async_resource_handler.cc
Hunk #1 FAILED at 137.
Hunk #2 FAILED at 149.
Hunk #3 FAILED at 177.
Hunk #4 FAILED at 249.
Hunk #5 FAILED at 311.
5 out of 5 hunks FAILED -- saving rejects to file
content/browser/loader/async_resource_handler.cc.rej
Patch: content/browser/loader/async_resource_handler.cc
Index: content/browser/loader/async_resource_handler.cc
diff --git a/content/browser/loader/async_resource_handler.cc
b/content/browser/loader/async_resource_handler.cc
index
f007a417fb8ffe997e20c439506d9f8a6834c367..34d77f24b290282920eddf3440bd0caa692718d4
100644
--- a/content/browser/loader/async_resource_handler.cc
+++ b/content/browser/loader/async_resource_handler.cc
@@ -137,8 +137,9 @@ bool AsyncResourceHandler::OnUploadProgress(int request_id,
uint64 size) {
const ResourceRequestInfoImpl* info =
ResourceRequestInfoImpl::ForRequest(request_);
+ // Cancel the request if the renderer is gone unless it's detachable.
if (!info->filter())
- return false;
+ return info->is_detached();
return info->filter()->Send(
new ResourceMsg_UploadProgress(request_id, position, size));
}
@@ -149,8 +150,9 @@ bool AsyncResourceHandler::OnRequestRedirected(int
request_id,
bool* defer) {
const ResourceRequestInfoImpl* info =
ResourceRequestInfoImpl::ForRequest(request_);
+ // Cancel the request if the renderer is gone unless it's detached.
if (!info->filter())
- return false;
+ return info->is_detached();
*defer = did_defer_ = true;
@@ -177,8 +179,9 @@ bool AsyncResourceHandler::OnResponseStarted(int request_id,
const ResourceRequestInfoImpl* info =
ResourceRequestInfoImpl::ForRequest(request_);
+ // Cancel the request if the renderer is gone unless it's detachable.
if (!info->filter())
- return false;
+ return info->is_detached();
if (rdh_->delegate()) {
rdh_->delegate()->OnResponseStarted(
@@ -249,8 +252,18 @@ bool AsyncResourceHandler::OnReadCompleted(int request_id,
int bytes_read,
const ResourceRequestInfoImpl* info =
ResourceRequestInfoImpl::ForRequest(request_);
- if (!info->filter())
+
+ // Don't send any data if the resource is detached from the renderer.
+ if (info->is_detached()) {
+ buffer_->RecycleLeastRecentlyAllocated();
+ return true;
+ }
+
+ // Cancel the request if the renderer is gone.
+ if (!info->filter()) {
+ DCHECK(!info->is_detachable());
return false;
+ }
buffer_->ShrinkLastAllocation(bytes_read);
@@ -311,8 +324,9 @@ bool AsyncResourceHandler::OnResponseCompleted(
const std::string& security_info) {
const ResourceRequestInfoImpl* info =
ResourceRequestInfoImpl::ForRequest(request_);
+ // Cancel the request if the renderer is gone unless it's detachable.
if (!info->filter())
- return false;
+ return info->is_detachable();
// If we crash here, figure out what URL the renderer was requesting.
// http://crbug.com/107692
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jkarlin@google.com/25772002/592001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jkarlin@google.com/25772002/592001
Failed to apply patch for content/browser/loader/resource_loader.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file content/browser/loader/resource_loader.cc
Hunk #3 FAILED at 219.
Hunk #4 succeeded at 438 (offset -29 lines).
Hunk #5 succeeded at 461 (offset -29 lines).
1 out of 5 hunks FAILED -- saving rejects to file
content/browser/loader/resource_loader.cc.rej
Patch: content/browser/loader/resource_loader.cc
Index: content/browser/loader/resource_loader.cc
diff --git a/content/browser/loader/resource_loader.cc
b/content/browser/loader/resource_loader.cc
index
55add725731dd2aa3a0e1267c61ac58c3f239fb1..7dc42d6638240c9b739d75500a1540028ad5def0
100644
--- a/content/browser/loader/resource_loader.cc
+++ b/content/browser/loader/resource_loader.cc
@@ -7,7 +7,9 @@
#include "base/command_line.h"
#include "base/message_loop/message_loop.h"
#include "base/metrics/histogram.h"
+#include "base/strings/string_number_conversions.h"
#include "base/time/time.h"
+#include "base/timer/timer.h"
#include "content/browser/child_process_security_policy_impl.h"
#include "content/browser/loader/cross_site_resource_handler.h"
#include "content/browser/loader/resource_loader_delegate.h"
@@ -36,6 +38,13 @@ using base::TimeTicks;
namespace content {
namespace {
+// TODO(jkarlin): The value is high to reduce the chance of the detachable
+// request timing out, forcing a blocked second request to open a new
connection
+// and start over. Reduce this value once we have a better idea of what it
+// should be and once we stop blocking multiple simultaneous requests for the
+// same resource (see bugs 46104 and 31014).
+const int kDefaultDetachableDelayOnCancelMs = 30000;
+
void PopulateResourceResponse(net::URLRequest* request,
ResourceResponse* response) {
response->head.error_code = request->status().error();
@@ -210,6 +219,7 @@ void ResourceLoader::Init(scoped_ptr<net::URLRequest>
request,
waiting_for_upload_progress_ack_ = false;
is_transferring_ = false;
client_cert_store_ = client_cert_store.Pass();
+ detachable_delay_on_cancel_ms_ = kDefaultDetachableDelayOnCancelMs;
request_->set_delegate(this);
handler_->SetController(this);
@@ -458,6 +468,18 @@ void ResourceLoader::StartRequestInternal() {
delegate_->DidStartRequest(this);
}
+void ResourceLoader::Detach() {
+ ResourceRequestInfoImpl* info = GetRequestInfo();
+
+ if (info->is_detached())
+ return;
+ info->set_detached();
+ detached_timer_.reset(new base::OneShotTimer<ResourceLoader>());
+ detached_timer_->Start(
+ FROM_HERE, TimeDelta::FromMilliseconds(detachable_delay_on_cancel_ms_),
+ this, &ResourceLoader::Cancel);
+}
+
void ResourceLoader::CancelRequestInternal(int error, bool from_renderer) {
VLOG(1) << "CancelRequestInternal: " << request_->url().spec();
@@ -469,6 +491,11 @@ void ResourceLoader::CancelRequestInternal(int error, bool
from_renderer) {
if (from_renderer && (info->is_download() || info->is_stream()))
return;
+ if (from_renderer && info->is_detachable()) {
+ Detach();
+ return;
+ }
+
// TODO(darin): Perhaps we should really be looking to see if the status is
// IO_PENDING?
bool was_pending = request_->is_pending();
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jkarlin@google.com/25772002/652001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jkarlin@google.com/25772002/652001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jkarlin@google.com/25772002/652001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jkarlin@google.com/25772002/652001
Message was sent while issue was closed.
Change committed as 231910 |
