New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 601584 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 13
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 589894
issue 667551
issue 780556

Blocking:
issue 477150



Sign in to add a comment

chrome.gpuBenchmarking.pointerActionSequence() failure in presence of an OOPIF triggers cross-origin-iframe.html layout test failure.

Project Member Reported by lukasza@chromium.org, Apr 7 2016

Issue description

Site Isolation FYI bot has caught a regression in https://build.chromium.org/p/chromium.fyi/builders/Site%20Isolation%20Linux/builds/8690

Repro (--additional-drt-flag=--site-per-process is the important bit):

$ third_party/WebKit/Tools/Scripts/run-webkit-tests -t gn -v --additional-drt-flag=--site-per-process --additional-drt-flag=--no-sandbox http/tests/security/xss-DENIED-object-element.html


Expected behavior: test passes

Actual behavior: 

Regressions: Unexpected text-only failures (1)
  http/tests/bluetooth/https/requestDevice/cross-origin-iframe.html [ Failure ]
 
Owner: jyasskin@chromium.org
Status: Assigned (was: Untriaged)
https://codereview.chromium.org/1862953002 probably broke this.
Confirmed - https://codereview.chromium.org/1862953002 moved bluetooth/requestDevice-sandboxed-iframe.html into http/tests/bluetooth/https/requestDevice/cross-origin-iframe.html and the test apparently never before worked for --site-per-process.

So - there is probably no reason to be alarmed by this - I'll just add a test expectation for --site-per-process mode.
Owner: ----
BTW - the test failure is:

FAIL Request device from a unique origin. Should reject with SecurityError. assert_equals: expected "SecurityError: requestDevice() called from cross-origin iframe." but got "SecurityError: Must be handling a user gesture to show a permission request."
Harness: the test ran to completion.
Cc: jyasskin@chromium.org lukasza@chromium.org
"Investigate and implement support for UserGestureIndicator with OOPIFs" is in Q2 OKRs for site-isolation :-)   Therefore - let me move jyasskin@ to CC.
Labels: -Pri-3 Pri-2
Status: Untriaged (was: Assigned)
Summary: No OOPIF support for UserGestureIndicator triggers cross-origin-iframe.html layout test failure in --site-per-process-mode (was: http/tests/bluetooth/https/requestDevice/cross-origin-iframe.html fails in --site-per-process-mode)
Project Member

Comment 7 by bugdroid1@chromium.org, Apr 7 2016

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

commit 315cc7123ae79a389900c4c28856577fd042709a
Author: lukasza <lukasza@chromium.org>
Date: Thu Apr 07 22:33:42 2016

Add test expectations for recent regressions caught by Site Isolation FYI bot.

BUG= 601584 ,  601581 
NOTRY=true

Review URL: https://codereview.chromium.org/1867003005

