[WebXR] Replace the session reference in WebXRSpace subclasses with weak pointers
Created attachment 440718 [details] Patch
<rdar://problem/84066341>
Comment on attachment 440718 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440718&action=review > Source/WebCore/ChangeLog:12 > + WebXRSpace had a pure virtual session() accessor that returned > + a reference to a WebXRSession. This made subclasses hold strong > + references to the WebXRSession, and would become problematic > + for a WebXRSpace subclass that was (indirectly) owned by the > + WebXRSession (found in the Hand Input module). Pretty sure this was already causing leaks elsewhere for other XRSpaces, like the targetRaySpace and gripSpace in WebXRInputSource. > Source/WebCore/Modules/webxr/WebXRReferenceSpace.cpp:72 > + if (!m_session) > + return identity; This feels a bit weird to me. It at least needs a comment explaining itself, but I would assume this would propagate an Exception to the caller. > Source/WebCore/Modules/webxr/WebXRReferenceSpace.cpp:120 > + if (!m_session) > + return { }; Same as above. > Source/WebCore/Modules/webxr/WebXRSpace.cpp:61 > + WebXRSession* xrSession = session(); > + if (!xrSession) > + return false; Seems like this should be an exception rather than just returning false.
Created attachment 440732 [details] EWS test
Comment on attachment 440718 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440718&action=review >> Source/WebCore/Modules/webxr/WebXRReferenceSpace.cpp:72 >> + return identity; > > This feels a bit weird to me. It at least needs a comment explaining itself, but I would assume this would propagate an Exception to the caller. Good point. I changed ::nativeOrigin to return an optional<TransformationMatrix>, updated all the subclasses, and return an Exception at the calling site if the session has disappeared. >> Source/WebCore/Modules/webxr/WebXRReferenceSpace.cpp:120 >> + return { }; > > Same as above. Ditto. >> Source/WebCore/Modules/webxr/WebXRSpace.cpp:61 >> + return false; > > Seems like this should be an exception rather than just returning false. Done. Similar solution - this returns an optional that ends up returning an exception in the caller.
Committed r283880 (242757@main): <https://commits.webkit.org/242757@main>