hwi@ Thanks for the issue.

As per the above description, this issue is reported on Android.
Hence marking the OS as Android, adding 'TE-NeedsTriageFromHYD' and requesting the appropriate team to look into the issue and help in further triaging.

Thanks..

Ah thanks, susan.boorgula@!

Tested this issue on Android and able to reproduce this issue.

Steps followed:
1. Launch chrome
2. Navigate to http://www.popuptest.com/
3. Click on 'Multi-PopUp Test'
4. Observe One window open, and 5 popups blocked

Note: This is issue is reproducible by after enabling #enable-chrome-modern-design

Chrome Version:
69.0.3494.0 Dev ,69.0.3495.2 Canary

OS:
8.1.0

Android device:
Nexus 6P

Unable to provide per revision bisect as we are getting all good builds.Tried to increase range but no luck.Hence providing the manual bisect results,
Good build:69.0.3475.0
Bad build: 69.0.3476.0

Marking this is issue as Untriaged for more inputs on same.

Please navigate to below link for log's and screen cast --
go/chrome-androidlogs/865243

Thanks!

I tried reproducing this with chrome modern disabled and could still reproduce the bug. Are you sure that #enable-chrome-modern-design is necessary?

c6: The issue happens *both with and without* #enable-chrome-modern-design (Canary 69.0.3496.0).

CL range: 
=========
https://chromium.googlesource.com/chromium/src/+log/69.0.3475.0..69.0.3476.0?pretty=fuller&n=10000

Releated patches in the above range
===================================
https://chromium.googlesource.com/chromium/src/+/25d68397fe4e4471239e8581b3b5742ad655305f

https://chromium.googlesource.com/chromium/src/+/d9b5b4af6ea956dc86290ad99e7d0d1020cf0504

Strongly suspects Charlie's patch, hence assigning.

This is indeed related to my change, but only manifests with UserActivationv2. Adding mustaq to cc.

Mustaq: If UAv2 is not launching in M69, we can remove RB-Stable. I imagine we have a browser test that tests this already, have you noticed popup block failures for enabling UAv2 on fieldtrial_testing_configs?

BTW a smaller repro is here:
data:text/html,<a href="https://cr.kungfoo.net/popups/window_open.html">clickme</a>

Clicking probably should not allow the "window_open" page to spawn a popup.

UAv2 is not launching in M69.  It is currently enabled only for Dev/Canary, testing config was not required for this trial.

I have been keeping an eye on the tests through this CL: crrev.com/c/796938.  The remaining failures are mostly in 3 "groups": notifications and extensions (plus some wpt layout tests), so I think we don't have a realistic browser test for this bug yet.

The last test run of my test CL (last week) shows a few new layout test failures around nav.  Not sure if they cover this particular case.

Thanks Mustaq, removing RB-Stable per #11.

I did some analysis and I have figured out that UAv2 relies on the ConsumeGestureOnNavigation launch to fix this bug. From what I can tell, clicking a link imbues activation on the subsequent page load.

BEFORE r571302:
The consumption in DidStartProvisionalLoad was actually called twice (strange PlzNavigate logic that I don't fully understand yet): once in a response to the mouse event (after HTMLAnchorElement::HandleClick) and once in response to RenderFrameImpl::CommitNavigation.

The first DidStartProvisionalLoad is on the previous page, but the second DidStartProvisionalLoad is on the subsequent page.

AFTER r571302:
We no longer consume a gesture on navigation commit, so the activation stays on the subsequent page, and it can spawn popups.


I think UAv2 needs some notion of consumption at commit time, which is out of scope for my feature, which consumes at navigation start. Mustaq, are you aware of gestures persisting across navigations like this? Can you take a look?

It's logical to assume that a new page load starts with a clean state, but I have seen plenty of cases where a past activation state is carried over.  Not sure how consuming on commit would affect those cases.

This needs an owner who is expert in navigation states.  Unfortunately I am not one of them.

csharrison@: When is ConsumeGestureOnNavigation launching?

mustaq: ConsumeGestureOnNavigation launched in M67.

Hmm, then why do we _no longer_ consume a gesture on navigation commit after r571302?

It seems "consuming on commit" idea would simply bring back the bug r571302 was trying to fix, right?

In any case, this is a navigation related problem, and I am not the right owner here.

The ConsumeGestureOnNavigation experiment was only scoped to consume gestures on navigation start, not commit. The old gesture code reset gestures on commit anyway, I think, so I thought this was a UAv2 bug.

Copying an important comment from a doc comment (the link below):  "... (user gesture bit) is set back on the new document when we commit only on Android".  

clamy@: Could you please confirm if this is still an Android-only case?  Any clue why this was needed in the first place?  And link the code that does this?

https://docs.google.com/document/d/1mohg6bRELWpW95NPq4V7IDJjjpWwMPWC6JNU-DAPYGg/edit?disco=AAAABqsBMBU

+mlamouri who modified user gesture for user activation.

I'm not familiar with the code that consumes user-gesture on the renderer-side. On the browser-side, we only set user gesture for browser-initiated navigations on Android: see this code https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_controller_impl.cc?dr=CSs&g=0&l=2498. We would have liked to have it properly set on other platforms, but several tests were not expecting this so we didn't move forward with it.

Same issue is observed in ChromeOS Version 70.0.3505.0 (Official Build) canary (64-bit)

Deleted: Screenshot 2018-08-01 at 8.11.50 PM.png 219 KB

	Screenshot 2018-08-01 at 8.11.50 PM.png 219 KB View Download

 Issue 870446  has been merged into this issue.

The merged bug in #c21 is on Mac, so this looks like a widespread problem.

Let me pick this up to see why only UAv2 exposes the problem.

I seem to have reached the same conclusion as csharrison@ in #c12 above: the second user activation notification call (made at RenderFrameImpl::CommitNavigation) during a navigation look mysterious.  Digging into the history reveals that the call was added to fix a PasswordManager test: https://codereview.chromium.org/2378393002.

But that test now passes without the call!  And removing the call seems to fix this bug, yayy!!

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

commit efa963a8c9db46d9ef23898ebdb6f07252f33cb6
Author: Mustaq Ahmed <mustaq@google.com>
Date: Mon Nov 12 15:28:57 2018

Skip user gesture creation during RenderFrameImpl navigation commit.

|RenderFrameImpl::CommitNavigation*| functions got |WebScopedUserGesture|s few
years ago to fix a PasswordManager test (crrev.com/2378393002).  Those artificial
user gesture tokens seem to have become redundant in the meantime through changes
in navigation and/or PasswordManager.  This CL removes those tokens because they
are causing popup-blocker failure with UAv2.

Bug:  865243 
Change-Id: I6ed7d73a5784f82ce3ba695067a4cbff0a9c24b4
Reviewed-on: https://chromium-review.googlesource.com/c/1323626
Commit-Queue: Mustaq Ahmed <mustaq@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607233}
[modify] https://crrev.com/efa963a8c9db46d9ef23898ebdb6f07252f33cb6/chrome/browser/chrome_navigation_browsertest.cc
[modify] https://crrev.com/efa963a8c9db46d9ef23898ebdb6f07252f33cb6/content/renderer/render_frame_impl.cc