Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9113028:
Substitute ResourceHandlers with dummy ResourceHandler while request is transferred (Closed)
|
Created:
8 years, 11 months ago by battre Modified:
8 years, 11 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, jam, dpranke-watch+content_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org, sky Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSubstitute ResourceHandlers with dummy ResourceHandler while request is transferred
This CL implements a new ResourceHandler that only allows cancelling its
request. The idea is that in case a URLRequest is being transferred to a new
renderer, we need to get rid of its old ResourceHandlers as these may have
dependencies to the old renderer which is about to die. We replace the
request's old ResourceHandler with a new TransferredResourceHandler until
the request is resumed by the new renderer.
BUG=107692
TEST=no
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=117658
Patch Set 1 #
Total comments: 7
Patch Set 2 : Addressed Matt's comments #
Total comments: 4
Patch Set 3 : Renamed paused_resource_handler.* to transferred_resource_handler.* #Patch Set 4 : Merged with ToT #
Total comments: 4
Patch Set 5 : Addressed Darin's comments #
Total comments: 8
Patch Set 6 : Addressed Darin's comments #
Messages
Total messages: 22 (0 generated)
Please review. Thanks, Dominic http://codereview.chromium.org/9113028/diff/1/content/browser/renderer_host/r... File content/browser/renderer_host/resource_dispatcher_host.cc (right): http://codereview.chromium.org/9113028/diff/1/content/browser/renderer_host/r... content/browser/renderer_host/resource_dispatcher_host.cc:1321: info->set_resource_handler(new AllowCancelResourceHandler()); We cannot put this code into the TransferNavigationResourceHandler, as resource handlers are refcounted. We would remove the only reference to the ResourceHandler while it is being executed.
http://codereview.chromium.org/9113028/diff/1/content/browser/renderer_host/a... File content/browser/renderer_host/allow_cancel_resource_handler.h (right): http://codereview.chromium.org/9113028/diff/1/content/browser/renderer_host/a... content/browser/renderer_host/allow_cancel_resource_handler.h:13: class AllowCancelResourceHandler : public ResourceHandler { This is a bit awkwardly named IMO. Something more generic like PausedResourceHandler might be preferable. http://codereview.chromium.org/9113028/diff/1/content/browser/renderer_host/r... File content/browser/renderer_host/resource_dispatcher_host.cc (right): http://codereview.chromium.org/9113028/diff/1/content/browser/renderer_host/r... content/browser/renderer_host/resource_dispatcher_host.cc:1314: // ResourceHandlers should not received any notifications because they may receive* http://codereview.chromium.org/9113028/diff/1/content/browser/renderer_host/r... content/browser/renderer_host/resource_dispatcher_host.cc:1321: info->set_resource_handler(new AllowCancelResourceHandler()); On 2012/01/05 22:55:17, battre wrote: > We cannot put this code into the TransferNavigationResourceHandler, as resource > handlers are refcounted. We would remove the only reference to the > ResourceHandler while it is being executed. It's perfectly fine to delete an instance of a class while running one of its methods - just as long as you don't dereference that instance after the fact. Please move this to the TransferNavigationResourceHandler (make sure it's the last code that gets executed before returning, and add a comment warning that it might 'delete this').
http://codereview.chromium.org/9113028/diff/1/content/browser/renderer_host/a... File content/browser/renderer_host/allow_cancel_resource_handler.h (right): http://codereview.chromium.org/9113028/diff/1/content/browser/renderer_host/a... content/browser/renderer_host/allow_cancel_resource_handler.h:13: class AllowCancelResourceHandler : public ResourceHandler { On 2012/01/05 23:52:44, Matt Perry wrote: > This is a bit awkwardly named IMO. Something more generic like > PausedResourceHandler might be preferable. Done. http://codereview.chromium.org/9113028/diff/1/content/browser/renderer_host/r... File content/browser/renderer_host/resource_dispatcher_host.cc (right): http://codereview.chromium.org/9113028/diff/1/content/browser/renderer_host/r... content/browser/renderer_host/resource_dispatcher_host.cc:1314: // ResourceHandlers should not received any notifications because they may On 2012/01/05 23:52:44, Matt Perry wrote: > receive* Done. http://codereview.chromium.org/9113028/diff/1/content/browser/renderer_host/r... content/browser/renderer_host/resource_dispatcher_host.cc:1321: info->set_resource_handler(new AllowCancelResourceHandler()); On 2012/01/05 23:52:44, Matt Perry wrote: > On 2012/01/05 22:55:17, battre wrote: > > We cannot put this code into the TransferNavigationResourceHandler, as > resource > > handlers are refcounted. We would remove the only reference to the > > ResourceHandler while it is being executed. > > It's perfectly fine to delete an instance of a class while running one of its > methods - just as long as you don't dereference that instance after the fact. > Please move this to the TransferNavigationResourceHandler (make sure it's the > last code that gets executed before returning, and add a comment warning that it > might 'delete this'). Done. Moved to MarkAsTransferredNavigation as discussed.
lgtm
+sky for OWNERS approval
I don't know this particular area well enough to review it. Could you get one of the other owners to review it? -Scott
-sky +darin for OWNERS approval
http://codereview.chromium.org/9113028/diff/2006/content/browser/renderer_hos... File content/browser/renderer_host/paused_resource_handler.h (right): http://codereview.chromium.org/9113028/diff/2006/content/browser/renderer_hos... content/browser/renderer_host/paused_resource_handler.h:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. New files should use 2012, right? http://codereview.chromium.org/9113028/diff/2006/content/browser/renderer_hos... content/browser/renderer_host/paused_resource_handler.h:15: class PausedResourceHandler : public ResourceHandler { Sorry, I'm not sure I agree on the name. There's a lot of cases that we pause requests with the intention of resuming them. This seems like it will be confusing to people who might think those cases should use a PausedResourceHandler. Can we come up with a name that indicates this isn't expected to be unpaused? Something like TransferredResourceHandler? That may not great either, though, since it's close to TransferNavigationResourceHandler.
http://codereview.chromium.org/9113028/diff/2006/content/browser/renderer_hos... File content/browser/renderer_host/paused_resource_handler.h (right): http://codereview.chromium.org/9113028/diff/2006/content/browser/renderer_hos... content/browser/renderer_host/paused_resource_handler.h:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2012/01/06 17:54:52, creis wrote: > New files should use 2012, right? Done. http://codereview.chromium.org/9113028/diff/2006/content/browser/renderer_hos... content/browser/renderer_host/paused_resource_handler.h:15: class PausedResourceHandler : public ResourceHandler { On 2012/01/06 17:54:52, creis wrote: > Sorry, I'm not sure I agree on the name. There's a lot of cases that we pause > requests with the intention of resuming them. This seems like it will be > confusing to people who might think those cases should use a > PausedResourceHandler. > > Can we come up with a name that indicates this isn't expected to be unpaused? > Something like TransferredResourceHandler? That may not great either, though, > since it's close to TransferNavigationResourceHandler. Lacking a better proposal I have renamed the class to TransferredResourceHandler. Done.
Thanks. LGTM.
On 2012/01/06 18:39:04, battre wrote: > http://codereview.chromium.org/9113028/diff/2006/content/browser/renderer_hos... > File content/browser/renderer_host/paused_resource_handler.h (right): > > http://codereview.chromium.org/9113028/diff/2006/content/browser/renderer_hos... > content/browser/renderer_host/paused_resource_handler.h:1: // Copyright (c) 2011 > The Chromium Authors. All rights reserved. > On 2012/01/06 17:54:52, creis wrote: > > New files should use 2012, right? > > Done. > > http://codereview.chromium.org/9113028/diff/2006/content/browser/renderer_hos... > content/browser/renderer_host/paused_resource_handler.h:15: class > PausedResourceHandler : public ResourceHandler { > On 2012/01/06 17:54:52, creis wrote: > > Sorry, I'm not sure I agree on the name. There's a lot of cases that we pause > > requests with the intention of resuming them. This seems like it will be > > confusing to people who might think those cases should use a > > PausedResourceHandler. > > > > Can we come up with a name that indicates this isn't expected to be unpaused? > > Something like TransferredResourceHandler? That may not great either, though, > > since it's close to TransferNavigationResourceHandler. > > Lacking a better proposal I have renamed the class to > TransferredResourceHandler. How about PendingTransferredResourceHandler? Sorry for the nitpicking on the name :)
On 2012/01/06 19:35:33, Matt Perry wrote: > On 2012/01/06 18:39:04, battre wrote: > > > http://codereview.chromium.org/9113028/diff/2006/content/browser/renderer_hos... > > File content/browser/renderer_host/paused_resource_handler.h (right): > > > > > http://codereview.chromium.org/9113028/diff/2006/content/browser/renderer_hos... > > content/browser/renderer_host/paused_resource_handler.h:1: // Copyright (c) > 2011 > > The Chromium Authors. All rights reserved. > > On 2012/01/06 17:54:52, creis wrote: > > > New files should use 2012, right? > > > > Done. > > > > > http://codereview.chromium.org/9113028/diff/2006/content/browser/renderer_hos... > > content/browser/renderer_host/paused_resource_handler.h:15: class > > PausedResourceHandler : public ResourceHandler { > > On 2012/01/06 17:54:52, creis wrote: > > > Sorry, I'm not sure I agree on the name. There's a lot of cases that we > pause > > > requests with the intention of resuming them. This seems like it will be > > > confusing to people who might think those cases should use a > > > PausedResourceHandler. > > > > > > Can we come up with a name that indicates this isn't expected to be > unpaused? > > > Something like TransferredResourceHandler? That may not great either, > though, > > > since it's close to TransferNavigationResourceHandler. > > > > Lacking a better proposal I have renamed the class to > > TransferredResourceHandler. > > How about PendingTransferredResourceHandler? Sorry for the nitpicking on the > name :) I'll wait for Darin's verdict and change it if there is no objection.
http://codereview.chromium.org/9113028/diff/19001/content/browser/renderer_ho... File content/browser/renderer_host/transferred_resource_handler.h (right): http://codereview.chromium.org/9113028/diff/19001/content/browser/renderer_ho... content/browser/renderer_host/transferred_resource_handler.h:15: class TransferredResourceHandler : public ResourceHandler { Perhaps it would be useful to factor out an UnreachedResourceHandler, which purely asserts NOTREACHED on all methods. Then subclass that to special-case the behavior of OnResponseCompleted and OnRequestClosed. Or... maybe you could rename TransferredResourceHandler to DoomedResourceHandler. It expects to only receive failure and close events. That sort of makes sense. It could make sense for such a resource handler to retain a reference to the ResourceHandler it replaces. http://codereview.chromium.org/9113028/diff/19001/content/browser/renderer_ho... content/browser/renderer_host/transferred_resource_handler.h:22: TransferredResourceHandler(ResourceHandler* old_handlers); nit: why is this variable name pluralized? it is not an array is it?
http://codereview.chromium.org/9113028/diff/19001/content/browser/renderer_ho... File content/browser/renderer_host/transferred_resource_handler.h (right): http://codereview.chromium.org/9113028/diff/19001/content/browser/renderer_ho... content/browser/renderer_host/transferred_resource_handler.h:15: class TransferredResourceHandler : public ResourceHandler { On 2012/01/06 22:59:02, darin wrote: > Perhaps it would be useful to factor out an UnreachedResourceHandler, which > purely asserts NOTREACHED on all methods. Then subclass that to special-case > the behavior of OnResponseCompleted and OnRequestClosed. I think this is not very useful as we won't have a second class that will derive from UnreachedResourceHandler. > Or... maybe you could rename TransferredResourceHandler to > DoomedResourceHandler. It expects to only receive failure and close events. > That sort of makes sense. It could make sense for such a resource handler to > retain a reference to the ResourceHandler it replaces. Is "DoomedResourceHandler" really an appropriate name? The URLRequest is not doomed. This is what is going on: 1) A Renderer tries to open a website. 2) The ResourceDispatcherHost creates a URLRequest 3) We decide that the website needs to be opened in a different Renderer (due to cross-origin navigation) 4) The ResourceDispatcherHost parks the URLRequest and asks the new Renderer to open the same page. 5) The new Renderer sends the request to the ResourceDispatcherHost 6) The ResourceDispatcherHost recognizes the request and resume the old URLRequest instead of creating a new one For the time during which the URLRequest is parked, we need to assign it a temporary ResourceHandler. The old ones depend on the old RenderProcessHost instance that gets killed. This ResourceHandler is really just a dummy that is replaced when the RDH resumes the URLRequest. I think I would consider neither the URLRequest nor the ResourceHandler as "doomed". I would propose "TransferredRequestResourceHandler". We have a TransferNavigationResourceHandler that checks whether the request gets transferred to a new renderer. The RH would handle transferred URLRequests. Previous suggestions "TransferredResourceHandler" and "PendingTransferredResourceHandler" would work for me as well. WDYT? http://codereview.chromium.org/9113028/diff/19001/content/browser/renderer_ho... content/browser/renderer_host/transferred_resource_handler.h:22: TransferredResourceHandler(ResourceHandler* old_handlers); On 2012/01/06 22:59:02, darin wrote: > nit: why is this variable name pluralized? it is not an array is it? It a linked chain of handlers. I don't mind using singular because we are pointing to the first one. I will change this.
On Fri, Jan 6, 2012 at 3:37 PM, <battre@chromium.org> wrote: > > http://codereview.chromium.**org/9113028/diff/19001/** > content/browser/renderer_host/**transferred_resource_handler.h<http://codereview.chromium.org/9113028/diff/19001/content/browser/renderer_host/transferred_resource_handler.h> > File content/browser/renderer_host/**transferred_resource_handler.h > (right): > > http://codereview.chromium.**org/9113028/diff/19001/** > content/browser/renderer_host/**transferred_resource_handler.**h#newcode15<http://codereview.chromium.org/9113028/diff/19001/content/browser/renderer_host/transferred_resource_handler.h#newcode15> > content/browser/renderer_host/**transferred_resource_handler.**h:15: class > TransferredResourceHandler : public ResourceHandler { > On 2012/01/06 22:59:02, darin wrote: > >> Perhaps it would be useful to factor out an UnreachedResourceHandler, >> > which > >> purely asserts NOTREACHED on all methods. Then subclass that to >> > special-case > >> the behavior of OnResponseCompleted and OnRequestClosed. >> > > I think this is not very useful as we won't have a second class that > will derive from UnreachedResourceHandler. Right, I am thinking ahead a bit. I just added LayeredResourceHandler to remove a bit of code duplication. Anyways, this suggestion is mutually exclusive with the suggestion below... > > > Or... maybe you could rename TransferredResourceHandler to >> DoomedResourceHandler. It expects to only receive failure and close >> > events. > >> That sort of makes sense. It could make sense for such a resource >> > handler to > >> retain a reference to the ResourceHandler it replaces. >> > > Is "DoomedResourceHandler" really an appropriate name? The URLRequest is > not doomed. > > This is what is going on: > > 1) A Renderer tries to open a website. > 2) The ResourceDispatcherHost creates a URLRequest > 3) We decide that the website needs to be opened in a different Renderer > (due to cross-origin navigation) > 4) The ResourceDispatcherHost parks the URLRequest and asks the new > Renderer to open the same page. > 5) The new Renderer sends the request to the ResourceDispatcherHost > 6) The ResourceDispatcherHost recognizes the request and resume the old > URLRequest instead of creating a new one > > For the time during which the URLRequest is parked, we need to assign it > a temporary ResourceHandler. The old ones depend on the old > RenderProcessHost instance that gets killed. > > This ResourceHandler is really just a dummy that is replaced when the > RDH resumes the URLRequest. > > I think I would consider neither the URLRequest nor the ResourceHandler > as "doomed". > > I would propose "**TransferredRequestResourceHand**ler". We have a > TransferNavigationResourceHand**ler that checks whether the request gets > transferred to a new renderer. The RH would handle transferred > URLRequests. > > Previous suggestions "TransferredResourceHandler" and > "**PendingTransferredResourceHand**ler" would work for me as well. > > WDYT? I understand what you are saying, but looking purely at the new class, it seems to pretty clearly be about asserting that all methods except for "OnResponseComplete" and "OnRequestClosed" are unreached. It also asserts that OnResponseComplete communicates a status of CANCELED or FAILED. This to me looks like the pattern of an URLRequest that has failed or has been cancelled. This class doesn't seem to have anything specific to do with how you are using it. -Darin > > > http://codereview.chromium.**org/9113028/diff/19001/** > content/browser/renderer_host/**transferred_resource_handler.**h#newcode22<http://codereview.chromium.org/9113028/diff/19001/content/browser/renderer_host/transferred_resource_handler.h#newcode22> > content/browser/renderer_host/**transferred_resource_handler.**h:22: > TransferredResourceHandler(**ResourceHandler* old_handlers); > On 2012/01/06 22:59:02, darin wrote: > >> nit: why is this variable name pluralized? it is not an array is it? >> > > It a linked chain of handlers. I don't mind using singular because we > are pointing to the first one. I will change this. > > http://codereview.chromium.**org/9113028/<http://codereview.chromium.org/9113... >
> > Or... maybe you could rename TransferredResourceHandler to > >> DoomedResourceHandler. It expects to only receive failure and close > >> > > events. > > > >> That sort of makes sense. It could make sense for such a resource > >> > > handler to > > > >> retain a reference to the ResourceHandler it replaces. > >> > > > > Is "DoomedResourceHandler" really an appropriate name? The URLRequest is > > not doomed. > > > > This is what is going on: > > > > 1) A Renderer tries to open a website. > > 2) The ResourceDispatcherHost creates a URLRequest > > 3) We decide that the website needs to be opened in a different Renderer > > (due to cross-origin navigation) > > 4) The ResourceDispatcherHost parks the URLRequest and asks the new > > Renderer to open the same page. > > 5) The new Renderer sends the request to the ResourceDispatcherHost > > 6) The ResourceDispatcherHost recognizes the request and resume the old > > URLRequest instead of creating a new one > > > > For the time during which the URLRequest is parked, we need to assign it > > a temporary ResourceHandler. The old ones depend on the old > > RenderProcessHost instance that gets killed. > > > > This ResourceHandler is really just a dummy that is replaced when the > > RDH resumes the URLRequest. > > > > I think I would consider neither the URLRequest nor the ResourceHandler > > as "doomed". > > > > I would propose "**TransferredRequestResourceHand**ler". We have a > > TransferNavigationResourceHand**ler that checks whether the request gets > > transferred to a new renderer. The RH would handle transferred > > URLRequests. > > > > Previous suggestions "TransferredResourceHandler" and > > "**PendingTransferredResourceHand**ler" would work for me as well. > > > > WDYT? > > > I understand what you are saying, but looking purely at the new class, it > seems to pretty clearly be about asserting that all methods except for > "OnResponseComplete" and "OnRequestClosed" are unreached. It also asserts > that OnResponseComplete communicates a status of CANCELED or FAILED. This > to me looks like the pattern of an URLRequest that has failed or has been > cancelled. This class doesn't seem to have anything specific to do with > how you are using it. Done.
On 2012/01/09 19:49:30, battre wrote: > > > Or... maybe you could rename TransferredResourceHandler to > > >> DoomedResourceHandler. It expects to only receive failure and close > > >> > > > events. > > > > > >> That sort of makes sense. It could make sense for such a resource > > >> > > > handler to > > > > > >> retain a reference to the ResourceHandler it replaces. > > >> > > > > > > Is "DoomedResourceHandler" really an appropriate name? The URLRequest is > > > not doomed. > > > > > > This is what is going on: > > > > > > 1) A Renderer tries to open a website. > > > 2) The ResourceDispatcherHost creates a URLRequest > > > 3) We decide that the website needs to be opened in a different Renderer > > > (due to cross-origin navigation) > > > 4) The ResourceDispatcherHost parks the URLRequest and asks the new > > > Renderer to open the same page. > > > 5) The new Renderer sends the request to the ResourceDispatcherHost > > > 6) The ResourceDispatcherHost recognizes the request and resume the old > > > URLRequest instead of creating a new one > > > > > > For the time during which the URLRequest is parked, we need to assign it > > > a temporary ResourceHandler. The old ones depend on the old > > > RenderProcessHost instance that gets killed. > > > > > > This ResourceHandler is really just a dummy that is replaced when the > > > RDH resumes the URLRequest. > > > > > > I think I would consider neither the URLRequest nor the ResourceHandler > > > as "doomed". > > > > > > I would propose "**TransferredRequestResourceHand**ler". We have a > > > TransferNavigationResourceHand**ler that checks whether the request gets > > > transferred to a new renderer. The RH would handle transferred > > > URLRequests. > > > > > > Previous suggestions "TransferredResourceHandler" and > > > "**PendingTransferredResourceHand**ler" would work for me as well. > > > > > > WDYT? > > > > > > I understand what you are saying, but looking purely at the new class, it > > seems to pretty clearly be about asserting that all methods except for > > "OnResponseComplete" and "OnRequestClosed" are unreached. It also asserts > > that OnResponseComplete communicates a status of CANCELED or FAILED. This > > to me looks like the pattern of an URLRequest that has failed or has been > > cancelled. This class doesn't seem to have anything specific to do with > > how you are using it. > > Done. @darin: friendly ping
http://codereview.chromium.org/9113028/diff/23001/content/browser/renderer_ho... File content/browser/renderer_host/doomed_resource_handler.h (right): http://codereview.chromium.org/9113028/diff/23001/content/browser/renderer_ho... content/browser/renderer_host/doomed_resource_handler.h:13: // requests while activated for a URLRequest. This is used for requests that this second sentence is particular to the way you are using this class, but as discussed before, this class could be used for other applications too. i would delete this comment. http://codereview.chromium.org/9113028/diff/23001/content/browser/renderer_ho... content/browser/renderer_host/doomed_resource_handler.h:17: // As the DoomedResourceHandler is constructed and subsituted from code nit: subsituted -> substituted http://codereview.chromium.org/9113028/diff/23001/content/browser/renderer_ho... content/browser/renderer_host/doomed_resource_handler.h:22: DoomedResourceHandler(ResourceHandler* old_handler); nit: mark this "explicit" http://codereview.chromium.org/9113028/diff/23001/content/browser/renderer_ho... content/browser/renderer_host/doomed_resource_handler.h:52: this comment is redundant. don't you say the same thing above? perhaps it is better to keep the public one.
I have addressed your comments. Please push this into the CQ if it LGTY (we need to get this into canary ASAP). Thanks, Dominic http://codereview.chromium.org/9113028/diff/23001/content/browser/renderer_ho... File content/browser/renderer_host/doomed_resource_handler.h (right): http://codereview.chromium.org/9113028/diff/23001/content/browser/renderer_ho... content/browser/renderer_host/doomed_resource_handler.h:13: // requests while activated for a URLRequest. This is used for requests that On 2012/01/12 06:22:52, darin wrote: > this second sentence is particular to the way you are using this class, but as > discussed before, this class could be used for other applications too. i would > delete this comment. Done. http://codereview.chromium.org/9113028/diff/23001/content/browser/renderer_ho... content/browser/renderer_host/doomed_resource_handler.h:17: // As the DoomedResourceHandler is constructed and subsituted from code On 2012/01/12 06:22:52, darin wrote: > nit: subsituted -> substituted Done. http://codereview.chromium.org/9113028/diff/23001/content/browser/renderer_ho... content/browser/renderer_host/doomed_resource_handler.h:22: DoomedResourceHandler(ResourceHandler* old_handler); On 2012/01/12 06:22:52, darin wrote: > nit: mark this "explicit" Done. http://codereview.chromium.org/9113028/diff/23001/content/browser/renderer_ho... content/browser/renderer_host/doomed_resource_handler.h:52: On 2012/01/12 06:22:52, darin wrote: > this comment is redundant. don't you say the same thing above? perhaps it is > better to keep the public one. Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/battre@chromium.org/9113028/31001
Change committed as 117658 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
