New issue
Advanced search Search tips

Issue 891881 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Regression in Android.TabNavigationInterceptResult with the async intercept experiment

Project Member Reported by csharrison@chromium.org, Oct 3

Issue description

Pasting from email:
I did some investigation today. Recall that our experiment is only enabled for http/s URLs.

Tab Clobbering
I looked through the code and it seems like "tab clobbering" is done when the intent cannot be resolved and we fallback to a fallback URL (OVERRIDE_WITH_CLOBBERING_TAB), which loads in the current tab. This is a bit confusing since I would have assumed it "destroys" the old tab. I don't think fallback URLs can happen with http/s.

OVERRIDE_WITH_ASYNC_ACTION
Override with async action seems to be used for:
1. Incognito --> play store
2. Incognito --> some app that can't be handled within Chrome
3. Filesystem access (e.g. file URL)

Again, none of these cases are possible with http/s URLs. The navigation is cancelled synchronously when the user is shown a prompt.

Http/s URLs and OVERRIDE_WITH_EXTERNAL_INTENT
It does seem like there is behavior change here, with multiple navigations canceling each other.
https://cr.kungfoo.net/intents has some links. Navigating to youtube.com 6 times where youtube is the default app for that URL:
  - Without our experiment, this triggers 6 "Created external intent" events.
  - With our experiment, this triggers a single "No override" event.
This may be a bug in the logic of the "user gesture carryover", combined with the "navigations cancelling each other before the should ignore logic is called" referenced above.

Cc: ntfschr@chromium.org
Labels: OS-Android
Sorry for the delay here, been busy with other work. The experiment is showing 1%  improvements on various %iles for FCP on Android so I am eager to land it.

Here's a mechanism I found that explains the regression:
TabRedirectHandler.updateNewUrlLoading happens asynchronously now for http/s URLs. So consider the case when a user clicks a normal link, and does an interaction before the async updateNewUrlLoading occurs.
  - Before the change this would cause the _next_ navigation to be considered isNewLoadingStartedByUser.
  - After the change, any subsequent navigation (w/out another gesture) is considered an effective redirect, and carries over the mInitialNavigationType and mShouldNotOverrideUrlLoadingUntilNewUrlLoading.


How this translates into different behavior is complicated, but I can see a few different ways:

1. Navigate by user typing, followed by an interaction before updateNewUrlLoading, followed by a commit and a client redirect to an intent. We don't override loading for user typed navigations.

2. Navigate by reloading, followed by an interaction before updateNewUrlLoading, followed by a commit and a client redirect to an intent. shouldStayInChrome is true for reloads.

3.
 a. Navigate to foo.com
 b. Client redirect to intent w/ fallback to bar.com, user interacts _before_ the fallback's async check occurs.
 c. bar.com also has a client redirect to an intent with a fallback, which is now blocked, because (c) is not considered an effective redirect of (b).




In any case, if this is the mechanism causing the regression, it seems reasonably OK with me from a semantics point of view. The "effective redirect" calculation is a heuristic anyways and you could argue that this new behavior better reflects users intentions.

I haven't been able to reproduce this since it is so timing dependent, and instrumentation tests currently don't have the ability (as far as I can tell) to hook into this piece of navigation, but this seems to be the most likely culprit.

Nate: Any thoughts?

Sign in to add a comment