1555337 - Indicate if a URL change has been caused by a user interaction.

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[James Hugman [:jhugman] [@jhugman]]

Android components wishes to see if a change in location has happened due to user interaction.

Events that would cause triggeredByUserInteraction to be true:

1. A user clicks on a link, and the URL is loaded
2. A user clicks on a link, and the URL is loaded, then redirects to another URL which is loaded.
3. A user taps on a button, and JS changes the window.location.
4. A user taps on a button, and JS starts a timer to change the window.location.

Events that would cause triggeredByUserInteraction to be false:

1. The user does not do anything, and JS changes the window.location.
2. The user does something that is not tapping (e.g. scrolling) and JS changes the window.location.

In Fennec, this was done through 'time since last user interacted with page'.

This is in support of supporting of opening external apps (e.g. YouTube, Twitter) and app banners (open the market, open SoundCloud/Reddit/Tumblr)

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

We tried to do this for bug 1487542, but it turns out to be fairly complicated. The Fennec impl has caused a lot of problems in the past too. It might be time to solve it for real.

bz, has anyone thought about tracking user gesture status all the way through the cases describe in comment #0?

Comment 2

[Boris Zbarsky [:bzbarsky]]

A user clicks on a link, and the URL is loaded, then redirects to another URL which is loaded.

Is this limited to HTTP 3xx redirects? Or does it include scripted "redirects" via location sets, <meta> refresh "redirects", etc?

A user taps on a button, and JS starts a timer to change the window.location.

What if the timer is for 1 hour? Should that still count as user interaction?

An obvious thing missing from the list is "user taps a button, and JS creates a Promise whose resolution changes window.location".

has anyone thought about tracking user gesture status all the way through the cases describe in comment #0

We have several things that do such tracking through async bits. Specifically, various stuff propagates popup control state, and Promises have Promise::MaybePropagateUserInputEventHandling.

It doesn't help that we have several different definitions of "user input" (at least three, actually; EventStateManager::IsHandlingUserInput, popup controls, and the additional time-based check in nsContentUtils::CheckRequestFullscreenAllowed).

I don't know whether the ESM considers scrolling as "user input". I do know it includes more than just tapping.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, and right now nothing tracks whether a load was started via user gesture, but we could add that to the LoadInfo. Would we want to allow multiple loads due to one user gesture?

Comment 4

[James Hugman [:jhugman] [@jhugman]]

Also falling in to category of triggeredByUserInteraction = false: changing window location on page load.

I'm not sure if this should be allowed at all, but it's especially relevant in the context of opening an external app.

Test page:
https://www.kevinbrosnan.net/testcases/intent-spam.html

Comment 5

[James Hugman [:jhugman] [@jhugman]]

A user clicks on a link, and the URL is loaded, then redirects to another URL which is loaded.

Is this limited to HTTP 3xx redirects? Or does it include scripted "redirects" via location sets, <meta> refresh "redirects", etc?

This is to stop attacks on an app-links system. If the web page isn't changed, but some other native code is executed, then this code could be executed multiple times. This may make the browser unusable, or be an explicit attack on a third party app.

A user taps on a button, and JS starts a timer to change the window.location.

What if the timer is for 1 hour? Should that still count as user interaction?

I included this as it was a way of implementing app banners. I don't know if this method of implementing app banners is discouraged now, and therefore this should not be triggeredByUserInteraction.

An obvious thing missing from the list is "user taps a button, and JS creates a Promise whose resolution changes window.location".

You're right. I don't think comment #0 is exhaustive.

Comment 6

[Kevin Brosnan [Ex-Mozilla]]

Attachment: intent-spam.html

The source for my intent attack. Attached as plain text so it is easier to view. The attack requires you to have the Wikipedia app installed. https://play.google.com/store/apps/details?id=org.wikipedia

Comment 7

[Boris Zbarsky [:bzbarsky]]

So this is basically trying to fix bug 670328? The last proposal there was to just use existing popup blocker state at the point when navigation starts... I've just been lame about trying to do it.

Comment 8

[Chris Peterson [:cpeterson]]

Vesta, what is the priority of making app links (that launch an app from a web page, e.g. reddit.com launching the Reddit app)?

