Do not allow XR session on non user-activated events #24133

paulrouget · 2019-09-04T07:01:31Z

This sets the thread in "user interaction mode" when the dispatched event is trusted. I also tried an approach where we would not rely on the dispatched event but just set "user interaction mode" when we get a compositor event (which, we can assume, are only user generated). That worked as well.

Fixes #23787.

This change is

highfive · 2019-09-04T07:01:36Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/script_thread.rs, components/script/dom/xrtest.rs, components/script/dom/event.rs, components/script/dom/xr.rs
@KiChjang: components/script/script_thread.rs, components/script/dom/xrtest.rs, components/script/dom/event.rs, components/script/dom/xr.rs

highfive · 2019-09-04T07:01:38Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify script code, but no tests are modified. Please consider adding a test!

paulrouget · 2019-09-04T12:16:54Z

Alright. Gonna use the compositor event approach instead.

paulrouget · 2019-09-04T13:21:49Z

Done.

jdm · 2019-09-04T14:10:47Z

@bors-servo r+

bors-servo · 2019-09-04T14:10:48Z

📌 Commit fd25e9e has been approved by jdm

bors-servo · 2019-09-05T06:58:58Z

⌛ Testing commit fd25e9e with merge d05ebf3...


        Auto merge of #24133 - paulrouget:userAction, r=jdm

@jdm

Do not allow XR session on non user-activated events @jdm can you look at this? This sets the thread in "user interaction mode" when the dispatched event is trusted. I also tried an approach where we would not rely on the dispatched event but just set "user interaction mode" when we get a compositor event (which, we can assume, are only user generated). That worked as well. r? @jdm  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24133)

bors-servo · 2019-09-05T07:44:53Z

💔 Test failed - linux-rel-wpt

CYBAI · 2019-09-05T07:49:25Z

{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "Requesting immersive session outside of a user gesture rejects", 
    "test": "/webxr/xrDevice_requestSession_immersive_no_gesture.https.html", 
    "line": 374587, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "TIMEOUT", 
    "group": "default", 
    "message": "Test timed out", 
    "stack": null, 
    "subtest": "create_session", 
    "test": "/_mozilla/webxr/create_session.html", 
    "line": 393534, 
    "action": "test_result", 
    "expected": "PASS"
}
{
    "status": "ERROR", 
    "group": "default", 
    "message": "The operation is insecure.", 
    "stack": "undefined:undefined:undefined", 
    "subtest": null, 
    "test": "/_mozilla/webxr/create_session.html", 
    "line": 393535, 
    "action": "test_result", 
    "expected": "OK"
}
{
    "status": "FAIL", 
    "group": "default", 
    "message": "promise_test: Unhandled rejection with value: object \"SecurityError: The operation is insecure.\"", 
    "stack": null, 
    "subtest": "obtain_frame", 
    "test": "/_mozilla/webxr/obtain_frame.html", 
    "line": 394168, 
    "action": "test_result", 
    "expected": "PASS"
}
{
    "status": "FAIL", 
    "group": "default", 
    "message": "promise_test: Unhandled rejection with value: object \"SecurityError: The operation is insecure.\"", 
    "stack": null, 
    "subtest": "Ensure that XRWebGLLayer's constructor throws appropriate errors", 
    "test": "/webxr/xrWebGLLayer_constructor.https.html", 
    "line": 401177, 
    "action": "test_result", 
    "expected": "TIMEOUT"
}
{
    "status": "OK", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": null, 
    "test": "/webxr/xrWebGLLayer_constructor.https.html", 
    "line": 401178, 
    "action": "test_result", 
    "expected": "ERROR"
}

paulrouget · 2019-09-12T05:44:10Z

@bors-servo try=wpt

bors-servo · 2019-09-12T05:44:14Z

⌛ Trying commit f0bd0e7 with merge 30ad319...


        Auto merge of #24133 - paulrouget:userAction, r=<try>

@jdm

