Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2510083003:
PlzNavigate: properly update NavigationHandle for 204/205 and download responses (Closed)
|
Created:
4 years, 1 month ago by yzshen1 Modified:
3 years, 11 months ago Reviewers:
clamy CC:
chromium-reviews, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, Randy Smith (Not in Mondays), loading-reviews+metrics_chromium.org, csharrison+watch_chromium.org, darin-cc_chromium.org, loading-reviews_chromium.org, mmenke Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: properly update NavigationHandle for 204/205 and download responses.
This fixes the following browser test for PlzNavigate:
PageLoadMetricsBrowserTest.Ignore204Pages
PageLoadMetricsBrowserTest.IgnoreDownloads
BUG=652767
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #
Total comments: 3
Patch Set 4 : . #Patch Set 5 : . #Patch Set 6 : . #
Total comments: 7
Patch Set 7 : address comments of clamy@ #
Depends on Patchset: Messages
Total messages: 25 (18 generated)
Description was changed from ========== PlzNavigate: properly update NavigationHandle for 204/205 response BUG=652767 ========== to ========== PlzNavigate: properly update NavigationHandle for 204/205 response BUG=652767 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by yzshen@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: properly update NavigationHandle for 204/205 response BUG=652767 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: properly update NavigationHandle for 204/205 response This fixes the following browser test for PlzNavigate: PageLoadMetricsBrowserTest.Ignore204Pages BUG=652767 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
yzshen@chromium.org changed reviewers: + clamy@chromium.org
Hi, Camille. Would you please take a look? Thanks! https://codereview.chromium.org/2510083003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2510083003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:631: net_error_code_ = net::ERR_ABORTED; Changing the net error code seems weird. But it matches the behavior of non-PlzNavigate. Does it make sense?
Thanks! One comment below. I'm fine with setting the error code, the issue seems to be how it's done. See my comment below. https://codereview.chromium.org/2510083003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (left): https://codereview.chromium.org/2510083003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:387: response->head.headers->response_code() == 205)) { If the issue is just the error code in the NavigationHandle, you can do navigation_handle_->set_net_error_code(net::ERR_ABORTED) here, instead of moving this block. I think it is clearer.
Thanks, Camille! https://codereview.chromium.org/2510083003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (left): https://codereview.chromium.org/2510083003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:387: response->head.headers->response_code() == 205)) { On 2016/11/21 16:57:14, clamy (slow) wrote: > If the issue is just the error code in the NavigationHandle, you can do > navigation_handle_->set_net_error_code(net::ERR_ABORTED) here, instead of moving > this block. I think it is clearer. At least MetricsWebContentsObserver::DidFinishNavigation currently requires response headers to be set as well. Maybe there are/will be others needing such info. It seems more consistent to update the navigation handle using NavigationHandleImpl::WillProcessResponse. WDYT?
The CQ bit was checked by yzshen@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: properly update NavigationHandle for 204/205 response This fixes the following browser test for PlzNavigate: PageLoadMetricsBrowserTest.Ignore204Pages BUG=652767 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: properly update NavigationHandle for 204/205 and download responses. This fixes the following browser test for PlzNavigate: PageLoadMetricsBrowserTest.Ignore204Pages PageLoadMetricsBrowserTest.IgnoreDownloads BUG=652767 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Friendly ping, Camille. I also updated the CL to handle the download case. PTAL
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 yzshen@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_site_isolation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Thanks! A few comments below. https://codereview.chromium.org/2510083003/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2510083003/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:629: const bool is_204_205_response = I'm still not super happy about the code flow here for several reasons: 1) Architecturally, this function is supposed to go through all the throttles and check their results. It's weird to have some special case above which cancels things. I would prefer we cancel things in WillProcessResponse before we call this function. 2) More importantly, this will now cause us to possibly spawn a process in downloads and 204, 205s only to delete it moments later. This is quite wasteful IMO, so I'd like to avoid that. In order to avoid the issue above, I don't see a better way but to merge NavigationRequest & NavigationHandle. This is something I've been wanting to do eventually when PlzNavigate ships, so I'll have a try at it and see what it looks like. In the meantime, I'd suggest splitting the CL in two, with the part about propagating the is_download bit in one CL, and the part about handling 204/205s in NavigationHandle in another. https://codereview.chromium.org/2510083003/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2510083003/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:431: common_params_.should_replace_current_entry, is_download, false, Could we pass the values for is_stream and GlobalRequestID() while we're at it? The files to modify are the same. https://codereview.chromium.org/2510083003/diff/100001/content/browser/loader... File content/browser/loader/navigation_resource_handler.cc (right): https://codereview.chromium.org/2510083003/diff/100001/content/browser/loader... content/browser/loader/navigation_resource_handler.cc:130: if (!info->is_stream() && !info->IsDownload()) I think we should call DetachFromCore if the request is a download at this point. Otherwise we may end up trying to cancel the request if the NavigationURLLoader is deleted.
The CQ bit was checked by yzshen@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...
Thanks, Camille! https://codereview.chromium.org/2510083003/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2510083003/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:629: const bool is_204_205_response = On 2016/11/25 10:46:00, clamy (slow) wrote: > I'm still not super happy about the code flow here for several reasons: > 1) Architecturally, this function is supposed to go through all the throttles > and check their results. It's weird to have some special case above which > cancels things. I would prefer we cancel things in WillProcessResponse before we > call this function. Done. > 2) More importantly, this will now cause us to possibly spawn a process in > downloads and 204, 205s only to delete it moments later. This is quite wasteful > IMO, so I'd like to avoid that. It seems to me that one simple way to avoid this is to split the current NavigationHandleImpl::WillProcessResponse() into two calls, e.g. (better names needed): - UpdateResponseInfo() which just sets response header/is_download/is_stream/etc. and - ThrottleCheckForWillProcessResponse() which runs the throttle checks. And then in NavigationRequest::OnResponseStarted(), we do: ============================================= ... navigation_handle_->UpdateResponseInfo(...); if (navigation is 204/205 or download) { frame_tree_node_->navigator()->DiscardPendingEntryIfNeeded( navigation_handle_.get()); frame_tree_node_->ResetNavigationRequest(false); return; } // select appropriate renderer, maybe spawn new process. navigation_handle_->ThrottleCheckForWillProcessResponse(); ============================================= If merging NavigationRequest/NavigationHandle will take a while to happen, maybe we could do such a simple fix so that the issue won't block some browser tests or shipping PlzNavigation. > > In order to avoid the issue above, I don't see a better way but to merge > NavigationRequest & NavigationHandle. This is something I've been wanting to do > eventually when PlzNavigate ships, so I'll have a try at it and see what it > looks like. In the meantime, I'd suggest splitting the CL in two, with the part > about propagating the is_download bit in one CL, and the part about handling > 204/205s in NavigationHandle in another. I have created a separate CL for propagating is_download/is_stream/request_id: https://codereview.chromium.org/2537593002/ https://codereview.chromium.org/2510083003/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2510083003/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:431: common_params_.should_replace_current_entry, is_download, false, On 2016/11/25 10:46:00, clamy (slow) wrote: > Could we pass the values for is_stream and GlobalRequestID() while we're at it? > The files to modify are the same. Done. https://codereview.chromium.org/2510083003/diff/100001/content/browser/loader... File content/browser/loader/navigation_resource_handler.cc (right): https://codereview.chromium.org/2510083003/diff/100001/content/browser/loader... content/browser/loader/navigation_resource_handler.cc:130: if (!info->is_stream() && !info->IsDownload()) On 2016/11/25 10:46:00, clamy (slow) wrote: > I think we should call DetachFromCore if the request is a download at this > point. Otherwise we may end up trying to cancel the request if the > NavigationURLLoader is deleted. In practice it won't cause trouble because before returning to the message loop this NavigationResourceHandler object will be destructed -> it will try to notify the UI thread about the request failure -> when that task is run on the UI thread, the NavigationURLLoaderImpl has already been destroyed so it is a no-op. I agree that it is better not to rely on that. Thanks for the suggestion!
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_...)
https://codereview.chromium.org/2510083003/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2510083003/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:629: const bool is_204_205_response = On 2016/11/28 22:03:59, yzshen1 wrote: > On 2016/11/25 10:46:00, clamy (slow) wrote: > > I'm still not super happy about the code flow here for several reasons: > > 1) Architecturally, this function is supposed to go through all the throttles > > and check their results. It's weird to have some special case above which > > cancels things. I would prefer we cancel things in WillProcessResponse before > we > > call this function. > > Done. > > > 2) More importantly, this will now cause us to possibly spawn a process in > > downloads and 204, 205s only to delete it moments later. This is quite > wasteful > > IMO, so I'd like to avoid that. > > It seems to me that one simple way to avoid this is to split the current > NavigationHandleImpl::WillProcessResponse() into two calls, e.g. (better names > needed): > - UpdateResponseInfo() which just sets response > header/is_download/is_stream/etc. and > - ThrottleCheckForWillProcessResponse() which runs the throttle checks. > > And then in NavigationRequest::OnResponseStarted(), we do: > > ============================================= > ... > navigation_handle_->UpdateResponseInfo(...); > > if (navigation is 204/205 or download) { > frame_tree_node_->navigator()->DiscardPendingEntryIfNeeded( > navigation_handle_.get()); > frame_tree_node_->ResetNavigationRequest(false); > return; > } > > // select appropriate renderer, maybe spawn new process. > > navigation_handle_->ThrottleCheckForWillProcessResponse(); > ============================================= > > If merging NavigationRequest/NavigationHandle will take a while to happen, maybe > we could do such a simple fix so that the issue won't block some browser tests > or shipping PlzNavigation. > > > > > In order to avoid the issue above, I don't see a better way but to merge > > NavigationRequest & NavigationHandle. This is something I've been wanting to > do > > eventually when PlzNavigate ships, so I'll have a try at it and see what it > > looks like. In the meantime, I'd suggest splitting the CL in two, with the > part > > about propagating the is_download bit in one CL, and the part about handling > > 204/205s in NavigationHandle in another. > > I have created a separate CL for propagating is_download/is_stream/request_id: > https://codereview.chromium.org/2537593002/ Yes this is something we could do as well. Let me first see if I can get to a CL with the merge quickly enough, otnerwise we'll move to the code you suggested. Thanks! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
