A big chunk of this crate seems unsound to me

[nox]

Many APIs in there use a Arc<RefCell<T>>, and then use that as raw pointers somehow on the Servo side. That seems highly suspicious to me. @MortimerGoro Is there some documentation somewhere about how this is all used?

[MortimerGoro]

There is some doc about how raw pointers are used here: https://github.com/servo/servo/blob/master/components/webvr/webvr_thread.rs#L294

Besides Servo WebVR implementation, one of the use cases intended for this library was to allow using it by any standalone rust app. Arc<T> seemed like a good default for general usage by any library that could opt for different threading strategies

[nox]

My issue is that Arc<RefCell<T>> is as useful as RefCell<T> in a multithreaded context, given Arc<T> requires T: Send + Sync to be Send or Sync, and RefCell<T> isn't Sync.

[MortimerGoro]

I get the point. Maybe is not well documented but the assumption is that nobody is going to sync poses or submit frames to a VRDevice from different threads at the same time (== calling mutable methods). That would fail on the underlying native SDKs even using Arc<Mutex<>> to implement sync on the Rust side. So I didn't add the Mutex because I felt that it was just adding some overhead for nothing (And I'm very obssesed with latency :D)

If there is a better container to express all that at compile time that would be great. The use case is a shared pointer reference that can send between threads, inmutable methods can be called safely from any thread, but mutable methods are only called from the same render thread at a time (it could be sent to another render thread for a different VR sessionm but only one at a time).

[nox]

When you say "that would fail", how would the failure manifest itself?

[MortimerGoro]

Undefined crashes per platform, I mean that the native SDKs are not expected to be called from different threads at the same time for submitting frames or syncing the pose.

[nox]

Ok, then that should definitely never be doable from safe code. :) Not like we are going to resolve this issue today though. I think we should wait and discuss about that IRL when our overlord Lars has a date for the workweek. :3

[Manishearth]

Just stumbled across this: We should at least swap the Arc<RefCell>s out for Rc<RefCell> or Arc<Mutex>, Arc<RefCell> is no different from Rc<RefCell> in what you're allowed to do with it, but has unnecessary overhead.

The use case is a shared pointer reference that can send between threads, inmutable methods can be called safely from any thread, but mutable methods are only called from the same render thread at a time

This isn't safe; you shouldn't be able to read from the other threads whilst the render thread is mutating.

I think this should just be Rc<RefCell> for now.

[MortimerGoro]

+1 for Rc<RefCell>

[asajeffrey]

And now it's my turn.

If the idea is to have a read-only view of the display that can be used from any thread, and a read-write view that can only be used from the thread that created it, then are we wanting two types? VRDisplay and ROVRDisplay, where only ROVRDisplay implements Sync and Send? Under the hood this would probably need to use a RWLock to ensure that there's no read/write conflict.

[asajeffrey]

AFAICT the implementations allow UB from safe code, due to (e.g.)

rust-webvr/rust-webvr/src/api/oculusvr/service.rs

Line 27 in 2e1cbcd

unsafe impl Send for OculusVRService {}

To get UB:

• Thread 1 creates a service, and uses it to get a VRDisplayPtr.
• Thread 1 sends the service to thread 2.
• Thread 2 uses the service to get the same VRDisplayPtr.
• Both threads try to get read access, which updates the borrow counts on the RefCell, resulting in a write/write conflict.

I don't think this will be fixed by moving to Rc<RefCell>.

[asajeffrey]

A conversation with @MortimerGoro on slack today (so no permalink because it's not public).

The argument for soundness seems to be that there's two threads with access to the same VRDisplay: the VR thread and the GL thread. The GL thread has mutable access, the VR thread has immutable access. Buuuuuut the JS thread only ever calls methods that involve one thread or the other, so we don't end up with r/w conflict.

I'm not sure about this, e.g. https://github.com/servo/servo/blob/81ab255b70cd5cc2a3ef6f27414c3655ed26a82f/components/script/dom/vrdisplay.rs#L201 sends a GetFrameData message to the VR thread (and blocks waiting for the result), and https://github.com/servo/servo/blob/81ab255b70cd5cc2a3ef6f27414c3655ed26a82f/components/script/dom/vrdisplay.rs#L390 sends a SubmitFrame message to the GL thread (and doesn't block). So it looks to me that if JS calls SubmitFrame then GetFrameData immediately afterwards, then we can end up with a r/w conflict.

[asajeffrey]

Oh, and I should add that this is coming up for me because I'm implementing the magicleap back end, for which the VRDisplay should keep a GL context. Since GL contexts have thread affinity, life would be much simpler if we didn't send VRDisplay objects, but instead created them on the thread where they'll be used.

[MortimerGoro]

I'm not sure about this, e.g. https://github.com/servo/servo/blob/81ab255b70cd5cc2a3ef6f27414c3655ed26a82f/components/script/dom/vrdisplay.rs#L201 sends a GetFrameData message to the VR thread (and blocks waiting for the result), and https://github.com/servo/servo/blob/81ab255b70cd5cc2a3ef6f27414c3655ed26a82f/components/script/dom/vrdisplay.rs#L390 sends a SubmitFrame message to the GL thread (and doesn't block). So it looks to me that if JS calls SubmitFrame then GetFrameData immediately afterwards, then we can end up with a r/w conflict.

GetFrameData only goes thought the WebVR thread when it's not presenting (also called magic window mode). In that situation SubmitFrame cannot be called from JS.
https://github.com/servo/servo/blob/81ab255b70cd5cc2a3ef6f27414c3655ed26a82f/components/script/dom/vrdisplay.rs#L217

When it's presenting the FrameData it's received from the GL thread that generated it.

Oh, and I should add that this is coming up for me because I'm implementing the magicleap back end, for which the VRDisplay should keep a GL context. Since GL contexts have thread affinity, life would be much simpler if we didn't send VRDisplay objects, but instead created them on the thread where they'll be used.

Could you create the GLContext only when WebXR presentation starts or do you need it earlier?

As mentioned in slack once we have support for all the external textures in Servo WebGL (e.g. SurfaceTextures on Android, IOSurfaces on mac, etc) we won't need to call the SubmitFrame() and other render methods from the same WebGL thread. That could simplify a lot of things and maybe we can move all the code to a single thread in webvr_thread.rs

[asajeffrey]

Hmm, so the safety relies on the fact that the display is still presenting when GetFrameData is called? So if the display is paused between SubmitFrame and GetFrameData, can this result in UB? I'd be much happier about this API if it exposed a safe surface and didn't need unsafe code in script together with reasoning for safety that's too complicated for my little brain.

I could create the GL context when the presentation starts, but it would be much nicer if I could make the VRDisplay owning the GL context !Sync and !Send so I could be confident that GL calls only happen on the right thread.