Do not allow XR session on non user-activated events @jdm can you look at this? This sets the thread in "user interaction mode" when the dispatched event is trusted. I also tried an approach where we would not rely on the dispatched event but just set "user interaction mode" when we get a compositor event (which, we can assume, are only user generated). That worked as well. r? @jdm  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24133)

servo-wpt-sync · 2019-09-12T05:45:36Z

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#19018.

bors-servo · 2019-09-12T06:34:24Z

💔 Test failed - linux-rel-wpt

paulrouget · 2019-09-12T08:10:43Z

@bors-servo try=wpt

atouchet · 2019-09-19T03:43:19Z

This looks like it needs to be restarted.

jdm · 2019-09-19T12:48:35Z

@bors-servo retry

bors-servo · 2019-09-19T12:48:40Z

⌛ Testing commit 30eca13 with merge d65c0dc...


        Auto merge of #24133 - paulrouget:userAction, r=jdm

@jdm

Do not allow XR session on non user-activated events @jdm can you look at this? This sets the thread in "user interaction mode" when the dispatched event is trusted. I also tried an approach where we would not rely on the dispatched event but just set "user interaction mode" when we get a compositor event (which, we can assume, are only user generated). That worked as well. r? @jdm  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24133)

jdm · 2019-09-19T14:47:12Z

This needs a ./mach fmt, sadly.

bors-servo · 2019-09-19T15:38:24Z

💔 Test failed - status-taskcluster

servo-wpt-sync · 2019-09-20T05:50:10Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#19051.

servo-wpt-sync · 2019-09-20T05:50:25Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#19051.


        Do not allow XR session on non user-activated events

servo-wpt-sync · 2019-09-20T05:55:56Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#19051.

paulrouget · 2019-09-20T10:13:39Z

@bors-servo r=jdm

bors-servo · 2019-09-20T10:13:40Z

📌 Commit ea7b581 has been approved by jdm

bors-servo · 2019-09-20T10:54:22Z

⌛ Testing commit ea7b581 with merge 293ccd0...


        Auto merge of #24133 - paulrouget:userAction, r=jdm

Do not allow XR session on non user-activated events This sets the thread in "user interaction mode" when the dispatched event is trusted. I also tried an approach where we would not rely on the dispatched event but just set "user interaction mode" when we get a compositor event (which, we can assume, are only user generated). That worked as well. Fixes #23787.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24133)

bors-servo · 2019-09-20T13:20:19Z

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: jdm
Pushing 293ccd0 to master...

Manishearth · 2019-09-24T17:33:16Z

tests/wpt/web-platform-tests/webxr/xrWebGLLayer_constructor.https.html