Cr-Commit-Position: refs/heads/master@{#385904}

[modify] https://crrev.com/315cc7123ae79a389900c4c28856577fd042709a/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process

Let me know if you need me to do anything about this. If the gesture isn't propagated, #3 is the expected error.

Comment 9 by creis@chromium.org, Apr 8 2016

Cc: dominickn@chromium.org jochen@chromium.org
Note: we have a bug on file for updating user gestures for OOPIFs (issue 589894) and a thread about it on site-isolation-dev:
https://groups.google.com/a/chromium.org/forum/#!msg/site-isolation-dev/XXQRts1W6kE/YeuFS7cqGQAJ
Labels: Test-Layout
Components: -Blink>LayoutTests
Deprecating component:Blink>LayoutTests, to use label Test=Layout instead.
Blockedon: 589894
Cc: kenrb@chromium.org alex...@chromium.org
Adding people who have been looking at OOPIF support for user gestures.
Labels: UserActivation
Cc: mustaq@chromium.org
 Issue 780556  (User Activation v2 support for OOPIF) should make most of those failing layouttests pass.
alexmos@ - I hoped that after your r540240 (forwarding user gesture for cross-process postMessage) these tests would pass, but sadly they still fail... :-(

Note that r532456 has migrated http/tests/bluetooth/https/requestDevice/cross-origin-iframe.html into external/wpt.  FWIW, I see that the following test still times out (at ToT=r541618):
    external/wpt/bluetooth/requestDevice/cross-origin-iframe.sub.https.html

Blockedon: 780556
Status: Available (was: Untriaged)
I dug into this a bit, as I indeed expected external/wpt/bluetooth/requestDevice/cross-origin-iframe.sub.https.html to pass 
after r540240.  It turns out that the tests times out before it ever tries to postMessage the OOPIF while having the user gesture.  It seems that something is wrong with callWithTrustedClick() in the test: it never resolves when the iframe on the page is an OOPIF.  That helper is implemented in bluetooth-helpers.js, which basically injects a dummy button and clicks it using test_driver.click().  Hacking up the test to use eventSender instead to click the button (which I see is deprecated) makes this test pass with OOPIFs enabled.  So we need to understand what's wrong with test_driver.click(), but it's likely not related to OOPIF support for user gesture.

I see that test_driver.click() is implemented in LayoutTests/external/wpt/resources/testdriver.js, which goes to window.test_driver_internal.click in LayoutTests/resources/testdriver-vendor.js, which uses chrome.gpuBenchmarking.pointerActionSequence().  Maybe something's wrong with the latter on this test page?  The page just has a default-sized iframe followed by the injected button.  The coordinates to be clicked match up both with and without OOPIFs.

For reference, as this is now a WPT test, to repro this with OOPIFs enabled you need to use --isolate-origins with WPT origins: i.e., 
--additional-driver-flag=--isolate-origins=http://www.web-platform.test:8001/,http://www1.web-platform.test:8001/,http://www2.web-platform.test:8001/,http://xn--n8j6ds53lwwkrqhv28a.web-platform.test:8001/,http://xn--lve-6lad.web-platform.test:8001/,http://www.web-platform.test:8081/,http://www1.web-platform.test:8081/,http://www2.web-platform.test:8081/,http://xn--n8j6ds53lwwkrqhv28a.web-platform.test:8081/,http://xn--lve-6lad.web-platform.test:8081/,https://www.web-platform.test:8444/,https://www1.web-platform.test:8444/,https://www2.web-platform.test:8444/,https://xn--n8j6ds53lwwkrqhv28a.web-platform.test:8444/,https://xn--lve-6lad.web-platform.test:8444/
Cc: wjmaclean@chromium.org
Owner: dtapu...@chromium.org
Status: Assigned (was: Available)
Summary: chrome.gpuBenchmarking.pointerActionSequence() failure in presence of an OOPIF triggers cross-origin-iframe.html layout test failure. (was: No OOPIF support for UserGestureIndicator triggers cross-origin-iframe.html layout test failure in --site-per-process-mode)
Updating title to reflect the real cause of this test failure - I suspect #19 is what caused the timeout in the first place, and the UG was a secondary problem (now fixed) which would've instead produced a text diff.

Investigating a bit further, I see that the synthetic event generation from chrome.gpuBenchmarking.pointerActionSequence() goes to GpuBenchmarking::PointerActionSequence() which ends up in the browser process via InputInjectorImpl::QueueSyntheticPointerAction(), and then goes to a bunch of logic in SyntheticGestureController, etc which I'm not familiar with.  I did instrument the output of RenderWidgetHostImpl::ForwardMouseEventWithLatencyInfo, and without the OOPIF I see three events:
MouseMove x=110.5 y=191
MouseDown x=110.5 y=191
MouseUp x=110.5 y=191

With the OOPIF on the page, I see:
MouseMove x=0 y=0
MouseDown x=0 y=0
MouseUp x=110.5 y=191

In both cases I think the event is correctly going to the right (root) RWH (and not the OOPIF), but when there's an OOPIF on the page, it seems we've lost proper coordinates for the first two events.  I think the coords are ok when they arrive at InputInjectorImpl::QueueSyntheticPointerAction, so something is broken on the path from that to ForwardMouseEventWithLatencyInfo.

dtapuska@: it seems that you added QueueSyntheticPointerAction (and all of input_injector.mojom).  Would you be able to take a look at this, or know someone who can?  Also +wjmaclean@ who might know this code as well.
Cc: nzolghadr@chromium.org sadrul@chromium.org
It seems dtapuska@ is OOO at the moment, so +nzolghadr@ and also +sadrul@ (based on git blame of SyntheticGestureController) for any ideas re: #19 and #20.
Cc: kereliuk@chromium.org
+kereliuk@, since this also seems related to testdriver.click(), similarily to  https://crbug.com/820617#c2 
alexmos@ can you specify where the wpt test is now that the test is moved to wpt? alternative just copy paste the command you use to run the tests with all the flags and I will use the exact same thing.
Ria the problem happens in this line:
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_input_event_router.cc?type=cs&l=361

It seems that we get some wrong target for the events. It comes from here:
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_input_event_router.cc?type=cs&l=195

Digging further it seems that the surface id is not valid here:
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_view_base.cc?type=cs&l=401

and then no transformed_point is set and it remains it's initial value of 0,0.

It seems that you have written a whole bunch of this code. Do you have any idea why the surface id is invalid there?
Cc: riajiang@chromium.org
Forgot to add Ria.
Cc: dtapu...@chromium.org
Owner: riajiang@chromium.org
Assigning to Ria to investigate.
Cc: fsam...@chromium.org
Navid is correct that |surface_id| is not valid here https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_view_base.cc?type=cs&l=394. I took a closer look and found that we do have a valid FrameSinkId, but the LocalSurfaceId is not valid - this is because the surface was never activated in https://cs.chromium.org/chromium/src/content/browser/renderer_host/delegated_frame_host.cc?type=cs&sq=package:chromium&l=487, which I think means we never submitted any compositor frame for that surface (fsamuel@, is it true that only after we have submitted a compositor frame that we can have a valid active LocalSurfaceId https://cs.chromium.org/chromium/src/content/browser/renderer_host/delegated_frame_host.h?type=cs&l=239?).

Events still go to the correct target because the expected target is the root view, and we do return the valid FrameSinkId for the root view from https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_view_base.cc?type=cs&l=395, we just don't transform the location.

I'm not familiar with how we set up layout tests - can we submit compositor frame to active the frame in the test? Could you point me to a similar passing layout test where OOPIFs are supposed to receive the event?
If I recall correctly, layout tests may not always generate compositor output, and there's a special command to force that ... though the exact details aren't in my head at the moment ...
RE: Could you point me to a similar passing layout test where OOPIFs are supposed to receive the event?

r423762 added OOPIF support to testRunner.eventSender, but this support is limited to simulating the event within the renderer process (i.e. after this CL an OOPIF can use testRunner.eventSender to simulate a mouse click within itself or within other local frames;  but... even after this CL it is not possible to use testRunner.eventSender to simulate clicks in *other* remote frames).

Looking at the changes in r423762, I see that it enabled http/tests/security/referrer-policy-redirect-link.html and http/tests/security/frameNavigation/xss-ALLOWED-parent-navigation-change.html.

I am not sure if this answers your question.  Your question seems to be focused on simulating and targeting mouse events from the *browser* process.  testRunner.eventSender simulates mouse events within a *renderer* process.  It is possible that there are no existing, passing layout tests where OOPIF receives an event via chrome.gpuBenchmarking.pointerActionSequence() - the existing test infrastructure might be missing OOPIF support in this area - we truly appreciate you looking into making sure that chrome.gpuBenchmarking.pointerActionSequence() works correctly with OOPIFs.  
Blockedon: 667551
This work is blocked on Vlad's work on layout test support for OOPIFs.
re c#30: Thanks for the pointer! I asked that question because I thought if there are existing passing layout tests for OOPIFs then I can check how they are activating frames there. I talked with fsamuel@ offline and it seems like we don't have that yet and Vlad is working on that (c#31). Maybe we can check back after that's done?

wjmaclean@, do you remember where we are using that special command for layout tests to generate compositor output?
Project Member

Comment 33 by bugdroid1@chromium.org, Jul 10

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

commit 5e93ea1955845b344d6e137195b0f27638162b06
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Tue Jul 10 17:25:50 2018

Fix OOPIF hit testing in layout tests.

If we haven't received an activated surface yet ensure that we fallback
to querying the renderer for the target of input events.

BUG= 601584 

Change-Id: Id5dfcdb6bf54b98bebbd2a3b715d77202f402781
Reviewed-on: https://chromium-review.googlesource.com/1131219
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573789}
[modify] https://crrev.com/5e93ea1955845b344d6e137195b0f27638162b06/content/browser/renderer_host/render_widget_host_view_base.cc

Project Member

Comment 34 by bugdroid1@chromium.org, Jul 11

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

commit 858ba7c4bf900725f1e1b2802700a9a9b8f3b0a9
Author: Colin Blundell <blundell@chromium.org>
Date: Wed Jul 11 09:38:26 2018

Revert "Fix OOPIF hit testing in layout tests."

This reverts commit 5e93ea1955845b344d6e137195b0f27638162b06.

Reason for revert: FindIt reported this CL as introducing flake ( crbug.com/862478 ). Example failure: https://logs.chromium.org/v/?s=chromium%2Fbuildbucket%2Fcr-buildbucket.appspot.com%2F8941342327988612400%2F%2B%2Fsteps%2Fcontent_browsertests%2F0%2Flogs%2FTracingControllerTest.EnableAndStopTracingWithFilePath%2F0

Original change's description:
> Fix OOPIF hit testing in layout tests.
> 
> If we haven't received an activated surface yet ensure that we fallback
> to querying the renderer for the target of input events.
> 
> BUG= 601584 
> 
> Change-Id: Id5dfcdb6bf54b98bebbd2a3b715d77202f402781
> Reviewed-on: https://chromium-review.googlesource.com/1131219
> Reviewed-by: Ken Buchanan <kenrb@chromium.org>
> Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#573789}

TBR=kenrb@chromium.org,dtapuska@chromium.org

Change-Id: Id34a56853b93c66469a9ae4487f99f94ce3d5817
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  601584 
Reviewed-on: https://chromium-review.googlesource.com/1133018
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574118}
[modify] https://crrev.com/858ba7c4bf900725f1e1b2802700a9a9b8f3b0a9/content/browser/renderer_host/render_widget_host_view_base.cc

