Closed Bug 1555337 Opened 6 months ago Closed 12 days ago

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

Categories

(GeckoView :: General, defect, P1)

Unspecified
Android

Tracking

(firefox68 wontfix, firefox69 wontfix, firefox70 wontfix, firefox71? ?, firefox72 fixed)

RESOLVED FIXED
mozilla72
Tracking Status
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 ? ?
firefox72 --- fixed

People

(Reporter: jhugman, Assigned: snorp, NeedInfo)

References

Details

(Whiteboard: [geckoview:m1910] [geckoview:m1911])

Attachments

(2 files)

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)

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?

Flags: needinfo?(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.

Flags: needinfo?(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?

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

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.

Flags: needinfo?(bzbarsky)
Attached file 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

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.

Flags: needinfo?(bzbarsky)

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.

Flags: needinfo?(vzare)
Priority: -- → P1
Whiteboard: [geckoview:fenix:p2]

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.

Flags: needinfo?(vzare)
Whiteboard: [geckoview:fenix:p2] → [geckoview:fenix:m7]

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

Whiteboard: [geckoview:fenix:m7] → [geckoview:fenix:m8]

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>

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

Priority: P1 → P2

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

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.

Flags: needinfo?(etoop)
OS: All → Android
Whiteboard: [geckoview:fenix:m8] → [geckoview:fenix:m9]
Rank: 12
Whiteboard: [geckoview:fenix:m9]

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

Flags: needinfo?(etoop)

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.

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.

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

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

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.

Priority: P2 → P1
Whiteboard: [geckoview:m1910]

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

Assignee: nobody → snorp

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

(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.

Whiteboard: [geckoview:m1910] → [geckoview:m1910] [geckoview:m1911]

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.

Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b3988d311be3
Add `LoadRequest#hasUserGesture` r=geckoview-reviewers,rbarker,agi
Status: NEW → RESOLVED
Closed: 12 days ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72

Can we please have this uplifted to 71?

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?

Flags: needinfo?(royang51)
You need to log in before you can comment on or make changes to this bug.