Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2657813004:
PlzNavigate: set the error code in NavigationHandle on abort (Closed)
DescriptionPlzNavigate: set the error code in NavigationHandle on abort
This CL ensures that when a NavigationThrottle aborts a navigation, the
rigth error code of net::ERR_ABORTED will be set on the NavigationHandle
when PlzNavigate is enabled.
BUG=681029
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2657813004
Cr-Commit-Position: refs/heads/master@{#449030}
Committed: https://chromium.googlesource.com/chromium/src/+/54b81d6a37ba2a48bfd34bf42d954e4c8fe56d93
Patch Set 1 #Patch Set 2 : PlzNavigate: set the error code in NavigationHandle on abort #
Total comments: 8
Patch Set 3 : Rebase #Patch Set 4 : Addressed comments #Patch Set 5 : Rebase #Patch Set 6 : Reverted throttle result change #
Total comments: 6
Patch Set 7 : Rebase + addressed comments #
Messages
Total messages: 36 (23 generated)
Description was changed from ========== PlzNavigate: set the error code in NavigationHandle on abort This CL ensures that when a NavigationThrottle aborts a navigation, the rigth error code of net::ERR_ABORTED will be set on the NavigationHandle when PlzNavigate is enabled. BUG=681029 ========== to ========== PlzNavigate: set the error code in NavigationHandle on abort This CL ensures that when a NavigationThrottle aborts a navigation, the rigth error code of net::ERR_ABORTED will be set on the NavigationHandle when PlzNavigate is enabled. BUG=681029 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== PlzNavigate: set the error code in NavigationHandle on abort This CL ensures that when a NavigationThrottle aborts a navigation, the rigth error code of net::ERR_ABORTED will be set on the NavigationHandle when PlzNavigate is enabled. BUG=681029 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: set the error code in NavigationHandle on abort This CL ensures that when a NavigationThrottle aborts a navigation, the rigth error code of net::ERR_ABORTED will be set on the NavigationHandle when PlzNavigate is enabled. BUG=681029 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
clamy@chromium.org changed reviewers: + nasko@chromium.org, rdevlin.cronin@chromium.org
@nasko: PTAL @redvlin: PTAL at the changes in extension. https://codereview.chromium.org/2657813004/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2657813004/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:578: OnRequestFailed(false, net::ERR_ABORTED); OnRequestFailed will set the error code on the NavigationHandle & delete the pending NavigationEntry if needed. https://codereview.chromium.org/2657813004/diff/20001/extensions/browser/exte... File extensions/browser/extension_navigation_throttle.cc (right): https://codereview.chromium.org/2657813004/diff/20001/extensions/browser/exte... extensions/browser/extension_navigation_throttle.cc:93: return content::NavigationThrottle::BLOCK_REQUEST; Some tests expect this particular return code when this is triggered. I'm not sure how they passed previously.
https://codereview.chromium.org/2657813004/diff/20001/extensions/browser/exte... File extensions/browser/extension_navigation_throttle.cc (right): https://codereview.chromium.org/2657813004/diff/20001/extensions/browser/exte... extensions/browser/extension_navigation_throttle.cc:93: return content::NavigationThrottle::BLOCK_REQUEST; On 2017/01/26 15:33:45, clamy wrote: > Some tests expect this particular return code when this is triggered. I'm not > sure how they passed previously. Which tests? I wonder if the return on line 64 should also be returning BLOCK_REQUEST instead of cancel?
https://codereview.chromium.org/2657813004/diff/20001/extensions/browser/exte... File extensions/browser/extension_navigation_throttle.cc (right): https://codereview.chromium.org/2657813004/diff/20001/extensions/browser/exte... extensions/browser/extension_navigation_throttle.cc:93: return content::NavigationThrottle::BLOCK_REQUEST; On 2017/01/26 15:50:17, Devlin (catching up) wrote: > On 2017/01/26 15:33:45, clamy wrote: > > Some tests expect this particular return code when this is triggered. I'm not > > sure how they passed previously. > > Which tests? I wonder if the return on line 64 should also be returning > BLOCK_REQUEST instead of cancel? The issue was with WebViewTests/WebViewTest.Shim_TestLoadAbortChromeExtensionURLWrongPartition/0.
https://codereview.chromium.org/2657813004/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2657813004/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl_browsertest.cc:896: NavigationThrottle::PROCEED, NavigationThrottle::CANCEL_AND_IGNORE); nit: Should we also ensure that if we use BLOCK_REQUEST we get the expected error code? https://codereview.chromium.org/2657813004/diff/20001/extensions/browser/exte... File extensions/browser/extension_navigation_throttle.cc (right): https://codereview.chromium.org/2657813004/diff/20001/extensions/browser/exte... extensions/browser/extension_navigation_throttle.cc:93: return content::NavigationThrottle::BLOCK_REQUEST; On 2017/01/26 15:58:06, clamy wrote: > On 2017/01/26 15:50:17, Devlin (catching up) wrote: > > On 2017/01/26 15:33:45, clamy wrote: > > > Some tests expect this particular return code when this is triggered. I'm > not > > > sure how they passed previously. > > > > Which tests? I wonder if the return on line 64 should also be returning > > BLOCK_REQUEST instead of cancel? > > The issue was with > WebViewTests/WebViewTest.Shim_TestLoadAbortChromeExtensionURLWrongPartition/0. Let's ensure that we pick the right error code we believe this code should be returning. Tests can be fixed too. There is non-trivial difference between cancelling and blocking, which one is the semantically correct here?
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...
Thanks! https://codereview.chromium.org/2657813004/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2657813004/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl_browsertest.cc:896: NavigationThrottle::PROCEED, NavigationThrottle::CANCEL_AND_IGNORE); On 2017/01/26 17:20:18, nasko wrote: > nit: Should we also ensure that if we use BLOCK_REQUEST we get the expected > error code? Done. https://codereview.chromium.org/2657813004/diff/20001/extensions/browser/exte... File extensions/browser/extension_navigation_throttle.cc (right): https://codereview.chromium.org/2657813004/diff/20001/extensions/browser/exte... extensions/browser/extension_navigation_throttle.cc:93: return content::NavigationThrottle::BLOCK_REQUEST; On 2017/01/26 17:20:18, nasko wrote: > On 2017/01/26 15:58:06, clamy wrote: > > On 2017/01/26 15:50:17, Devlin (catching up) wrote: > > > On 2017/01/26 15:33:45, clamy wrote: > > > > Some tests expect this particular return code when this is triggered. I'm > > not > > > > sure how they passed previously. > > > > > > Which tests? I wonder if the return on line 64 should also be returning > > > BLOCK_REQUEST instead of cancel? > > > > The issue was with > > WebViewTests/WebViewTest.Shim_TestLoadAbortChromeExtensionURLWrongPartition/0. > > Let's ensure that we pick the right error code we believe this code should be > returning. Tests can be fixed too. There is non-trivial difference between > cancelling and blocking, which one is the semantically correct here? I've modified the other return, still letting the tests run to see if everything is ok with it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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...
@nasko, devlin: PTAL https://codereview.chromium.org/2657813004/diff/100001/extensions/browser/ext... File extensions/browser/extension_navigation_throttle.cc (right): https://codereview.chromium.org/2657813004/diff/100001/extensions/browser/ext... extensions/browser/extension_navigation_throttle.cc:93: return content::NavigationThrottle::BLOCK_REQUEST; I went back to modifying just this return. After further research, this seems to be the right result to return. As explained in the comment above, this particular code path corresponds to the checks performed in extensions::url_request_util::AllowCrossRendererResourceLoad when PlzNavigate is disabled. Those are called by ExtensionProtocolHandler::MaybeCreateJob which will cancel the request with an error code of net::ERR_BLOCKED_BY_CLIENT when the checks fail (https://cs.chromium.org/chromium/src/extensions/browser/extension_protocols.c...). So returning content::NavigationThrottle::BLOCK_REQUEST matches what the code is currently doing.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with a couple of comments. https://codereview.chromium.org/2657813004/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2657813004/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:909: shell()->web_contents(), NavigationThrottle::BLOCK_REQUEST, I would've expected the cases will include BLOCK_REQUEST for all three possible callbacks. Is it the case that we can't block from the other two? https://codereview.chromium.org/2657813004/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2657813004/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:586: // DO NOT ADD CODE after this. The previous call to OnRequestFailed has nit: I'd add an empty line above the comment to make it easier to see.
https://codereview.chromium.org/2657813004/diff/100001/extensions/browser/ext... File extensions/browser/extension_navigation_throttle.cc (right): https://codereview.chromium.org/2657813004/diff/100001/extensions/browser/ext... extensions/browser/extension_navigation_throttle.cc:93: return content::NavigationThrottle::BLOCK_REQUEST; On 2017/02/07 11:58:28, clamy (slow - catching up) wrote: > I went back to modifying just this return. After further research, this seems to > be the right result to return. As explained in the comment above, this > particular code path corresponds to the checks performed in > extensions::url_request_util::AllowCrossRendererResourceLoad when PlzNavigate is > disabled. Those are called by ExtensionProtocolHandler::MaybeCreateJob which > will cancel the request with an error code of net::ERR_BLOCKED_BY_CLIENT when > the checks fail > (https://cs.chromium.org/chromium/src/extensions/browser/extension_protocols.c...). > So returning content::NavigationThrottle::BLOCK_REQUEST matches what the code is > currently doing. FWIW, when I originally ported this I first tried block_request to match the other code, but because of the behavior this cl is fixing, I had to change it to cancel to pass.
extensions lgtm
Thanks! https://codereview.chromium.org/2657813004/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2657813004/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:909: shell()->web_contents(), NavigationThrottle::BLOCK_REQUEST, On 2017/02/07 21:25:25, nasko (very slow) wrote: > I would've expected the cases will include BLOCK_REQUEST for all three possible > callbacks. Is it the case that we can't block from the other two? We can only use NavigationThrottle::BLOCK_REQUEST on the request start on the main frame. It would work on redirects when PlzNavigate is enabled, but currently it only works for subframes. It doesn't work on WillprocessResponse where we should be using BLOCK_RESPONSE instead. I am going to submit as is, since this blocking two other people, and will expend test coverage later. https://codereview.chromium.org/2657813004/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2657813004/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:586: // DO NOT ADD CODE after this. The previous call to OnRequestFailed has On 2017/02/07 21:25:26, nasko (very slow) wrote: > nit: I'd add an empty line above the comment to make it easier to see. Done.
The CQ bit was checked by clamy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2657813004/#ps120001 (title: "Rebase + addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1486571666142670,
"parent_rev": "fe8d0ed2e37f97f330ac27cffc38d4ae061cdd79", "commit_rev":
"54b81d6a37ba2a48bfd34bf42d954e4c8fe56d93"}
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: set the error code in NavigationHandle on abort This CL ensures that when a NavigationThrottle aborts a navigation, the rigth error code of net::ERR_ABORTED will be set on the NavigationHandle when PlzNavigate is enabled. BUG=681029 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: set the error code in NavigationHandle on abort This CL ensures that when a NavigationThrottle aborts a navigation, the rigth error code of net::ERR_ABORTED will be set on the NavigationHandle when PlzNavigate is enabled. BUG=681029 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2657813004 Cr-Commit-Position: refs/heads/master@{#449030} Committed: https://chromium.googlesource.com/chromium/src/+/54b81d6a37ba2a48bfd34bf42d95... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/54b81d6a37ba2a48bfd34bf42d95... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
