Cancelling a cross-site navigation causes page to be unresponsive to input |
|||||
Issue descriptionChrome Version: 70.0.3501.0 OS: Android What steps will reproduce the problem? (1) Visit a page with links to other sites (e.g. news.ycombinator.com) (2) Optionally, use devtools remote debugging to throttle the network to make the following steps easier (3) Tap a link to start a cross-site navigation (4) Before the navigation commits, open the menu and tap the stop button (5) Attempt to interact with the existing page with the touchscreen What is the expected result? The page should respond to touch. What happens instead? The page does not respond to touch. This does not appear to happen for same-site navigations and I can only reproduce this with --site-per-process.
,
Jul 26
So for the desktop case, I don't think there's any issue with input per se, it's just that we're still showing the old document even though we've committed the navigation. The page stops responding to input as soon as the navigation commits. The old RWHV is destroyed and input events get routed to the new RWHV. This seems like sensible behaviour. Presumably, if the navigation is canceled after the commit, we should just show the new document even if it is effectively blank, otherwise we'll indefinitely show content on screen whose RWHV is gone. I'm still looking at the android case.
,
Jul 26
In the android case, after canceling the navigation before the commit, we start targeting the input events to a new RWHV instead of the existing one. If we go on to reload the original page, that new RWHV is destroyed, and then we go back to targeting events to the original RWHV.
,
Jul 30
So for the desktop case, the |new_content_rendering_timeout_| is never started, so we never stop showing the old document. We early return here before starting the timer: https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_impl.cc?rcl=1e4639d7ac2bc1fdd25427f4df7239906d5c918d&l=1205
,
Jul 31
For the android case, after the new RenderWidgetHostViewAndroid is created, it's added to the ViewAndroid hierarchy even before the navigation commits. So there are now two RWHVAs in the hierarchy. The ViewAndroid targeting for input events sends the events to the new RWHVA: https://cs.chromium.org/chromium/src/ui/android/view_android.cc?rcl=914a6a5785c362ebedfa611648dafde6b51fc8ad&l=591
,
Jul 31
Re #4: It is concerning that a navigation is committing without the timer starting. It would be interesting to know why not. fwiw, it sounds like the Android and desktop scenarios are two independent bugs.
,
Aug 2
+boliu +jinsuk in case an of our recent content refactors changed how touch propagation goes.
,
Aug 2
Android-specific comment: So WebContentsViewAndroid has a ViewAndroid (I'll call this parent), and RenderWidgetHostViewAndroid has a ViewAndroid (call this child). When a cross-site navigation happens, the WCVA swaps to a new RWHVA, there is a moment in the when parent ViewAndroid has two children. The new child is added at the end of the list (see ViewAndroid::AddChild). While both childs are present, event is forwarded to the new child (see ViewAndroid::HitTest which iterates through base::Reversed(children_)). So then if anything cancels the navigation right there, events are forwarded to the new host. Question for navigation folks: Is there a point in time when input events should start be forwarded to the new RWHV? Is it RWHVBase::OnDidNavigateMainFrameToNewPage? I see it's called from RenderFrameHostImpl::DidCommitNavigationInternal which sounds like the right thing to me. Question for mcnee@: when the navigation is cancelled in your case, has the navigation been committed yet?
,
Aug 2
So I have a potential fix here: https://chromium-review.googlesource.com/c/chromium/src/+/1157275 But, there seems to be an issue with android webview (see my comment on the cl).
,
Aug 2
On the question in comment 8 - yes, we should not be sending input events to the new RWHV until we have committed the navigation. You are correct that RWHVBase::OnDidNavigateMainFrameToNewPage sound like the right place to do it.
,
Aug 2
So for chrome on android, WebContentsViewAndroid::RenderViewHostChanged seemed to be enough. However, it doesn't get called for interstitials or android webview. Fortunately, OnDidNavigateMainFrameToNewPage does get called for android webview as well.
,
Aug 2
Then safer fix for android: when the new RWHVA is created, at it to the front of the children_ list, so input continues to go to the old RWHVA. Then when OnDidNavigateMainFrameToNewPage happens, move the new RWHVA to the back of the list. This is less dependent on other things, and works for the first navigation as well.
,
Aug 7
I've filed issue 871844 for the desktop case since it's unrelated to the issue on android.
,
Aug 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1843005f0bf2112ef9d031b50e893a356b9f4dcc commit 1843005f0bf2112ef9d031b50e893a356b9f4dcc Author: Kevin McNee <mcnee@chromium.org> Date: Fri Aug 10 16:50:56 2018 Prevent speculative RWHVAs from handling input meant for existing views Currently, when we create a RenderWidgetHostViewAndroid, we attach its corresponding native view to the hierarchy. However, RWHVAs may be created for a speculative RenderFrameHost for a cross-site navigation, and would not replace the old RWHVA until the navigation commits. This gives us two RWHVAs in the hierarchy. In this state, input events are targeted to the speculative RWHVA. Moreover, if the cross-site navigation is canceled before it commits, then we stay in this state where input events are sent to the unused RWHVA, causing the page to be seemingly unresponsive to input. We now move new RWHVAs behind any existing ones in the view hierarchy. Bug: 867932 Change-Id: I845d99ecbde7f721f6fa4b6e1b0243abb89a97fc Reviewed-on: https://chromium-review.googlesource.com/1165482 Reviewed-by: Bo <boliu@chromium.org> Commit-Queue: Kevin McNee <mcnee@chromium.org> Cr-Commit-Position: refs/heads/master@{#582206} [modify] https://crrev.com/1843005f0bf2112ef9d031b50e893a356b9f4dcc/content/browser/frame_host/interstitial_page_impl.cc [modify] https://crrev.com/1843005f0bf2112ef9d031b50e893a356b9f4dcc/content/browser/renderer_host/render_widget_host_view_android.cc [modify] https://crrev.com/1843005f0bf2112ef9d031b50e893a356b9f4dcc/content/browser/renderer_host/render_widget_host_view_android.h [modify] https://crrev.com/1843005f0bf2112ef9d031b50e893a356b9f4dcc/content/browser/renderer_host/render_widget_host_view_base.h [modify] https://crrev.com/1843005f0bf2112ef9d031b50e893a356b9f4dcc/content/browser/site_per_process_browsertest.cc [modify] https://crrev.com/1843005f0bf2112ef9d031b50e893a356b9f4dcc/ui/android/view_android.cc [modify] https://crrev.com/1843005f0bf2112ef9d031b50e893a356b9f4dcc/ui/android/view_android.h [modify] https://crrev.com/1843005f0bf2112ef9d031b50e893a356b9f4dcc/ui/android/view_android_unittests.cc
,
Aug 13
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by mcnee@chromium.org
, Jul 26Labels: -Pri-2 OS-Linux Pri-1
Summary: Cancelling a cross-site navigation causes page to be unresponsive to input (was: Cancelling a cross-site navigation causes page to be unresponsive to touch)