Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2847443002:
PlzNavigate: make NavigationResourceHandler a LayeredResourceHandler (Closed)
|
Created:
3 years, 8 months ago by clamy Modified:
3 years, 7 months ago CC:
chromium-reviews, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, Randy Smith (Not in Mondays), darin-cc_chromium.org, loading-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: make NavigationResourceHandler a LayeredResourceHandler
This CL makes NavigationResourceHandler a LayeredResourceHandler and
moves it just after the MimeSniffingResourceHandler. In PlzNavigate, the
ordering of ResourceHandlers in navigation is the following:
- ThrottlingResourceHandler
- MimeSniffingResourceHandler
- ThrottlingResourceHandler (no NavigationResourceThrottle)
- InterceptingResourceHandler
- NavigationResourceHandler: interacts with the UI thread & writes the
response in a stream.
After this CL, the ordering of ResourceHandlers will be the following:
- ThrottlingResourceHandler
- MimeSniffingResourceHandler
- NavigationResourceHandler: interacts with the UI thread
- ThrottlingResourceHandler (no NavigationResourceThrottle)
- InterceptingResourceHandler
- StreamResourceHandler: writes the response in a stream.
The reason is two-fold. First, it better matches the ordering of
ResourceHandlers in the current navigation architecture, which is:
- ThrottlingResourceHandler
- MimeSniffingResourceHandler
- ThrottlingResourceHandler with a NavigationResourceThrottle (interacts
with the UI thread)
- InterceptingResourceHandler
- AsyncResourceHandler
In particular, this ensures that the NavigationThrottles will be
executed before teh second-round of ResourceThrottles both in
PlzNavigate and in the current architecture.
Second, moving NavigationResourceHandler before the
InterceptingResourceHandler allows to properly pause downloads & stream
requests while we execute the NavigationThrottles on the UI thread. This
was not possible before because InterceptingResourceHandler does not
expect its next handler to pause the request in OnResponseStarted. Not
pausing it is causing race conditions with the processing of the
navigation on the UI thread.
BUG=709771
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2847443002
Cr-Commit-Position: refs/heads/master@{#468692}
Committed: https://chromium.googlesource.com/chromium/src/+/ea2f911cf6b0b023ff26e17bb9e57c871c8f52f5
Patch Set 1 #Patch Set 2 : Removed one unit test #
Total comments: 28
Patch Set 3 : Rebase #Patch Set 4 : Addressed comments #
Messages
Total messages: 30 (18 generated)
Description was changed from ========== PlzNavigate: make NavigationResourceHandler a LayeredResourceHandler This CL makes NavigationResourceHandler a LayeredResourceHandler and moves it just after the MimeSniffingResourceHandler. In PlzNavigate, the ordering of ResourceHandlers in navigation is the following: - ThrottlingResourceHandler - MimeSniffingResourceHandler - ThrottlingResourceHandler (no NavigationResourceThrottle) - InterceptingResourceHandler - NavigationResourceHandler: interacts with the UI thread & writes the response in a stream. After this CL, the ordering of ResourceHandlers will be the following: - ThrottlingResourceHandler - MimeSniffingResourceHandler - NavigationResourceHandler: interacts with the UI thread - ThrottlingResourceHandler (no NavigationResourceThrottle) - InterceptingResourceHandler - StreamResourceHandler: writes the response in a stream. The reason is two-fold. First, it better matches the ordering of ResourceHandlers in the current navigation architecture, which is: - ThrottlingResourceHandler - MimeSniffingResourceHandler - ThrottlingResourceHandler with a NavigationResourceThrottle (interacts with the UI thread) - InterceptingResourceHandler - AsyncResourceHandler In particular, this ensures that the NavigationThrottles will be executed before teh second-round of ResourceThrottles both in PlzNavigate and in the current architecture. Second, moving NavigationResourceHandler before the InterceptingResourceHandler allows to properly pause downloads & stream requests while we execute the NavigationThrottles on the UI thread. This was not possible before because InterceptingResourceHandler does not expect its next handler to pause the request in OnResponseReceived. Not pausing it is causing race conditions with the processing of the navigation on the UI thread. BUG=709771 ========== to ========== PlzNavigate: make NavigationResourceHandler a LayeredResourceHandler This CL makes NavigationResourceHandler a LayeredResourceHandler and moves it just after the MimeSniffingResourceHandler. In PlzNavigate, the ordering of ResourceHandlers in navigation is the following: - ThrottlingResourceHandler - MimeSniffingResourceHandler - ThrottlingResourceHandler (no NavigationResourceThrottle) - InterceptingResourceHandler - NavigationResourceHandler: interacts with the UI thread & writes the response in a stream. After this CL, the ordering of ResourceHandlers will be the following: - ThrottlingResourceHandler - MimeSniffingResourceHandler - NavigationResourceHandler: interacts with the UI thread - ThrottlingResourceHandler (no NavigationResourceThrottle) - InterceptingResourceHandler - StreamResourceHandler: writes the response in a stream. The reason is two-fold. First, it better matches the ordering of ResourceHandlers in the current navigation architecture, which is: - ThrottlingResourceHandler - MimeSniffingResourceHandler - ThrottlingResourceHandler with a NavigationResourceThrottle (interacts with the UI thread) - InterceptingResourceHandler - AsyncResourceHandler In particular, this ensures that the NavigationThrottles will be executed before teh second-round of ResourceThrottles both in PlzNavigate and in the current architecture. Second, moving NavigationResourceHandler before the InterceptingResourceHandler allows to properly pause downloads & stream requests while we execute the NavigationThrottles on the UI thread. This was not possible before because InterceptingResourceHandler does not expect its next handler to pause the request in OnResponseReceived. Not pausing it is causing race conditions with the processing of the navigation on the UI thread. BUG=709771 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by clamy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== PlzNavigate: make NavigationResourceHandler a LayeredResourceHandler This CL makes NavigationResourceHandler a LayeredResourceHandler and moves it just after the MimeSniffingResourceHandler. In PlzNavigate, the ordering of ResourceHandlers in navigation is the following: - ThrottlingResourceHandler - MimeSniffingResourceHandler - ThrottlingResourceHandler (no NavigationResourceThrottle) - InterceptingResourceHandler - NavigationResourceHandler: interacts with the UI thread & writes the response in a stream. After this CL, the ordering of ResourceHandlers will be the following: - ThrottlingResourceHandler - MimeSniffingResourceHandler - NavigationResourceHandler: interacts with the UI thread - ThrottlingResourceHandler (no NavigationResourceThrottle) - InterceptingResourceHandler - StreamResourceHandler: writes the response in a stream. The reason is two-fold. First, it better matches the ordering of ResourceHandlers in the current navigation architecture, which is: - ThrottlingResourceHandler - MimeSniffingResourceHandler - ThrottlingResourceHandler with a NavigationResourceThrottle (interacts with the UI thread) - InterceptingResourceHandler - AsyncResourceHandler In particular, this ensures that the NavigationThrottles will be executed before teh second-round of ResourceThrottles both in PlzNavigate and in the current architecture. Second, moving NavigationResourceHandler before the InterceptingResourceHandler allows to properly pause downloads & stream requests while we execute the NavigationThrottles on the UI thread. This was not possible before because InterceptingResourceHandler does not expect its next handler to pause the request in OnResponseReceived. Not pausing it is causing race conditions with the processing of the navigation on the UI thread. BUG=709771 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: make NavigationResourceHandler a LayeredResourceHandler This CL makes NavigationResourceHandler a LayeredResourceHandler and moves it just after the MimeSniffingResourceHandler. In PlzNavigate, the ordering of ResourceHandlers in navigation is the following: - ThrottlingResourceHandler - MimeSniffingResourceHandler - ThrottlingResourceHandler (no NavigationResourceThrottle) - InterceptingResourceHandler - NavigationResourceHandler: interacts with the UI thread & writes the response in a stream. After this CL, the ordering of ResourceHandlers will be the following: - ThrottlingResourceHandler - MimeSniffingResourceHandler - NavigationResourceHandler: interacts with the UI thread - ThrottlingResourceHandler (no NavigationResourceThrottle) - InterceptingResourceHandler - StreamResourceHandler: writes the response in a stream. The reason is two-fold. First, it better matches the ordering of ResourceHandlers in the current navigation architecture, which is: - ThrottlingResourceHandler - MimeSniffingResourceHandler - ThrottlingResourceHandler with a NavigationResourceThrottle (interacts with the UI thread) - InterceptingResourceHandler - AsyncResourceHandler In particular, this ensures that the NavigationThrottles will be executed before teh second-round of ResourceThrottles both in PlzNavigate and in the current architecture. Second, moving NavigationResourceHandler before the InterceptingResourceHandler allows to properly pause downloads & stream requests while we execute the NavigationThrottles on the UI thread. This was not possible before because InterceptingResourceHandler does not expect its next handler to pause the request in OnResponseReceived. Not pausing it is causing race conditions with the processing of the navigation on the UI thread. BUG=709771 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
clamy@chromium.org changed reviewers: + mmenke@chromium.org, nasko@chromium.org
@nasko, mmenke: PTAL The main issue that patch is trying to solve is that we don't pause the navigation request in OnResponseStarted when it's a download/stream when PlzNavigate is enabled. This is because the ResourceHandler that is supposed to pause it is after the InterceptingResourceHandler which does not expect such a pause. I tried in a first approach to modify InterceptingResourceHandler so that it allows its next handler to pause the request in OnResponseStarted, but it proved a lot simpler to just split the NavigationResourceHandler in two and move the pausing part before InterceptingResourceHandler.
Description was changed from ========== PlzNavigate: make NavigationResourceHandler a LayeredResourceHandler This CL makes NavigationResourceHandler a LayeredResourceHandler and moves it just after the MimeSniffingResourceHandler. In PlzNavigate, the ordering of ResourceHandlers in navigation is the following: - ThrottlingResourceHandler - MimeSniffingResourceHandler - ThrottlingResourceHandler (no NavigationResourceThrottle) - InterceptingResourceHandler - NavigationResourceHandler: interacts with the UI thread & writes the response in a stream. After this CL, the ordering of ResourceHandlers will be the following: - ThrottlingResourceHandler - MimeSniffingResourceHandler - NavigationResourceHandler: interacts with the UI thread - ThrottlingResourceHandler (no NavigationResourceThrottle) - InterceptingResourceHandler - StreamResourceHandler: writes the response in a stream. The reason is two-fold. First, it better matches the ordering of ResourceHandlers in the current navigation architecture, which is: - ThrottlingResourceHandler - MimeSniffingResourceHandler - ThrottlingResourceHandler with a NavigationResourceThrottle (interacts with the UI thread) - InterceptingResourceHandler - AsyncResourceHandler In particular, this ensures that the NavigationThrottles will be executed before teh second-round of ResourceThrottles both in PlzNavigate and in the current architecture. Second, moving NavigationResourceHandler before the InterceptingResourceHandler allows to properly pause downloads & stream requests while we execute the NavigationThrottles on the UI thread. This was not possible before because InterceptingResourceHandler does not expect its next handler to pause the request in OnResponseReceived. Not pausing it is causing race conditions with the processing of the navigation on the UI thread. BUG=709771 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: make NavigationResourceHandler a LayeredResourceHandler This CL makes NavigationResourceHandler a LayeredResourceHandler and moves it just after the MimeSniffingResourceHandler. In PlzNavigate, the ordering of ResourceHandlers in navigation is the following: - ThrottlingResourceHandler - MimeSniffingResourceHandler - ThrottlingResourceHandler (no NavigationResourceThrottle) - InterceptingResourceHandler - NavigationResourceHandler: interacts with the UI thread & writes the response in a stream. After this CL, the ordering of ResourceHandlers will be the following: - ThrottlingResourceHandler - MimeSniffingResourceHandler - NavigationResourceHandler: interacts with the UI thread - ThrottlingResourceHandler (no NavigationResourceThrottle) - InterceptingResourceHandler - StreamResourceHandler: writes the response in a stream. The reason is two-fold. First, it better matches the ordering of ResourceHandlers in the current navigation architecture, which is: - ThrottlingResourceHandler - MimeSniffingResourceHandler - ThrottlingResourceHandler with a NavigationResourceThrottle (interacts with the UI thread) - InterceptingResourceHandler - AsyncResourceHandler In particular, this ensures that the NavigationThrottles will be executed before teh second-round of ResourceThrottles both in PlzNavigate and in the current architecture. Second, moving NavigationResourceHandler before the InterceptingResourceHandler allows to properly pause downloads & stream requests while we execute the NavigationThrottles on the UI thread. This was not possible before because InterceptingResourceHandler does not expect its next handler to pause the request in OnResponseStarted. Not pausing it is causing race conditions with the processing of the navigation on the UI thread. BUG=709771 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by clamy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_unittest.cc:2562: if (IsBrowserSideNavigationEnabled()) I've had to disable this test with PlzNavigate. The reason is the following: with the change, we properly wait on the UI thread before switching the last ResourceHandler and make the request a proper download request. This is the state the test expects the request to be in when it calls CancelRequestsForContext. But when we initialize the DownloadResourceHandler, it also posts a task to initialize the PowerSaveBlocker, which does not expect to be intialized in this unit test. Since the task to intialize the PowerSaveBlocker happens before notifying the UI thread of the response, when we add the additional wait here we end up processing the PowerSaveBlocker initialization. To solve the issue, I'd have to either modify the download code so that it does not create a PowerSaveBlocker, or just simulate the IO thread part of navigation. Doing that would require making NavigationURLLoaderImplCore a virtual class, with its Impl and Test version (along with the factories to make them) which seems a bit heavy handed. Considering that in PlzNavigate we only test about a third of what the test is testing anyway (because the rest doesn;t make sense), I went with disabling it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry, forgot about this one. At the front of my queue for tomorrow.
Based on the overall description of the change, it seems logical to me. I'd defer reviewing the actual code changes to mmenke@, as it is mostly code in the loader.
Should this have browser tests? Particularly with servicification of the network stack, I think it would be great if we had some tests of the behavior that was broken that will survive that change.
Not an expert on the streams logic, but this seems pretty reasonable to me. https://codereview.chromium.org/2847443002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2847443002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:799: loader_->ProceedWithResponse(); Why move this? This seems to be exclusive to the next two branches, which both also have early returns, so this seems to make no difference. https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... File content/browser/loader/navigation_resource_handler.cc (right): https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... content/browser/loader/navigation_resource_handler.cc:58: core_ = nullptr; Why does this just set core_ to nullptr instead of calling DetachFromCore? https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... content/browser/loader/navigation_resource_handler.cc:106: std::unique_ptr<net::RedirectInfo>(new net::RedirectInfo(redirect_info)); base::MakeUnique<net::RedirectInfo>(redirect_info)...Or could just make it no longer a unique_ptr. Don't think we actually get anything from making it a unique_ptr. https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... content/browser/loader/navigation_resource_handler.cc:156: next_handler_->OnResponseCompleted(status, std::move(controller)); So I guess we now unconditionally create writer_.stream(), and finalize it, even if there's nothing on the other end? Does that work? (I'm not at all familiar with content/browser/streams) https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... File content/browser/loader/navigation_resource_handler.h (right): https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... content/browser/loader/navigation_resource_handler.h:26: // control the flow of naviagtion requests. nit: navigation https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:1494: NavigationURLLoaderImplCore* loader, Can we give this a name that makes it clearer this is not a resource_loader? navigation_loader_core? https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:1551: if (IsBrowserSideNavigationEnabled() && IsResourceTypeFrame(resource_type)) { DCHECK(loader); DCHECK(stream_handle); Alternatively, could use loader on the branch, and then DCHECK on IsBrowserSideNavigationEnabled() && IsResourceTypeFrame(resource_type) here (And the inverse below). https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:1557: } } else { DCHECK(!loader); DCHECK(!stream_handle); } https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2086: GetStreamContextForResourceContext(extra_info->GetContext()); extra_info->GetContext() is just resource_context. https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2089: new_request->url().GetOrigin(), true)); Maybe a comment on the use of immediate mode? https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2091: static_cast<StreamResourceHandler*>(handler.get()) Remove this cast, and just make handler a unqiue_ptr<StreamResourceHandler> https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.h:612: std::unique_ptr<StreamHandle> stream_handle); Can we document these? While nothing else other than handler is documented, these two seem the least clear to me.
The CQ bit was checked by clamy@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2847443002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2847443002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:799: loader_->ProceedWithResponse(); On 2017/04/28 16:02:20, mmenke wrote: > Why move this? This seems to be exclusive to the next two branches, which both > also have early returns, so this seems to make no difference. Because in the case of downloads response_should_be_rendered_ should be rendered is false, but we need the load to proceed in the network stack. https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... File content/browser/loader/navigation_resource_handler.cc (right): https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... content/browser/loader/navigation_resource_handler.cc:58: core_ = nullptr; On 2017/04/28 16:02:20, mmenke wrote: > Why does this just set core_ to nullptr instead of calling DetachFromCore? No idea. I have changed it in the latest patchset. https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... content/browser/loader/navigation_resource_handler.cc:106: std::unique_ptr<net::RedirectInfo>(new net::RedirectInfo(redirect_info)); On 2017/04/28 16:02:20, mmenke wrote: > base::MakeUnique<net::RedirectInfo>(redirect_info)...Or could just make it no > longer a unique_ptr. Don't think we actually get anything from making it a > unique_ptr. Done. I made it a unique_ptr in order not to initialize it if we don't have redirects. I can make it a plain member variable if you prefer. https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... content/browser/loader/navigation_resource_handler.cc:156: next_handler_->OnResponseCompleted(status, std::move(controller)); On 2017/04/28 16:02:20, mmenke wrote: > So I guess we now unconditionally create writer_.stream(), and finalize it, even > if there's nothing on the other end? Does that work? (I'm not at all familiar Based on the tests, it seems to work. When we have a stream right now (like for pdfs) we do create it before having something reading it on the other end. And it's possible that we get the end of the stream before the renderer claims it (eg the renderer was busy for some time before starting to access the stream). So I think it should work. https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... File content/browser/loader/navigation_resource_handler.h (right): https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... content/browser/loader/navigation_resource_handler.h:26: // control the flow of naviagtion requests. On 2017/04/28 16:02:20, mmenke wrote: > nit: navigation Done. https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:1494: NavigationURLLoaderImplCore* loader, On 2017/04/28 16:02:21, mmenke wrote: > Can we give this a name that makes it clearer this is not a resource_loader? > navigation_loader_core? Done. https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:1551: if (IsBrowserSideNavigationEnabled() && IsResourceTypeFrame(resource_type)) { On 2017/04/28 16:02:21, mmenke wrote: > DCHECK(loader); > DCHECK(stream_handle); > > Alternatively, could use loader on the branch, and then DCHECK on > IsBrowserSideNavigationEnabled() && IsResourceTypeFrame(resource_type) here (And > the inverse below). Done. https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:1557: } On 2017/04/28 16:02:21, mmenke wrote: > } else { > DCHECK(!loader); > DCHECK(!stream_handle); > } Done. https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2086: GetStreamContextForResourceContext(extra_info->GetContext()); On 2017/04/28 16:02:21, mmenke wrote: > extra_info->GetContext() is just resource_context. Done. https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2089: new_request->url().GetOrigin(), true)); On 2017/04/28 16:02:21, mmenke wrote: > Maybe a comment on the use of immediate mode? Done. https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2091: static_cast<StreamResourceHandler*>(handler.get()) On 2017/04/28 16:02:21, mmenke wrote: > Remove this cast, and just make handler a unqiue_ptr<StreamResourceHandler> This require creating another std::unique_ptr<ResourceHandler> in line 2097 (handler = AddStandarHandlers... won't work). Which do you prefer? https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.h:612: std::unique_ptr<StreamHandle> stream_handle); On 2017/04/28 16:02:21, mmenke wrote: > Can we document these? While nothing else other than handler is documented, > these two seem the least clear to me. Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
This LGTM, but are there any downloads integration tests that require the changed behavior to pass? If so, can we enable them in this CL? If not, should there be? https://codereview.chromium.org/2847443002/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2847443002/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:799: loader_->ProceedWithResponse(); On 2017/05/02 15:05:44, clamy wrote: > On 2017/04/28 16:02:20, mmenke wrote: > > Why move this? This seems to be exclusive to the next two branches, which > both > > also have early returns, so this seems to make no difference. > > Because in the case of downloads response_should_be_rendered_ should be rendered > is false, but we need the load to proceed in the network stack. Thanks! Missed that case. https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... File content/browser/loader/navigation_resource_handler.cc (right): https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... content/browser/loader/navigation_resource_handler.cc:106: std::unique_ptr<net::RedirectInfo>(new net::RedirectInfo(redirect_info)); On 2017/05/02 15:05:44, clamy wrote: > On 2017/04/28 16:02:20, mmenke wrote: > > base::MakeUnique<net::RedirectInfo>(redirect_info)...Or could just make it no > > longer a unique_ptr. Don't think we actually get anything from making it a > > unique_ptr. > > Done. I made it a unique_ptr in order not to initialize it if we don't have > redirects. I can make it a plain member variable if you prefer. I'd tend to just make it a plain member variable, since we make few enough NavigationResourceHandlers that I don't think size or perf cost of a couple extra initializations hurts us at all. But if you prefer this approach, I think that's fine. Code should hopefully be going away relatively soon, anyways, using the URLLoader API instead. https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2847443002/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_unittest.cc:2562: if (IsBrowserSideNavigationEnabled()) On 2017/04/27 13:46:11, clamy wrote: > I've had to disable this test with PlzNavigate. The reason is the following: > with the change, we properly wait on the UI thread before switching the last > ResourceHandler and make the request a proper download request. This is the > state the test expects the request to be in when it calls > CancelRequestsForContext. But when we initialize the DownloadResourceHandler, it > also posts a task to initialize the PowerSaveBlocker, which does not expect to > be intialized in this unit test. Since the task to intialize the > PowerSaveBlocker happens before notifying the UI thread of the response, when we > add the additional wait here we end up processing the PowerSaveBlocker > initialization. > > To solve the issue, I'd have to either modify the download code so that it does > not create a PowerSaveBlocker, or just simulate the IO thread part of > navigation. Doing that would require making NavigationURLLoaderImplCore a > virtual class, with its Impl and Test version (along with the factories to make > them) which seems a bit heavy handed. Considering that in PlzNavigate we only > test about a third of what the test is testing anyway (because the rest doesn;t > make sense), I went with disabling it. This is fine for now, but what about when we actually enable PlzNavigate by default, and remove IsBrowserSideNavigationEnabled? How will we test this?
Thanks! Unfortunately, this was only caught because it broke pdf navigations on Canary. I'm trying to land this ASAP in order to fix the situation on Canary. I have filed https://bugs.chromium.org/p/chromium/issues/detail?id=717583 to re-enable the unit test & add a download integration test.
The CQ bit was unchecked by clamy@chromium.org
The CQ bit was checked by clamy@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/02 16:35:46, clamy wrote: > Thanks! Unfortunately, this was only caught because it broke pdf navigations on > Canary. I'm trying to land this ASAP in order to fix the situation on Canary. I > have filed https://bugs.chromium.org/p/chromium/issues/detail?id=717583 to > re-enable the unit test & add a download integration test. SGTM, thanks! Feel free to land whenever. Normally I'm an advocate of test first, but here I think there's a huge advantage to gathering live data that isn't present when normally adding a feature.
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493742971957810,
"parent_rev": "718f39723b40178893aed49600c24773c0ed5a88", "commit_rev":
"ea2f911cf6b0b023ff26e17bb9e57c871c8f52f5"}
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: make NavigationResourceHandler a LayeredResourceHandler This CL makes NavigationResourceHandler a LayeredResourceHandler and moves it just after the MimeSniffingResourceHandler. In PlzNavigate, the ordering of ResourceHandlers in navigation is the following: - ThrottlingResourceHandler - MimeSniffingResourceHandler - ThrottlingResourceHandler (no NavigationResourceThrottle) - InterceptingResourceHandler - NavigationResourceHandler: interacts with the UI thread & writes the response in a stream. After this CL, the ordering of ResourceHandlers will be the following: - ThrottlingResourceHandler - MimeSniffingResourceHandler - NavigationResourceHandler: interacts with the UI thread - ThrottlingResourceHandler (no NavigationResourceThrottle) - InterceptingResourceHandler - StreamResourceHandler: writes the response in a stream. The reason is two-fold. First, it better matches the ordering of ResourceHandlers in the current navigation architecture, which is: - ThrottlingResourceHandler - MimeSniffingResourceHandler - ThrottlingResourceHandler with a NavigationResourceThrottle (interacts with the UI thread) - InterceptingResourceHandler - AsyncResourceHandler In particular, this ensures that the NavigationThrottles will be executed before teh second-round of ResourceThrottles both in PlzNavigate and in the current architecture. Second, moving NavigationResourceHandler before the InterceptingResourceHandler allows to properly pause downloads & stream requests while we execute the NavigationThrottles on the UI thread. This was not possible before because InterceptingResourceHandler does not expect its next handler to pause the request in OnResponseStarted. Not pausing it is causing race conditions with the processing of the navigation on the UI thread. BUG=709771 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: make NavigationResourceHandler a LayeredResourceHandler This CL makes NavigationResourceHandler a LayeredResourceHandler and moves it just after the MimeSniffingResourceHandler. In PlzNavigate, the ordering of ResourceHandlers in navigation is the following: - ThrottlingResourceHandler - MimeSniffingResourceHandler - ThrottlingResourceHandler (no NavigationResourceThrottle) - InterceptingResourceHandler - NavigationResourceHandler: interacts with the UI thread & writes the response in a stream. After this CL, the ordering of ResourceHandlers will be the following: - ThrottlingResourceHandler - MimeSniffingResourceHandler - NavigationResourceHandler: interacts with the UI thread - ThrottlingResourceHandler (no NavigationResourceThrottle) - InterceptingResourceHandler - StreamResourceHandler: writes the response in a stream. The reason is two-fold. First, it better matches the ordering of ResourceHandlers in the current navigation architecture, which is: - ThrottlingResourceHandler - MimeSniffingResourceHandler - ThrottlingResourceHandler with a NavigationResourceThrottle (interacts with the UI thread) - InterceptingResourceHandler - AsyncResourceHandler In particular, this ensures that the NavigationThrottles will be executed before teh second-round of ResourceThrottles both in PlzNavigate and in the current architecture. Second, moving NavigationResourceHandler before the InterceptingResourceHandler allows to properly pause downloads & stream requests while we execute the NavigationThrottles on the UI thread. This was not possible before because InterceptingResourceHandler does not expect its next handler to pause the request in OnResponseStarted. Not pausing it is causing race conditions with the processing of the navigation on the UI thread. BUG=709771 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2847443002 Cr-Commit-Position: refs/heads/master@{#468692} Committed: https://chromium.googlesource.com/chromium/src/+/ea2f911cf6b0b023ff26e17bb9e5... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/ea2f911cf6b0b023ff26e17bb9e5... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
