chrome.gpuBenchmarking.pointerActionSequence() failure in presence of an OOPIF triggers cross-origin-iframe.html layout test failure. |
|||||||||||||||||||||
Issue descriptionSite 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 ]
,
Apr 7 2016
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.
,
Apr 7 2016
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.
,
Apr 7 2016
"Investigate and implement support for UserGestureIndicator with OOPIFs" is in Q2 OKRs for site-isolation :-) Therefore - let me move jyasskin@ to CC.
,
Apr 7 2016
,
Apr 7 2016
,
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
,
Apr 8 2016
Let me know if you need me to do anything about this. If the gesture isn't propagated, #3 is the expected error.
,
Apr 8 2016
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
,
May 18 2016
,
May 18 2016
Deprecating component:Blink>LayoutTests, to use label Test=Layout instead.
,
Jun 15 2016
,
Oct 3 2016
Adding people who have been looking at OOPIF support for user gestures.
,
Nov 1 2017
,
Nov 1 2017
,
Mar 9 2018
Issue 780556 (User Activation v2 support for OOPIF) should make most of those failing layouttests pass.
,
Mar 9 2018
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
,
Mar 9 2018
,
Mar 9 2018
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/
,
Mar 9 2018
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.
,
Mar 9 2018
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.
,
Mar 12 2018
+kereliuk@, since this also seems related to testdriver.click(), similarily to https://crbug.com/820617#c2
,
Mar 12 2018
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.
,
Mar 12 2018
#23: here's the command line I used to run the test: run-webkit-tests -v -t release --driver-logging --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/ --no-retry-failures --no-show-results external/wpt/bluetooth/requestDevice/cross-origin-iframe.sub.https.html
,
Mar 15 2018
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?
,
Mar 15 2018
Forgot to add Ria.
,
Mar 19 2018
Assigning to Ria to investigate.
,
Mar 22 2018
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?
,
Mar 22 2018
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 ...
,
Mar 22 2018
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.
,
Mar 22 2018
,
Mar 23 2018
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?
,
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
,
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
,
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
,
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
,
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
,
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
,
Aug 13
AFAIK no more tests are failing because of this - let's mark it as fixed? |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by dcheng@chromium.org
, Apr 7 2016Status: Assigned (was: Untriaged)