PlzNavigate: Debug URLs are being handled via failed network requests |
||||
Issue descriptionChrome Version: 64.0.3244.1 OS: All What steps will reproduce the problem? (1) Visit any web page. (2) Type chrome://kill in the omnibox. What is the expected result? The browser process should classify the navigation using IsRendererDebugURL and send it straight to the current renderer. The renderer should handle it in MaybeHandleDebugURL via NavigateInternal. (This is what happens without PlzNavigate, with --disable-browser-side-navigation.) What happens instead? The browser process attempts to request chrome://kill from the network stack, which leads to a failed navigation. The failure is sent to the renderer process, and we get to MaybeHandleDebugURL via OnFailedNavigation. This is surprising behavior and makes it harder to fix bugs like issue 760732. (It was discovered in https://chromium-review.googlesource.com/c/chromium/src/+/723100.) We should try to have it go through NavigateInternal if possible. arthursonzogni@ or clamy@, can you help triage?
,
Oct 20 2017
Yes I'm aware of the behavior, and it always looked a bit strange to me. I agree we should change it. I suspect adding !IsRendererDebugURL to IsURLHandledByNetworkStack should do the trick.
,
Oct 20 2017
,
Oct 20 2017
So this is slightly more complicated. If we have the renderer debug URLs go through NavigateInternal, we will attempt to load the ones that commit (kChromeUIDumpURL). Since we didn't go through the network stack in the first place we don't have a stream to request. Instead, the renderer requests the URL and gets killed because it's not supposed to do a main resource request that's not a stream. I suppose we should load an error page or a blank document in that case, and not attempt to reach the network. What do you think?
,
Oct 20 2017
This is a draft of the CL where I encounter the issue: https://chromium-review.googlesource.com/c/chromium/src/+/731083.
,
Oct 20 2017
Comment 4: Yes, it seems like we should improve the behavior around the debug URLs that actually return and continue running (without crashing or permanently hanging). Looks like that includes at least chrome://crashdump and chrome://shorthang. Looks like we use an error page for them today, even with --disable-browser-side-navigation. That's maybe ok. Or we could try to handle it more like a javascript: URL where it doesn't commit anything at all, which might be preferable? Thanks for starting the CL!
,
Oct 23 2017
I think it'd make more sense for them not to commit at all.
,
Nov 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7fced7b524d0ea7a165b0686e8d459880e1639f5 commit 7fced7b524d0ea7a165b0686e8d459880e1639f5 Author: clamy <clamy@chromium.org> Date: Thu Nov 16 19:52:43 2017 Do not send renderer debug URLs to the network stack Sending renderer debug URLs to the network stack will result in trying to commit an error page, which is when the debug URL will actually be handled. This causing issues when trying to block them. This CL also ensures that debug URLs will never commit. BUG= 776528 ,760732 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation Change-Id: Iaae35029e5fcd0b66c470468a8f90ca9736fff3e Reviewed-on: https://chromium-review.googlesource.com/731083 Commit-Queue: Charlie Reis <creis@chromium.org> Reviewed-by: Ned Nguyen <nednguyen@google.com> Reviewed-by: Charlie Reis <creis@chromium.org> Cr-Commit-Position: refs/heads/master@{#517157} [modify] https://crrev.com/7fced7b524d0ea7a165b0686e8d459880e1639f5/content/browser/browser_url_handler_impl.cc [modify] https://crrev.com/7fced7b524d0ea7a165b0686e8d459880e1639f5/content/browser/frame_host/debug_urls.cc [modify] https://crrev.com/7fced7b524d0ea7a165b0686e8d459880e1639f5/content/browser/frame_host/debug_urls.h [modify] https://crrev.com/7fced7b524d0ea7a165b0686e8d459880e1639f5/content/browser/frame_host/navigation_controller_impl.cc [modify] https://crrev.com/7fced7b524d0ea7a165b0686e8d459880e1639f5/content/browser/frame_host/navigation_handle_impl.cc [modify] https://crrev.com/7fced7b524d0ea7a165b0686e8d459880e1639f5/content/browser/frame_host/navigator_impl.cc [modify] https://crrev.com/7fced7b524d0ea7a165b0686e8d459880e1639f5/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/7fced7b524d0ea7a165b0686e8d459880e1639f5/content/browser/frame_host/render_frame_host_manager_browsertest.cc [modify] https://crrev.com/7fced7b524d0ea7a165b0686e8d459880e1639f5/content/browser/site_instance_impl.cc [modify] https://crrev.com/7fced7b524d0ea7a165b0686e8d459880e1639f5/content/browser/webui/web_ui_controller_factory_registry.cc [modify] https://crrev.com/7fced7b524d0ea7a165b0686e8d459880e1639f5/content/public/common/url_constants.cc [modify] https://crrev.com/7fced7b524d0ea7a165b0686e8d459880e1639f5/content/public/common/url_constants.h [modify] https://crrev.com/7fced7b524d0ea7a165b0686e8d459880e1639f5/content/public/common/url_utils.cc [modify] https://crrev.com/7fced7b524d0ea7a165b0686e8d459880e1639f5/content/public/common/url_utils.h [modify] https://crrev.com/7fced7b524d0ea7a165b0686e8d459880e1639f5/content/public/test/navigation_simulator.cc [modify] https://crrev.com/7fced7b524d0ea7a165b0686e8d459880e1639f5/content/renderer/render_frame_impl.cc [modify] https://crrev.com/7fced7b524d0ea7a165b0686e8d459880e1639f5/content/test/content_browser_test_test.cc [modify] https://crrev.com/7fced7b524d0ea7a165b0686e8d459880e1639f5/content/test/test_web_contents.cc [modify] https://crrev.com/7fced7b524d0ea7a165b0686e8d459880e1639f5/tools/perf/core/stacktrace_unittest.py
,
Nov 17 2017
|
||||
►
Sign in to add a comment |
||||
Comment 1 by igorcov@chromium.org
, Oct 20 2017