Project Member

Comment 35 by bugdroid1@chromium.org, Jul 11

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

commit 3dcee047ec982d521d9ad5301e00ac1bba134f01
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Wed Jul 11 20:17:43 2018

Reland "Fix OOPIF hit testing in layout tests."

This is a reland of 5e93ea1955845b344d6e137195b0f27638162b06

Original change's description:
> Fix OOPIF hit testing in layout tests.
>
> If we haven't received an activated surface yet ensure that we fallback
> to querying the renderer for the target of input events.
>
> BUG= 601584 
>
> Change-Id: Id5dfcdb6bf54b98bebbd2a3b715d77202f402781
> Reviewed-on: https://chromium-review.googlesource.com/1131219
> Reviewed-by: Ken Buchanan <kenrb@chromium.org>
> Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#573789}

TBR=kenrb@chromium.org

Bug:  601584 
Change-Id: I8a2d207db79cc237e1d5e22e5e9bfffb218486c4
Reviewed-on: https://chromium-review.googlesource.com/1133718
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574304}
[modify] https://crrev.com/3dcee047ec982d521d9ad5301e00ac1bba134f01/content/browser/renderer_host/render_widget_host_view_base.cc

Project Member

Comment 36 by bugdroid1@chromium.org, Jul 11

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

commit 08abdaa4ff32eca88a28ab84f7da1075819c311b
Author: Dirk Pranke <dpranke@chromium.org>
Date: Wed Jul 11 22:57:19 2018

