1515073 - Back/forward buttons should expose only pages with user-interaction

Status: RESOLVED FIXED

(Core :: DOM: Navigation, enhancement, P2)

Description

[Andrea Marchesini [:baku]]

The intent of this bug is to block the history push state for sites that abuse it. We already know the user-interaction state per every documents (bug 1491835). We can use this information to skip pages which have not been user-interacted.

This is similar to what Chromium is experimenting. See: https://www.slashgear.com/google-chrome-will-soon-say-no-changing-browser-history-17558484/

Comment 1

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=839642519e73e969629d020a62dd3194fbecd71e

Comment 2

[Johann Hofmann [:johannh]]

See also https://github.com/WICG/interventions/issues/21

Comment 3

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a639a38822d5a85353968bfcfc24dc09e858431d

Comment 4

[Johann Hofmann [:johannh]]

Attachment: Bug 1515073 - Part 1 - Add nsISHEntry::hasUserInteraction. r=peterv!

Comment 5

[Johann Hofmann [:johannh]]

Attachment: Bug 1515073 - Part 2 - Allow nsIWebNavigation::{goBack,goForward} to skip entries without user interaction. r=peterv

Comment 6

[Johann Hofmann [:johannh]]

Attachment: Bug 1515073 - Part 3 - Add support for skipping session entries without user interaction on desktop. r=Gijs!,JanH

Comment 7

[Johann Hofmann [:johannh]]

Attachment: Bug 1515073 - Part 4 - Add test for skipping pages without user interaction on back-forward. r=Gijs

Comment 8

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bb060275013c0ae036e78be1979f4cb117416a5

Comment 9

[Johann Hofmann [:johannh]]

It appears that someone made a site for us to test on: http://matthewrayfield.com/articles/animating-urls-with-javascript-and-emojis

Comment 10

[Johann Hofmann [:johannh]]

Attachment: Bug 1515073 - Part 1 - Add nsISHEntry::hasUserInteraction. r=bzbarsky

Comment 11

[Johann Hofmann [:johannh]]

Attachment: Bug 1515073 - Part 2 - Allow nsIWebNavigation::{goBack,goForward} to skip entries without user interaction. r=bzbarsky

Depends on D31601

Comment 12

[Johann Hofmann [:johannh]]

Attachment: Bug 1515073 - Part 5 - Disable browser.navigation.requireUserInteraction in tests. r=Gijs

Depends on D27588

Comment 16

[Johann Hofmann [:johannh]]

This is waiting on bug 1591943 to avoid complicating Fission work. Once that's done, I'll try and drive this over the finish line.

Comment 17

[(no longer active)]

(In reply to Johann Hofmann [:johannh] - Away until Dec 3rd from comment #16)

This is waiting on bug 1591943 to avoid complicating Fission work. Once that's done, I'll try and drive this over the finish line.

That is done now, FWIW.

Comment 18

[Shane Caraveo (:mixedpuppy)]

I'm not certain we want to break extensions this way (nor am I certain we dont want to)...browser.history can add/remove history entries. Those will not be marked with the user interaction flag, so navigation on them will be skipped.

We should consider whether to somehow add the flag when an extension calls addUrl[1]

Pinging Philip for another opinion on this.

[1] https://searchfox.org/mozilla-central/rev/2f09184ec781a2667feec87499d4b81b32b6c48e/browser/components/extensions/parent/ext-history.js#148

Comment 19

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #18)

I'm not certain we want to break extensions this way (nor am I certain we dont want to)

sorry...I may be wrong about what I said in comment 18. The history api uses placesutils.history, I'm not certain that has anything to do with the history state of a tab. I'm going to do a bit more investigation on how extensions interact with this. If this change does effect extensions (probably in content scripts) I think we still need to make a decision on whether that is what we want

Comment 20

[Shane Caraveo (:mixedpuppy)]

Also, the above is not meant to block these changes in any way, it's about whether we need to followup with a change for extensions.

Comment 21

[(no longer active)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #19)

(In reply to Shane Caraveo (:mixedpuppy) from comment #18)

I'm not certain we want to break extensions this way (nor am I certain we dont want to)

sorry...I may be wrong about what I said in comment 18. The history api uses placesutils.history, I'm not certain that has anything to do with the history state of a tab.

It doesn't, the browser history and session history are separate from each other. For example, in private browsing mode we have session history but we don't persist browser history.

I'm going to do a bit more investigation on how extensions interact with this. If this change does effect extensions (probably in content scripts) I think we still need to make a decision on whether that is what we want

I would expect extension content scripts to be able to push entries into the session history using the history.pushState API in the exact same way that web pages do. It would be interesting to scan existing extensions on AMO to see if we can find a legitimate use case for an extension to push a page that has never received user interaction onto the session history, and the user wanting to go back to that page; I can't imagine off the top of my head what such a use case might look like.

Comment 22

[Tomislav Jovanovic :zombie]

(In reply to :ehsan akhgari from comment #21)

I would expect extension content scripts to be able to push entries into the session history using the history.pushState API in the exact same way that web pages do. It would be interesting to scan existing extensions on AMO to see if we can find a legitimate use case for an extension to push a page that has never received user interaction onto the session history, and the user wanting to go back to that page; I can't imagine off the top of my head what such a use case might look like.

There was a XUL extension I used that put fake history into tabs from links opened via middle-click (open in new tab). It's basically what I wish my browser would would do by default.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/08b6902debe8
Part 1 - Add nsISHEntry::hasUserInteraction. r=peterv
https://hg.mozilla.org/integration/autoland/rev/e91f0572aa8c
Part 2 - Allow nsIWebNavigation::{goBack,goForward} to skip entries without user interaction. r=Gijs,peterv
https://hg.mozilla.org/integration/autoland/rev/e08524b0a11c
Part 3 - Add support for skipping session entries without user interaction on desktop. r=JanH,Gijs
https://hg.mozilla.org/integration/autoland/rev/88104bd105a8
Part 4 - Add test for skipping pages without user interaction on back-forward. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/891bf9d395fd
Part 5 - Disable browser.navigation.requireUserInteraction in tests. r=Gijs

Comment 25

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/08b6902debe8
https://hg.mozilla.org/mozilla-central/rev/e91f0572aa8c
https://hg.mozilla.org/mozilla-central/rev/e08524b0a11c
https://hg.mozilla.org/mozilla-central/rev/88104bd105a8
https://hg.mozilla.org/mozilla-central/rev/891bf9d395fd

Comment 26

[Johann Hofmann [:johannh]]

Going through the webcompat issues...

https://webcompat.com/issues/39963 (NSFW) is fixed
https://webcompat.com/issues/50326 is fixed
https://webcompat.com/issues/47715 I can't reproduce anymore
https://webcompat.com/issues/41847 (NSFW) seems fixed at first glance, but when navigating back and forth a little the site seems to be able to add another sh entry somehow, but not in Chrome. I'll file a bug to investigate

Comment 29

[Rob Wu [:robwu]]

Attachment: Bug 1515073 - Handle middle-click on long-press backForwardMenu only once + tests

Comment 30

[Mathew Hodson]

That patch is in the wrong bug I think. It should be in bug 1657992 right?

Comment 31

[Rob Wu [:robwu]]

Woops, thanks. Sorry for the noise here!

Comment 32

[Phabricator Automation]

Comment on attachment 9169534 [details]
Bug 1515073 - Handle middle-click on long-press backForwardMenu only once + tests

Revision D86784 was moved to bug 1657992. Setting attachment 9169534 [details] to obsolete.