New issue
Advanced search Search tips

Issue 867932 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 13
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

Cancelling a cross-site navigation causes page to be unresponsive to input

Project Member Reported by mcnee@chromium.org, Jul 26

Issue description

Chrome 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.
 
Components: -Internals>Input>Touch>Screen Internals>Input
Labels: -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)
So I'm noticing something similar for desktop, but the timing is a bit different.

If the cross-site navigation is canceled after the url in the omnibox is updated but before the existing page is no longer shown, then we get into the same state where the page is unresponsive to input. In the android case, I canceled before the url change.
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.
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.
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
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
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.
Cc: jinsuk...@chromium.org boliu@chromium.org
+boliu +jinsuk in case an of our recent content refactors changed how touch propagation goes.
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?
Owner: mcnee@chromium.org
Status: Assigned (was: Untriaged)
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).
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.
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.
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.
Labels: -OS-Linux
I've filed  issue 871844  for the desktop case since it's unrelated to the issue on android.
Project Member

Comment 14 by bugdroid1@chromium.org, 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

Status: Fixed (was: Assigned)

Sign in to add a comment