Revert "Reland "Fix OOPIF hit testing in layout tests.""

This reverts commit 3dcee047ec982d521d9ad5301e00ac1bba134f01.

Reason for revert: Suspect this is causing more flaky failures, see e.g.: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/linux-chromeos-rel/10567 and build 10568

Original change's description:
> Reland "Fix OOPIF hit testing in layout tests."
> 
> This is a reland of 5e93ea1955845b344d6e137195b0f27638162b06
> 
> Original change's description:
> > Fix OOPIF hit testing in layout tests.
> >
> > If we haven't received an activated surface yet ensure that we fallback
> > to querying the renderer for the target of input events.
> >
> > BUG= 601584 
> >
> > Change-Id: Id5dfcdb6bf54b98bebbd2a3b715d77202f402781
> > Reviewed-on: https://chromium-review.googlesource.com/1131219
> > Reviewed-by: Ken Buchanan <kenrb@chromium.org>
> > Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#573789}
> 
> TBR=kenrb@chromium.org
> 
> Bug:  601584 
> Change-Id: I8a2d207db79cc237e1d5e22e5e9bfffb218486c4
> Reviewed-on: https://chromium-review.googlesource.com/1133718
> Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
> Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#574304}

TBR=kenrb@chromium.org,dtapuska@chromium.org

