Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1616943003:
Teach navigation throttles how to cancel requests in WillProcessResponse. (Closed)
|
Created:
4 years, 11 months ago by Mike West Modified:
4 years, 10 months ago CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, loading-reviews_chromium.org, nasko+codewatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTeach navigation throttles how to cancel requests in WillProcessResponse.
This is the first step towards moving various response-time checks up
from Blink into the browser. To support things like 'X-Frame-Options',
we'll need to do throttle checks after we can evaluate headers. This
patch starts us down that path by teaching NavigationHandleImpl,
NavigationResourceThrottle, and NavigationThrottle about response-time
checks. This patch shouldn't introduce any behavioral changes, as no
NavigationThrottle actually uses the new functionality yet.
BUG=555418
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/409b51d438ea3f99d37025372adaf4d730553857
Cr-Commit-Position: refs/heads/master@{#372332}
Patch Set 1 #Patch Set 2 : PlzNavigate #Patch Set 3 : Fiddling. #
Total comments: 28
Patch Set 4 : Feedback. #
Total comments: 14
Patch Set 5 : Feedback. #
Total comments: 5
Patch Set 6 : Feedback. #
Total comments: 1
Patch Set 7 : Rebase. #
Messages
Total messages: 45 (13 generated)
mkwst@chromium.org changed reviewers: + clamy@chromium.org
In the interests of smaller patches that march towards something reasonable, I've extracted this out of the mess I'm making in https://codereview.chromium.org/1530393003. clamy@: Would you mind taking a look at the whole thing? I'll find folks to stamp the bits you don't own once you're happy with the implementation. Thanks!
On 2016/01/21 at 13:22:58, Mike West wrote: > In the interests of smaller patches that march towards something reasonable, I've extracted this out of the mess I'm making in https://codereview.chromium.org/1530393003. > > clamy@: Would you mind taking a look at the whole thing? I'll find folks to stamp the bits you don't own once you're happy with the implementation. > > Thanks! Looks like some download browsertests are failing that didn't fail in the initial patchset. Suggestions for places to dig in tomorrow would be ever so welcome. :)
mkwst@chromium.org changed reviewers: + davidben@chromium.org, nasko@chromium.org
On 2016/01/21 17:02:55, Mike West wrote: > On 2016/01/21 at 13:22:58, Mike West wrote: > > In the interests of smaller patches that march towards something reasonable, > I've extracted this out of the mess I'm making in > https://codereview.chromium.org/1530393003. > > > > clamy@: Would you mind taking a look at the whole thing? I'll find folks to > stamp the bits you don't own once you're happy with the implementation. > > > > Thanks! > > Looks like some download browsertests are failing that didn't fail in the > initial patchset. Suggestions for places to dig in tomorrow would be ever so > welcome. :) In general, downloads don't commit and should not be generating navigations. Since you are adding code to the stage right around where we make decisions whether the response is a download or not, I'd look at ThrottlingResourceHandler and its ordering in the list of ResourceHandlers in the ResourceDispatcherHost. I am not surprised to see issues, as we haven't needed to intercept navigations right before commit until now, so potential ordering issues in that area were never exposed.
On 2016/01/21 17:08:27, nasko wrote: > On 2016/01/21 17:02:55, Mike West wrote: > > On 2016/01/21 at 13:22:58, Mike West wrote: > > > In the interests of smaller patches that march towards something reasonable, > > I've extracted this out of the mess I'm making in > > https://codereview.chromium.org/1530393003. > > > > > > clamy@: Would you mind taking a look at the whole thing? I'll find folks to > > stamp the bits you don't own once you're happy with the implementation. > > > > > > Thanks! > > > > Looks like some download browsertests are failing that didn't fail in the > > initial patchset. Suggestions for places to dig in tomorrow would be ever so > > welcome. :) > > In general, downloads don't commit and should not be generating navigations. > Since you are adding code to the stage right around where we make decisions > whether the response is a download or not, I'd look at ThrottlingResourceHandler > and its ordering in the list of ResourceHandlers in the ResourceDispatcherHost. > I am not surprised to see issues, as we haven't needed to intercept navigations > right before commit until now, so potential ordering issues in that area were > never exposed. ThrottlingResourceHandler happens before MimeTypeResourceHandler, so if a NavigationThrottle hook is not supposed to see downloads, NavigationResourceThrottle would need to happen in the other order. Unfortunately, MimeTypeResourceHandler itself has annoying assumptions that its downstream handlers never defer OnResponseStarted. We probably need to tweak that logic in that case. :-/ https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo...
> > > Looks like some download browsertests are failing that didn't fail in the > > > initial patchset. Suggestions for places to dig in tomorrow would be ever so > > > welcome. :) This turned out to be a small mistake on my part. The current patch passes all existing tests (and looks like it works for the non-PlzNavigate case with https://codereview.chromium.org/1617043002). I talked with clamy@ this morning about this patch, and it sounds like she's interested in picking up the PlzNavigate pieces, as they end up being complicated. clamy@, did you want me to drop this entire patch in favor of yours? Or did you want to land the non-PlzNavigate bits (basically this patch minus the changes to NavigationRequest)?
I was more thinking of making a PlzNavigate specific patch that enables the new stuff in the interface your patch introduces. I think you can go ahead with the interface changes and their support in the current architecture, and I'll handle making sure it works when PlzNavigate is enabled.
On 2016/01/22 at 13:06:00, clamy wrote: > I was more thinking of making a PlzNavigate specific patch that enables the new stuff in the interface your patch introduces. I think you can go ahead with the interface changes and their support in the current architecture, and I'll handle making sure it works when PlzNavigate is enabled. Ok. Then I think this is a reasonable start; would you mind taking a closer look at the c/b/frame_host portion, clamy@? mmenke@, could you put c/b/loader on your list? nasko@: c/public/browser? Thanks!
mkwst@chromium.org changed reviewers: + mmenke@chromium.org
+mmenke for reals.
Thanks! A few comments. https://codereview.chromium.org/1616943003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1616943003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:292: DCHECK(result != NavigationThrottle::DEFER); This should match the other checks: // If the navigation is not deferred, run the callback. if (result != NavigationThrottle::DEFER) RunCompleteCallback(result); Since we're introducing the API, some user of the API could decide to defer the navigation at that step. https://codereview.chromium.org/1616943003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:401: case NavigationThrottle::DEFER: Since we're introducing it in the public API, we need to handle that case as well, as in the function just above. https://codereview.chromium.org/1616943003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1616943003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.h:164: // |callback| will be callwed when all throttle checks have completed, nit: s/callwed/called https://codereview.chromium.org/1616943003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.h:166: // MUST NOT be called with a result of DEFER. s/MUST NOT/will not -> NavigationHandle makes a guarantee that it won't call |callback| with a result of DEFER. https://codereview.chromium.org/1616943003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.h:201: WILL_PROCESS_RESPONSE, Can you add a DEFFERRING_PROCESS_RESPONSE state as well, to match the other checks? https://codereview.chromium.org/1616943003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl_unittest.cc (right): https://codereview.chromium.org/1616943003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl_unittest.cc:233: // WillProcessResponse cannot be deferred. The callback will be called. As mentioned in other comments, I'd like if WillProcessResponse could defer the navigation. https://codereview.chromium.org/1616943003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_request.h (right): https://codereview.chromium.org/1616943003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_request.h:164: void OnResponseChecksComplete(NavigationThrottle::ThrottleCheckResult result); Let's remove the PlzNavigate changes since I'll be working on those. NavigationRequest is a PlzNavigate specific file, so it should not be modified. https://codereview.chromium.org/1616943003/diff/40001/content/browser/loader/... File content/browser/loader/navigation_resource_throttle.cc (right): https://codereview.chromium.org/1616943003/diff/40001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:46: render_frame_host->navigation_handle()->ReadyToCommitNavigation( Could we make this call part of NavigationHandle: when it's done with WillProcessResponse and it's not canceled, it can call ReadyToCommitNavigation internally? https://codereview.chromium.org/1616943003/diff/40001/content/public/browser/... File content/public/browser/navigation_throttle.h (right): https://codereview.chromium.org/1616943003/diff/40001/content/public/browser/... content/public/browser/navigation_throttle.h:64: // CANCEL_AND_IGNORE and perform the destruction asynchronously. The Let's remove last condition. There is one known case of a Throttle that will need to use it (SafeBrowsing on Android). Also content/public question here for nasko: we're not supposed to land something in the public API if it's not used outside of content. However, there's no impl version of the NavigationThrottle, since it's an interface that's meant to be implemented outside of content. How do we deal with that case?
Only looked at loader/ (Wish I had more time to better understand navigation, but alas... I'll have to defer to the other reviewers there) https://codereview.chromium.org/1616943003/diff/40001/content/browser/loader/... File content/browser/loader/navigation_resource_throttle.cc (right): https://codereview.chromium.org/1616943003/diff/40001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:40: DCHECK_CURRENTLY_ON(BrowserThread::UI); Should include browser_thread header. https://codereview.chromium.org/1616943003/diff/40001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:41: CHECK(result != NavigationThrottle::DEFER); CHECK_NE is generally preferred. This file should also include base/logging.h https://codereview.chromium.org/1616943003/diff/40001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:115: void WillProcessResponseOnUIThread( This is an old method...but was never being used before? https://codereview.chromium.org/1616943003/diff/40001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:124: SendCheckResultToIOThread(callback, NavigationThrottle::PROCEED); If there's no RFH, should we really just proceed? https://codereview.chromium.org/1616943003/diff/40001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:131: SendCheckResultToIOThread(callback, NavigationThrottle::PROCEED); Again, when does this happen?
Description was changed from ========== Teach navigation throttles how to cancel requests in WillProcessResponse. This is the first step towards moving various response-time checks up from Blink into the browser. To support things like 'X-Frame-Options', we'll need to do throttle checks after we can evaluate headers. This patch starts us down that path by teaching NavigationHandleImpl, NavigationResourceThrottle, and NavigationThrottle about response-time checks. This patch shouldn't introduce any behavioral changes, as no NavigationThrottle actually uses the new functionality yet. BUG=555418 ========== to ========== Teach navigation throttles how to cancel requests in WillProcessResponse. This is the first step towards moving various response-time checks up from Blink into the browser. To support things like 'X-Frame-Options', we'll need to do throttle checks after we can evaluate headers. This patch starts us down that path by teaching NavigationHandleImpl, NavigationResourceThrottle, and NavigationThrottle about response-time checks. This patch shouldn't introduce any behavioral changes, as no NavigationThrottle actually uses the new functionality yet. BUG=555418 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1616943003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1616943003/60001
Thanks clamy@ and mmenke@! I've addressed feedback below. clamy@, I'd appreciate it if you could help with mmenke@'s questions about the presence of the NavigationHandle and RFH in NavigationResourceThrottle. :) https://codereview.chromium.org/1616943003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1616943003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:292: DCHECK(result != NavigationThrottle::DEFER); On 2016/01/25 at 16:36:21, clamy wrote: > This should match the other checks: > // If the navigation is not deferred, run the callback. > if (result != NavigationThrottle::DEFER) > RunCompleteCallback(result); > > Since we're introducing the API, some user of the API could decide to defer the navigation at that step. Done. https://codereview.chromium.org/1616943003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:401: case NavigationThrottle::DEFER: On 2016/01/25 at 16:36:21, clamy wrote: > Since we're introducing it in the public API, we need to handle that case as well, as in the function just above. Done. https://codereview.chromium.org/1616943003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1616943003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.h:164: // |callback| will be callwed when all throttle checks have completed, On 2016/01/25 at 16:36:21, clamy wrote: > nit: s/callwed/called Thanks! :) https://codereview.chromium.org/1616943003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.h:166: // MUST NOT be called with a result of DEFER. On 2016/01/25 at 16:36:21, clamy wrote: > s/MUST NOT/will not -> NavigationHandle makes a guarantee that it won't call |callback| with a result of DEFER. Done. https://codereview.chromium.org/1616943003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.h:201: WILL_PROCESS_RESPONSE, On 2016/01/25 at 16:36:21, clamy wrote: > Can you add a DEFFERRING_PROCESS_RESPONSE state as well, to match the other checks? Done. https://codereview.chromium.org/1616943003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_request.h (right): https://codereview.chromium.org/1616943003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_request.h:164: void OnResponseChecksComplete(NavigationThrottle::ThrottleCheckResult result); On 2016/01/25 at 16:36:21, clamy wrote: > Let's remove the PlzNavigate changes since I'll be working on those. NavigationRequest is a PlzNavigate specific file, so it should not be modified. Done. https://codereview.chromium.org/1616943003/diff/40001/content/browser/loader/... File content/browser/loader/navigation_resource_throttle.cc (right): https://codereview.chromium.org/1616943003/diff/40001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:40: DCHECK_CURRENTLY_ON(BrowserThread::UI); On 2016/01/25 at 17:57:33, mmenke wrote: > Should include browser_thread header. Done. https://codereview.chromium.org/1616943003/diff/40001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:41: CHECK(result != NavigationThrottle::DEFER); On 2016/01/25 at 17:57:33, mmenke wrote: > CHECK_NE is generally preferred. This file should also include base/logging.h Done. https://codereview.chromium.org/1616943003/diff/40001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:46: render_frame_host->navigation_handle()->ReadyToCommitNavigation( On 2016/01/25 at 16:36:21, clamy wrote: > Could we make this call part of NavigationHandle: when it's done with WillProcessResponse and it's not canceled, it can call ReadyToCommitNavigation internally? It made more sense to me for NavigationResourceThrottle to remain in control of the flow, and WillProcessResponse calling ReadyToCommitNavigation felt a little too magical. *shrug* I don't feel strongly about it; if you do, I'll move it. :) https://codereview.chromium.org/1616943003/diff/40001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:115: void WillProcessResponseOnUIThread( On 2016/01/25 at 17:57:33, mmenke wrote: > This is an old method...but was never being used before? It was being used, see the PostTask in NavigationResourceThrottle::WillProcessResponse. We're just adding a callback method to its signature. https://codereview.chromium.org/1616943003/diff/40001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:124: SendCheckResultToIOThread(callback, NavigationThrottle::PROCEED); On 2016/01/25 at 17:57:33, mmenke wrote: > If there's no RFH, should we really just proceed? This matches the existing behavior in the other methods; if we don't post something, we'll just hang while the IO thread waits for the UI thread to finish its checks. It might make sense to return CANCEL instead if we're not able to do checks, but I'm honestly not familiar enough with the lifetime of the RenderFrameHost/NavigationHandle to give a solid answer. If it's possible to DCHECK instead, I'd be happy doing that. clamy@? https://codereview.chromium.org/1616943003/diff/40001/content/public/browser/... File content/public/browser/navigation_throttle.h (right): https://codereview.chromium.org/1616943003/diff/40001/content/public/browser/... content/public/browser/navigation_throttle.h:64: // CANCEL_AND_IGNORE and perform the destruction asynchronously. The On 2016/01/25 at 16:36:21, clamy wrote: > Let's remove last condition. There is one known case of a Throttle that will need to use it (SafeBrowsing on Android). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Thanks! The c/b/frame_host part is looking mostly good. A few comments below. https://codereview.chromium.org/1616943003/diff/40001/content/browser/loader/... File content/browser/loader/navigation_resource_throttle.cc (right): https://codereview.chromium.org/1616943003/diff/40001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:124: SendCheckResultToIOThread(callback, NavigationThrottle::PROCEED); On 2016/01/26 09:41:42, Mike West wrote: > On 2016/01/25 at 17:57:33, mmenke wrote: > > If there's no RFH, should we really just proceed? > > This matches the existing behavior in the other methods; if we don't post > something, we'll just hang while the IO thread waits for the UI thread to finish > its checks. It might make sense to return CANCEL instead if we're not able to do > checks, but I'm honestly not familiar enough with the lifetime of the > RenderFrameHost/NavigationHandle to give a solid answer. > > If it's possible to DCHECK instead, I'd be happy doing that. clamy@? I think it's possible for downloads started from the browser. They won't be associated with a RFH, but we should let them proceed anyway. https://codereview.chromium.org/1616943003/diff/40001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:131: SendCheckResultToIOThread(callback, NavigationThrottle::PROCEED); On 2016/01/25 17:57:33, mmenke wrote: > Again, when does this happen? Actually this should never happen. We should probably replace it with a DCHECK(navigation_handle), and in other instances as well. This is a mistake in the original code. https://codereview.chromium.org/1616943003/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1616943003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:429: // No code after running the calback, as it might result in our destruction. nit: callback Good find! :) https://codereview.chromium.org/1616943003/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl_unittest.cc (right): https://codereview.chromium.org/1616943003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl_unittest.cc:28: void SetResult(NavigationThrottle::ThrottleCheckResult result) { Is this change still used in the unit tests ? https://codereview.chromium.org/1616943003/diff/60001/content/browser/loader/... File content/browser/loader/navigation_resource_throttle.cc (right): https://codereview.chromium.org/1616943003/diff/60001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:31: CHECK_NE(result, NavigationThrottle::DEFER); DCHECK? I think we're trying to avoid CHECKs in the code.
https://codereview.chromium.org/1616943003/diff/60001/content/browser/loader/... File content/browser/loader/navigation_resource_throttle.cc (right): https://codereview.chromium.org/1616943003/diff/60001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:43: CHECK_NE(result, NavigationThrottle::DEFER); DCHECK here, as well, I assume. https://codereview.chromium.org/1616943003/diff/60001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:49: render_frame_host, headers); This seems weird. We call into the NavigationHandle, and tell it to call us back. If it calls us back, and it told us to proceed, we then call *back* into the NavigationHandle, telling it that it told us to proceed? https://codereview.chromium.org/1616943003/diff/60001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:74: if (!navigation_handle) { How certain are we that this is "our" navigation handle, and not the handle for a new navigation? https://codereview.chromium.org/1616943003/diff/60001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:131: render_frame_host->navigation_handle(); How sure are we that this is our navigation handle, and not that for a new navigation? I don't see a cancellation path. https://codereview.chromium.org/1616943003/diff/60001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:140: render_process_id, render_frame_host_id, headers)); Hrm...Does seem kinda weird to me that the NavigationHandle doesn't know its own render frame IDs
https://codereview.chromium.org/1616943003/diff/60001/content/browser/loader/... File content/browser/loader/navigation_resource_throttle.cc (right): https://codereview.chromium.org/1616943003/diff/60001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:74: if (!navigation_handle) { On 2016/01/26 16:40:08, mmenke wrote: > How certain are we that this is "our" navigation handle, and not the handle for > a new navigation? Because the ResourceRequest is started after the browser creates the NavigationHandle and stopped before it destroys it. https://codereview.chromium.org/1616943003/diff/60001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:140: render_process_id, render_frame_host_id, headers)); On 2016/01/26 16:40:08, mmenke wrote: > Hrm...Does seem kinda weird to me that the NavigationHandle doesn't know its own > render frame IDs This is because the NavigationHandle was built as an interface that works both with the current navigation architecture and with PlzNavigate. In PlzNavigate you only get a RFH after processing the response, so the NavigationHandle does not know it before that point.
On 2016/01/26 16:48:05, clamy wrote: > https://codereview.chromium.org/1616943003/diff/60001/content/browser/loader/... > File content/browser/loader/navigation_resource_throttle.cc (right): > > https://codereview.chromium.org/1616943003/diff/60001/content/browser/loader/... > content/browser/loader/navigation_resource_throttle.cc:74: if > (!navigation_handle) { > On 2016/01/26 16:40:08, mmenke wrote: > > How certain are we that this is "our" navigation handle, and not the handle > for > > a new navigation? > > Because the ResourceRequest is started after the browser creates the > NavigationHandle and stopped before it destroys it. > > https://codereview.chromium.org/1616943003/diff/60001/content/browser/loader/... > content/browser/loader/navigation_resource_throttle.cc:140: render_process_id, > render_frame_host_id, headers)); > On 2016/01/26 16:40:08, mmenke wrote: > > Hrm...Does seem kinda weird to me that the NavigationHandle doesn't know its > own > > render frame IDs > > This is because the NavigationHandle was built as an interface that works both > with the current navigation architecture and with PlzNavigate. In PlzNavigate > you only get a RFH after processing the response, so the NavigationHandle does > not know it before that point. Thanks for the explanations!
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Thanks, clamy@ and mmenke@. I hope that the latest patchset addresses your feedback. https://codereview.chromium.org/1616943003/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl_unittest.cc (right): https://codereview.chromium.org/1616943003/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl_unittest.cc:28: void SetResult(NavigationThrottle::ThrottleCheckResult result) { On 2016/01/26 at 14:08:55, clamy wrote: > Is this change still used in the unit tests ? Nope. Removed, thanks! https://codereview.chromium.org/1616943003/diff/60001/content/browser/loader/... File content/browser/loader/navigation_resource_throttle.cc (right): https://codereview.chromium.org/1616943003/diff/60001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:31: CHECK_NE(result, NavigationThrottle::DEFER); On 2016/01/26 at 14:08:55, clamy wrote: > DCHECK? I think we're trying to avoid CHECKs in the code. Done. https://codereview.chromium.org/1616943003/diff/60001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:43: CHECK_NE(result, NavigationThrottle::DEFER); On 2016/01/26 at 16:40:08, mmenke wrote: > DCHECK here, as well, I assume. Done. https://codereview.chromium.org/1616943003/diff/60001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:49: render_frame_host, headers); On 2016/01/26 at 16:40:08, mmenke wrote: > This seems weird. We call into the NavigationHandle, and tell it to call us back. If it calls us back, and it told us to proceed, we then call *back* into the NavigationHandle, telling it that it told us to proceed? As I noted in my response to clamy@, it made sense to me to think of the NavigationResourceThrottle as being in control of the navigation, and the NavigationHandle as something of a state machine. But both of you have flagged this as being weird, so I'm happy to agree. :)
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1616943003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1616943003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Thanks! Lgtm with a few nits. https://codereview.chromium.org/1616943003/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1616943003/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:297: // If we're about to proceed, then we're ready to commit. nit: // A proceeding navigation is now about to commit. ? (I think we try to avoid using we in comments). https://codereview.chromium.org/1616943003/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:434: // No code after running the calback, as it might result in our destruction. nit: s/calback/callback https://codereview.chromium.org/1616943003/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl_unittest.cc (right): https://codereview.chromium.org/1616943003/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl_unittest.cc:138: main_test_rfh(), Now that we're adding this, could you add a check in the test where the navigation proceeds that NavigationHandle::GetRenderFrameHost() returns main_rfh if the result was proceed?
LGTM https://codereview.chromium.org/1616943003/diff/80001/content/browser/loader/... File content/browser/loader/navigation_resource_throttle.cc (right): https://codereview.chromium.org/1616943003/diff/80001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:8: #include "base/logging.h" While you're here, should also include base/bind.h and base/location.h
LGTM once all reviewers nits are resolved. https://codereview.chromium.org/1616943003/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1616943003/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:434: // No code after running the calback, as it might result in our destruction. nit: "might have resulted"?
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1616943003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1616943003/100001
https://codereview.chromium.org/1616943003/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1616943003/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:166: ReadyToCommitNavigation(render_frame_host_, response_headers_); This is a substantive change from the last patchset to fix a bug uncovered by the test clamy@ asked for. Thanks, clamy@! Mind taking another look?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Thanks! Lgtm
The CQ bit was checked by mkwst@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/1616943003/#ps100001 (title: "Feedback.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1616943003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1616943003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Teach navigation throttles how to cancel requests in WillProcessResponse. This is the first step towards moving various response-time checks up from Blink into the browser. To support things like 'X-Frame-Options', we'll need to do throttle checks after we can evaluate headers. This patch starts us down that path by teaching NavigationHandleImpl, NavigationResourceThrottle, and NavigationThrottle about response-time checks. This patch shouldn't introduce any behavioral changes, as no NavigationThrottle actually uses the new functionality yet. BUG=555418 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Teach navigation throttles how to cancel requests in WillProcessResponse. This is the first step towards moving various response-time checks up from Blink into the browser. To support things like 'X-Frame-Options', we'll need to do throttle checks after we can evaluate headers. This patch starts us down that path by teaching NavigationHandleImpl, NavigationResourceThrottle, and NavigationThrottle about response-time checks. This patch shouldn't introduce any behavioral changes, as no NavigationThrottle actually uses the new functionality yet. BUG=555418 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/409b51d438ea3f99d37025372adaf4d730553857 Cr-Commit-Position: refs/heads/master@{#372332} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/409b51d438ea3f99d37025372adaf4d730553857 Cr-Commit-Position: refs/heads/master@{#372332}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/1645363002/ by mkwst@chromium.org. The reason for reverting is: Windows tree doesn't like me: https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