-        .then((session) => {
+      let sessionPromise;
+      navigator.xr.test.simulateUserActivation(function() {
+        sessionPromise = navigator.xr.requestSession('inline');


Note: this was a mistake, inline sessions with no tracking data (which must be explicitly requested via features) don't need activation

File an issue? That sounds like a mistake in our implementation.

We don't support inline (#24186) or features (#24270), so this test will fail anyway.

In other words, we don't have enough of an implementation of the prerequisites to write the code for this. Paul's implementation is correct for the features we have right now, it's just the test change that isn't. Alex Cooper is already fixing the test so we're fine, I just wanted to note this.

highfive assigned jdm Sep 4, 2019

highfive added the S-awaiting-review label Sep 4, 2019

paulrouget force-pushed the paulrouget:userAction branch from a303222 to fd25e9e Sep 4, 2019

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 4, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 5, 2019

paulrouget force-pushed the paulrouget:userAction branch from fd25e9e to f0bd0e7 Sep 12, 2019

highfive added S-awaiting-review and removed S-tests-failed labels Sep 12, 2019

servo-wpt-sync mentioned this pull request Sep 12, 2019

Do not allow XR session on non user-activated events web-platform-tests/wpt#19018

Closed

highfive added the S-tests-failed label Sep 12, 2019

paulrouget force-pushed the paulrouget:userAction branch from f0bd0e7 to 316a815 Sep 12, 2019

highfive removed the S-tests-failed label Sep 12, 2019

jdm added the S-awaiting-merge label Sep 13, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Sep 19, 2019

paulrouget force-pushed the paulrouget:userAction branch from 30eca13 to e6d2d77 Sep 20, 2019

highfive added S-awaiting-review and removed S-tests-failed labels Sep 20, 2019

paulrouget force-pushed the paulrouget:userAction branch from e6d2d77 to 014e5d4 Sep 20, 2019

Do not allow XR session on non user-activated events

Loading status checks…

ea7b581

paulrouget force-pushed the paulrouget:userAction branch from 014e5d4 to ea7b581 Sep 20, 2019

highfive added S-awaiting-merge and removed S-awaiting-review labels Sep 20, 2019

bors-servo merged commit ea7b581 into servo:master Sep 20, 2019
3 checks passed

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Sep 20, 2019

bors-servo mentioned this pull request Sep 20, 2019

Improve spec compliance of discarding BCs #24143

Merged

0 of 5 tasks complete

Manishearth reviewed Sep 24, 2019

View changes

Manishearth mentioned this pull request Sep 24, 2019

Support features in webxr, require feature for local-floor access #24270

Closed

servo / servo

Do not allow XR session on non user-activated events #24133

Do not allow XR session on non user-activated events #24133

paulrouget commented Sep 4, 2019 •

edited by jdm

highfive commented Sep 4, 2019

highfive commented Sep 4, 2019

paulrouget commented Sep 4, 2019

paulrouget commented Sep 4, 2019

jdm commented Sep 4, 2019

bors-servo commented Sep 4, 2019

bors-servo commented Sep 5, 2019

bors-servo commented Sep 5, 2019

CYBAI commented Sep 5, 2019

paulrouget commented Sep 12, 2019

bors-servo commented Sep 12, 2019

servo-wpt-sync commented Sep 12, 2019

bors-servo commented Sep 12, 2019

paulrouget commented Sep 12, 2019

atouchet commented Sep 19, 2019

jdm commented Sep 19, 2019

bors-servo commented Sep 19, 2019

jdm commented Sep 19, 2019

bors-servo commented Sep 19, 2019

servo-wpt-sync commented Sep 20, 2019

servo-wpt-sync commented Sep 20, 2019

servo-wpt-sync commented Sep 20, 2019

paulrouget commented Sep 20, 2019

bors-servo commented Sep 20, 2019

bors-servo commented Sep 20, 2019

bors-servo commented Sep 20, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

servo / servo

Join GitHub today

Do not allow XR session on non user-activated events #24133

Do not allow XR session on non user-activated events #24133

Conversation

paulrouget commented Sep 4, 2019 • edited by jdm

highfive commented Sep 4, 2019

highfive commented Sep 4, 2019

paulrouget commented Sep 4, 2019

paulrouget commented Sep 4, 2019

jdm commented Sep 4, 2019

bors-servo commented Sep 4, 2019

bors-servo commented Sep 5, 2019

bors-servo commented Sep 5, 2019

CYBAI commented Sep 5, 2019

paulrouget commented Sep 12, 2019

bors-servo commented Sep 12, 2019

servo-wpt-sync commented Sep 12, 2019

bors-servo commented Sep 12, 2019

paulrouget commented Sep 12, 2019

atouchet commented Sep 19, 2019

jdm commented Sep 19, 2019

bors-servo commented Sep 19, 2019

jdm commented Sep 19, 2019

bors-servo commented Sep 19, 2019

servo-wpt-sync commented Sep 20, 2019

servo-wpt-sync commented Sep 20, 2019

servo-wpt-sync commented Sep 20, 2019

paulrouget commented Sep 20, 2019

bors-servo commented Sep 20, 2019

bors-servo commented Sep 20, 2019

bors-servo commented Sep 20, 2019

This comment has been minimized.

Manishearth Sep 24, 2019

This comment has been minimized.

jdm Sep 24, 2019

This comment has been minimized.

Manishearth Sep 24, 2019

This comment has been minimized.

Manishearth Sep 24, 2019

paulrouget commented Sep 4, 2019 •

edited by jdm