Change-Id: I9fec8992b35943f8dd2d8ce6bafe8ac9feb49411
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  601584 
Reviewed-on: https://chromium-review.googlesource.com/1134143
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Dirk Pranke <dpranke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574392}
[modify] https://crrev.com/08abdaa4ff32eca88a28ab84f7da1075819c311b/content/browser/renderer_host/render_widget_host_view_base.cc

Project Member

Comment 37 by bugdroid1@chromium.org, Jul 12

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

commit c09ebfd64bd9d402997f4c1ab5624d95347e2245
Author: Dave Tapuska <dtapuska@chromium.org>
Date: Thu Jul 12 17:38:05 2018

Reland "Fix OOPIF hit testing in layout tests."

This is a reland of 5e93ea1955845b344d6e137195b0f27638162b06

The revert of this CL was determined to be the cause of some flaky tests
on the chromeos bot. However those tests are disabled on linux and
have been flaky on chromeos for a long time. See
https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=content_browsertests&builder=chromium.chromiumos%3Alinux-chromeos-rel&sortOrder=forward&sortColumn=test

Original change's description:
> Fix OOPIF hit testing in layout tests.
>
> If we haven't received an activated surface yet ensure that we fallback
> to querying the renderer for the target of input events.
>
> BUG= 601584 
>
> Change-Id: Id5dfcdb6bf54b98bebbd2a3b715d77202f402781
> Reviewed-on: https://chromium-review.googlesource.com/1131219
> Reviewed-by: Ken Buchanan <kenrb@chromium.org>
> Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#573789}

TBR=kenrb@chromium.org

Bug:  601584 
Change-Id: Ifdd26f20bf6d44e704c33a7b7c4380a2de8c3696
Reviewed-on: https://chromium-review.googlesource.com/1135471
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574623}
[modify] https://crrev.com/c09ebfd64bd9d402997f4c1ab5624d95347e2245/content/browser/renderer_host/render_widget_host_view_base.cc

Project Member

Comment 38 by bugdroid1@chromium.org, Aug 13

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

commit 254536e2620d4e194b4d6e2b7c9b3b9f8dc971ad
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Mon Aug 13 18:27:43 2018

Remove even more expectations for tests that have been already fixed.

The bug has been in fixed r574623 - let's remove the test expectations
(FWIW the tests are passing when run 20 times on my local machine).

Bug:  601584 
Change-Id: I5f99d8a066407d3b7be0d5fc1bdaf389de181b87
Reviewed-on: https://chromium-review.googlesource.com/1169956
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582645}
[modify] https://crrev.com/254536e2620d4e194b4d6e2b7c9b3b9f8dc971ad/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process

Status: Fixed (was: Assigned)
AFAIK no more tests are failing because of this - let's mark it as fixed?

Sign in to add a comment