New issue
Advanced search Search tips

Issue 772515 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 661629
issue 781811



Sign in to add a comment

Popups should be blocked if no user gesture received since last main frame navigation started

Project Member Reported by csharrison@chromium.org, Oct 6 2017

Issue description

See intent to implement for one possible solution: consuming user gestures on main frame navs.

https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/kPli8ZCUeok
 
I am a bit confused: since popup already requires a user gesture, a newly opened page shouldn't be able to create popups until a user interaction happens.  Did you mean we have a crack that allows it?

Or the right question may be: by "last main frame nav started", did you mean to cover more than just the "tab-under" scenario?

In any case, a minimal repro would be helpful.

Sorry, this is not about a newly opened page, but about the page that initiated the navigation, before it commits.

E.g.

1. Start navigating from a.com to b.com
2. Open popup to c.com
3: Commit the navigation to b.com

A minimal repro is http://cr.kungfoo.net/popups/tab_under/ the "Popup and then navigate" button.
Sorry, my repro mentions the wrong button. To repro this issue, you want the "Navigate and then Popup".

In this case, if there is no user gesture between the beginning of the nav and the popup, the popup should be blocked.
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/26b6c12edd9a09262fc1c08f305890279ed9fb05

commit 26b6c12edd9a09262fc1c08f305890279ed9fb05
Author: Charles Harrison <csharrison@chromium.org>
Date: Sat Nov 04 06:30:46 2017

Make main frame navigations consume a user gesture

The tactical purpose of this patch is to constrain tab-unders [1] so
that they are forced to open a popup *before* navigating. This
constraint allows us to do more aggressive interventions in the future,
because pages that do a successful tab-under after this patch will
1. Be forced to navigate after opening a popup (and can be tagged as
   a popup-opening page)
2. Be forced to navigate without a user gesture (since popups consume
   a gesture).

We may be able to intervene on navigations which satisfy (1) and (2) in
the future.

You can see the effects of this patch by loading:
http://cr.kungfoo.net/popups/tab_under/

If you choose "Navigate and then Popup", the popup will be blocked.

Note: This feature is launched behind a disabled-by-default feature
flag. It is currently going through the intent to ship process on blink
dev [2]. It should not be enabled until LG from API owners.

[1]: A tab-under is when a page opens a popup to a link target, while
     also navigating the original page to an ad.

[2]: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/kPli8ZCUeok

Bug:  772515 
Change-Id: Id45d0b85aa55b0eb5a85bcaecc4ca3f9e551796e
Reviewed-on: https://chromium-review.googlesource.com/676656
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514035}
[modify] https://crrev.com/26b6c12edd9a09262fc1c08f305890279ed9fb05/chrome/browser/chrome_navigation_browsertest.cc
[add] https://crrev.com/26b6c12edd9a09262fc1c08f305890279ed9fb05/chrome/test/data/links.html
[add] https://crrev.com/26b6c12edd9a09262fc1c08f305890279ed9fb05/chrome/test/data/navigation_consumes_gesture.html
[modify] https://crrev.com/26b6c12edd9a09262fc1c08f305890279ed9fb05/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/26b6c12edd9a09262fc1c08f305890279ed9fb05/content/renderer/render_frame_impl.h

Blocking: 781811
What is the status of the popup blocker? I tested Chrome Canary 66.0.3332.0 with --enable-features=AbusiveExperienceEnforce, all tests in http://cr.kungfoo.net/popups/tab_under/ still fail (i.e. popup/redirect blocked icons appear, but popup and redirect still happen anyway).

It also fails completely in http://popunderjs.com/
Abusive Experience popup blocking is different from this feature and tab-under blocking (I know... it's confusing).

To test tab-under blocking fully, try running Chrome with
--enable-features=BlockTabUnders,ConsumeGestureOnNavigation

Note that due to  issue 733736  there was a gap in the implementation, which should be fixed in M66.

Project Member

Comment 8 by bugdroid1@chromium.org, Apr 9 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2ca250d409adfa73a666daabd3ba19b94d6443a7

commit 2ca250d409adfa73a666daabd3ba19b94d6443a7
Author: Charlie Harrison <csharrison@chromium.org>
Date: Mon Apr 09 22:02:18 2018

Enable ConsumeGestureOnNavigation by default

See blink-dev thread:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/kPli8ZCUeok

A browser test is moved to be a tentative WPT due to this change.

Bug:  772515 
Change-Id: Icf99c8c303c5055dcbcdace6ae94e3fcd1a01921
Reviewed-on: https://chromium-review.googlesource.com/980599
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Mustaq Ahmed <mustaq@chromium.org>
Reviewed-by: Jonathon Kereliuk <kereliuk@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549293}
[modify] https://crrev.com/2ca250d409adfa73a666daabd3ba19b94d6443a7/chrome/browser/chrome_navigation_browsertest.cc
[modify] https://crrev.com/2ca250d409adfa73a666daabd3ba19b94d6443a7/content/renderer/render_frame_impl.cc
[add] https://crrev.com/2ca250d409adfa73a666daabd3ba19b94d6443a7/third_party/WebKit/LayoutTests/external/wpt/html/browsers/browsing-the-web/navigating-across-documents/navigation-consumes-user-activation.tentative.sub.html