Fixing this bug will be tricky. App links kinda work in Fennec, but there are bugs.

Comment 9

[vzare]

Chris, this is high priority for Fenix. We had to take this out of MVP due to the security concerns outlined by Kevin but I'd like to be able to add it back in as soon as we have nailed this down.

Comment 10

[Chris Peterson [:cpeterson]]

Deferring this bug from Fenix's M7 (July) milestone to the M8 backlog for later in Q3.

Comment 11

[James Hugman [:jhugman] [@jhugman]]

Android Components 3857 also triggers this issue via an iframe.

<html>
<head>
  <title>Android Intents URLs test</title>
</head>
<body>
  <iframe src="intent://mozilla.org#Intent;scheme=https;package=org.mozilla.firefox;action=android.intent.action.VIEW;end" style="display: none;"></iframe>
</body>
</html>

Comment 12

[Chris Peterson [:cpeterson]]

I'm editing a bunch of GeckoView bugs. If you'd like to filter all this bugmail, search and destroy emails containing this UUID:

e88a5094-0fc0-4b7c-b7c5-aef00a11dbc9

Comment 13

[Chris Peterson [:cpeterson]]

The Fenix team would like this fix in Q3. Let's review it in September.

Comment 14

[Chris Peterson [:cpeterson]]

We don't want script-initiated redirects to show up in the user's browser history. This is a top Fenix request.

Emily will contact DOM team. There will be a little GV API work after the DOM parts are fixed.

Comment 15

[Emily Toop (:fluffyemily)]

I've contacted Hsin-Yi about this and am waiting to hear back as to when the DOM team can schedule this into their workflow

Comment 16

[Emily Toop (:fluffyemily)]

Hsin-yi has got back to me to say that a new API has been added recently that she thinks will give us what we need here. She has asked Edgar to update the bug with details.

Comment 17

[Edgar Chen [:edgar]]

Document.hasValidTransientUserGestureActivation is designed to track user interaction. Basically, It is kind of global state of the page indicates whether a page is considered as having user interaction within a recent time frame (5 sec). It also works in timeout callback or promise resolution as long as they happen also within the time frame that user interacts with the page.

Comment 18

[[ex-Mozilla] Agi Sferro | :agi]

Nice! Sounds like all we have to do in this bug is expose this flag to LoadRequest, onLocationChange and related.

Comment 19

[James Hugman [:jhugman] [@jhugman]]

Adding to LoadRequest would suit the needs of android-components/feature-app-links which prompted this bug.

Comment 20

[Chris Peterson [:cpeterson]]

We want to work on this in October.

Bobby says the Fission team is rewriting Gecko's session history code. We should fix this bug in GV now, but also make sure the Fission team's rewrite takes this issue into consideration.

Comment 21

[Chris Peterson [:cpeterson]]

Randall says Twitch's desktop/mobile redirects are a good test case.

Comment 22

[Chris Peterson [:cpeterson]]

James says he has a patch in progress that would be ready to land for GV's October sprint.

Comment 23

[Chris Peterson [:cpeterson]]

(In reply to Chris Peterson [:cpeterson] from comment #22)

James says he has a patch in progress that would be ready to land for GV's October sprint.

October has passed, so I'm rolling this bug into GV's November sprint because Fenix needs this fix for GV 72 to match desktop Firefox's new Notification Permission Prompt behavior.

Comment 24

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1555337 - Add `LoadRequest#hasUserGesture`

This uses Document.hasValidTransientUserGestureActivation to expose
gesture for use in the onLoadRequest() delegate method in GeckoView,
allowing apps to know if a load was initiated by a user.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b3988d311be3
Add `LoadRequest#hasUserGesture` r=geckoview-reviewers,rbarker,agi

Comment 26

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/b3988d311be3

Comment 27

[royang51]

Can we please have this uplifted to 71?

Comment 28

[Kevin Brosnan [Ex-Mozilla]]

Robert can you expand on why we want to uplift this. What does this uplift allow you to do? Why waiting for gv 72 beta around the 1st week of December is too long to wait?

Comment 29

[royang51]

Misunderstood the comment from my issue. We're good with December timeframe for GV 72 Beta. Please ignore my uplift request to GV 71. Thanks,