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...

@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...

@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...

@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.

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...

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 There is new code that needs to be reviewed. label Sep 4, 2019

paulrouget force-pushed the userAction branch from a303222 to fd25e9e Compare September 4, 2019 13:07

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Sep 4, 2019

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Sep 5, 2019

paulrouget force-pushed the userAction branch from fd25e9e to f0bd0e7 Compare September 12, 2019 05:42

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. 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 The changes caused existing tests to fail. label Sep 12, 2019

paulrouget force-pushed the userAction branch from f0bd0e7 to 316a815 Compare September 12, 2019 08:09

highfive removed the S-tests-failed The changes caused existing tests to fail. label Sep 12, 2019

jdm added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Sep 13, 2019

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Sep 19, 2019

paulrouget force-pushed the userAction branch from 30eca13 to e6d2d77 Compare September 20, 2019 05:48

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Sep 20, 2019

paulrouget force-pushed the userAction branch from e6d2d77 to 014e5d4 Compare September 20, 2019 05:48

Do not allow XR session on non user-activated events

ea7b581

paulrouget force-pushed the userAction branch from 014e5d4 to ea7b581 Compare September 20, 2019 05:53

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Sep 20, 2019

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

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Sep 20, 2019

bors-servo mentioned this pull request Sep 20, 2019

Improve spec compliance of discarding BCs #24143

Merged

5 tasks

Manishearth reviewed Sep 24, 2019

View reviewed changes

Manishearth mentioned this pull request Sep 24, 2019

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Manishearth Sep 24, 2019

jdm Sep 24, 2019

Manishearth Sep 24, 2019

Manishearth Sep 24, 2019

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

Manishearth Sep 24, 2019

Choose a reason for hiding this comment

jdm Sep 24, 2019

Choose a reason for hiding this comment

Manishearth Sep 24, 2019

Choose a reason for hiding this comment

Manishearth Sep 24, 2019

Choose a reason for hiding this comment

paulrouget commented Sep 4, 2019 •

edited by jdm