Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dirty canvas when exiting immersive sessions #26164

Merged
merged 2 commits into from Apr 14, 2020

Conversation

@Manishearth
Copy link
Member

Manishearth commented Apr 10, 2020

Fixes #26162, fixes servo/webxr#155

We basically end up in a situation where the main thread is asleep. It's unclear to me why it doesn't wake up after attempts to interact with the screen, but we stopped dirtying the canvas during the immersive session (#26077) so the main thread wasn't awake initially.

This is probably not the cleanest fix -- we really should not be ending up in situations where the main thread is perma-asleep -- however, we should be dirtying the canvas after exiting immersive mode anyway since the contents need to be shown to the user, so this fix is still valid, even if it's not fixing the root cause.

@highfive
Copy link

highfive commented Apr 10, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webglrenderingcontext.rs, components/script/dom/xrsystem.rs
  • @KiChjang: components/script/dom/webglrenderingcontext.rs, components/script/dom/xrsystem.rs
@highfive
Copy link

highfive commented Apr 10, 2020

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@Manishearth
Copy link
Member Author

Manishearth commented Apr 10, 2020

r? @jdm

@highfive highfive assigned jdm and unassigned SimonSapin Apr 10, 2020
@Manishearth Manishearth force-pushed the Manishearth:dirty-endsession branch from e96f0eb to d146303 Apr 10, 2020
@Manishearth
Copy link
Member Author

Manishearth commented Apr 10, 2020

I think this is also part of the fix for servo/webxr#155 , let me check

@Manishearth
Copy link
Member Author

Manishearth commented Apr 10, 2020

Yep, this same trick fixes the other issue.

@jdm
Copy link
Member

jdm commented Apr 10, 2020

Unused import warning is breaking the build.

@asajeffrey
Copy link
Member

asajeffrey commented Apr 10, 2020

This is dirtying the canvas rather than the document, does it wake up the main thread if the canvas is detached from the DOM?

@Manishearth Manishearth force-pushed the Manishearth:dirty-endsession branch from 46af1a0 to 41474cf Apr 10, 2020
@Manishearth
Copy link
Member Author

Manishearth commented Apr 10, 2020

Fixed.

@asajeffrey hmm. This would have already been broken then, right? That seems to be rooted in embedder events not getting consumed when the thread is asleep.

Copy link
Member

jdm left a comment

I am still able to reproduce #26162 with these changes.

@@ -5,6 +5,7 @@
use crate::dom::bindings::cell::DomRefCell;
use crate::dom::bindings::codegen::Bindings::XRSystemBinding::XRSessionInit;
use crate::dom::bindings::codegen::Bindings::XRSystemBinding::{XRSessionMode, XRSystemMethods};
use crate::dom::bindings::codegen::Bindings::XRWebGLLayerBinding::XRWebGLRenderingContext;

This comment has been minimized.

@jdm

jdm Apr 13, 2020

Member
warning: unused import: `crate::dom::bindings::codegen::Bindings::XRWebGLLayerBinding::XRWebGLRenderingContext`
 --> components\script\dom\xrsystem.rs:8:5
  |
8 | use crate::dom::bindings::codegen::Bindings::XRWebGLLayerBinding::XRWebGLRenderingContext;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default
@jdm
Copy link
Member

jdm commented Apr 13, 2020

Since I don't think #26179 is related to these changes, I think we should go ahead and merge them with the unused import removed.

@Manishearth Manishearth force-pushed the Manishearth:dirty-endsession branch from 41474cf to 453be48 Apr 13, 2020
@Manishearth
Copy link
Member Author

Manishearth commented Apr 13, 2020

@bors-servo r=jdm

@bors-servo
Copy link
Contributor

bors-servo commented Apr 13, 2020

📌 Commit 453be48 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Apr 13, 2020

Testing commit 453be48 with merge 7b0f386...

bors-servo added a commit that referenced this pull request Apr 13, 2020
Dirty canvas when exiting immersive sessions

Fixes #26162, fixes servo/webxr#155

We basically end up in a situation where the main thread is asleep. It's unclear to me why it doesn't wake up after attempts to interact with the screen, but we stopped dirtying the canvas during the immersive session (#26077) so the main thread wasn't awake initially.

This is probably not the cleanest fix -- we really should not be ending up in situations where the main thread is perma-asleep -- however, we should be dirtying the canvas after exiting immersive mode _anyway_ since the contents need to be shown to the user, so this fix is still valid, even if it's not fixing the root cause.
@bors-servo
Copy link
Contributor

bors-servo commented Apr 13, 2020

💔 Test failed - status-taskcluster

@jdm
Copy link
Member

jdm commented Apr 13, 2020

@bors-servo
Copy link
Contributor

bors-servo commented Apr 13, 2020

Testing commit 453be48 with merge b13658a...

bors-servo added a commit that referenced this pull request Apr 13, 2020
Dirty canvas when exiting immersive sessions

Fixes #26162, fixes servo/webxr#155

We basically end up in a situation where the main thread is asleep. It's unclear to me why it doesn't wake up after attempts to interact with the screen, but we stopped dirtying the canvas during the immersive session (#26077) so the main thread wasn't awake initially.

This is probably not the cleanest fix -- we really should not be ending up in situations where the main thread is perma-asleep -- however, we should be dirtying the canvas after exiting immersive mode _anyway_ since the contents need to be shown to the user, so this fix is still valid, even if it's not fixing the root cause.
@bors-servo
Copy link
Contributor

bors-servo commented Apr 13, 2020

💔 Test failed - status-taskcluster

@Manishearth
Copy link
Member Author

Manishearth commented Apr 14, 2020

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented Apr 14, 2020

Testing commit 453be48 with merge 33a74a4...

@bors-servo
Copy link
Contributor

bors-servo commented Apr 14, 2020

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing 33a74a4 to master...

@bors-servo bors-servo merged commit 33a74a4 into servo:master Apr 14, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.