Project Member

Comment 9 by bugdroid1@chromium.org, Apr 10 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e9c808f26dcdf8e2a2e96851624d8caf1b52ba0e

commit e9c808f26dcdf8e2a2e96851624d8caf1b52ba0e
Author: Makoto Shimazu <shimazu@chromium.org>
Date: Tue Apr 10 01:23:59 2018

Revert "Enable ConsumeGestureOnNavigation by default"

This reverts commit 2ca250d409adfa73a666daabd3ba19b94d6443a7.

Reason for revert: A leak bot complains navigation-consumes-user-activation.tentative.sub.html is leaking.
Sample build:
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linux%20Trusty%20Leak/builds/17509

Original change's description:
> Enable ConsumeGestureOnNavigation by default
> 
> See blink-dev thread:
> https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/kPli8ZCUeok
> 
> A browser test is moved to be a tentative WPT due to this change.
> 
> Bug:  772515 
> Change-Id: Icf99c8c303c5055dcbcdace6ae94e3fcd1a01921
> Reviewed-on: https://chromium-review.googlesource.com/980599
> Reviewed-by: Nasko Oskov <nasko@chromium.org>
> Reviewed-by: Mustaq Ahmed <mustaq@chromium.org>
> Reviewed-by: Jonathon Kereliuk <kereliuk@chromium.org>
> Commit-Queue: Charlie Harrison <csharrison@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#549293}

TBR=nasko@chromium.org,mustaq@chromium.org,csharrison@chromium.org,kereliuk@chromium.org

Change-Id: I0c998798d1367be61c633db76429c18ac554e4ff
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  772515 
Reviewed-on: https://chromium-review.googlesource.com/1003437
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549363}
[modify] https://crrev.com/e9c808f26dcdf8e2a2e96851624d8caf1b52ba0e/chrome/browser/chrome_navigation_browsertest.cc
[modify] https://crrev.com/e9c808f26dcdf8e2a2e96851624d8caf1b52ba0e/content/renderer/render_frame_impl.cc
[delete] https://crrev.com/bb693e3cab09f26eb1c0436e4695ef41ed1550d6/third_party/WebKit/LayoutTests/external/wpt/html/browsers/browsing-the-web/navigating-across-documents/navigation-consumes-user-activation.tentative.sub.html

Posting leak results from the bot for posterity:
17:44:08.287 30578 [3796/6775] external/wpt/html/browsers/browsing-the-web/navigating-across-documents/navigation-consumes-user-activation.tentative.sub.html failed unexpectedly (leak detected: ({"numberOfLivePausableObjects":[2,3]}))

17:52:15.260 30578 [3/7] external/wpt/html/browsers/browsing-the-web/navigating-across-documents/navigation-consumes-user-activation.tentative.sub.html failed unexpectedly (leak detected: ({"numberOfLiveDocuments":[1,2],"numberOfLiveNodes":[4,16],"numberOfLivePausableObjects":[2,3],"numberOfLiveResourceFetchers":[1,2]}))


17:52:16.738 30578 [2/2] external/wpt/html/browsers/browsing-the-web/navigating-across-documents/navigation-consumes-user-activation.tentative.sub.html failed unexpectedly (leak detected: ({"numberOfLiveDocuments":[1,2],"numberOfLiveNodes":[4,16],"numberOfLivePausableObjects":[2,3],"numberOfLiveResourceFetchers":[1,2]}))

This is probably from the fact that our test does strange things like navigating to another document on success.
Project Member

Comment 11 by bugdroid1@chromium.org, Apr 11 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5bab263e50fc9755106d88cc012d5d78e22d2e96

commit 5bab263e50fc9755106d88cc012d5d78e22d2e96
Author: Charlie Harrison <csharrison@chromium.org>
Date: Wed Apr 11 05:44:53 2018

Partial reland of r549293: Enable ConsumeGestureOnNavigation by default

r549293 was reverted because we converted a browser test into a
tentative WPT, which ended up being both leaky AND flaky.

This CL just keeps the browser test, and we'll do the WPT conversion
asynchronously.

Bug:  772515 
Change-Id: I1db454c741fe26981c3cd3156901b68ce29338ad
Reviewed-on: https://chromium-review.googlesource.com/1005318
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549781}
[modify] https://crrev.com/5bab263e50fc9755106d88cc012d5d78e22d2e96/content/renderer/render_frame_impl.cc

Status: Fixed (was: Assigned)

Sign in to add a comment