Bug 221225

Created attachment 418928 [details] wip

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

<rdar://problem/74112910>

Created attachment 420125 [details] Patch

Comment on attachment 420125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420125&action=review > Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:41 I would pass a WebXRSession& if possible. > Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:82 auto > Source/WebCore/Modules/webxr/WebXRFrame.cpp:49 > + , m_session(WTFMove(session)) Would be slightly more consistent to initialise m_session before m_IsAnimationFrame. Also s/m_IsAnimationFrame/m_isAnimationFrame/ > Source/WebCore/Modules/webxr/WebXRFrame.cpp:58 > + auto isOutsideNativeBoundsOfBoundedReferenceSpace = [] (const WebXRSpace& space, const WebXRSpace&) { Might be better as a free function. > Source/WebCore/Modules/webxr/WebXRFrame.cpp:74 > + auto isLocalReferenceSpace = [] (const WebXRSpace& space) { Ditto. > Source/WebCore/Modules/webxr/WebXRFrame.cpp:87 > + // than 15m (suggested by specs) return true. Should we return true here? > Source/WebCore/Modules/webxr/WebXRFrame.cpp:97 > + bool emulatedPosition; Might be good to initialise the value if possible > Source/WebCore/Modules/webxr/WebXRFrame.cpp:192 > + const double near = m_session->renderState().depthNear(); s/const // > Source/WebCore/Modules/webxr/WebXRFrame.cpp:203 > + aspect = (double) layer->framebufferWidth() / (double) layer->framebufferHeight(); static_cast > Source/WebCore/Modules/webxr/WebXRFrame.cpp:205 > + const double far = m_session->renderState().depthFar(); s/const // > Source/WebCore/Modules/webxr/WebXRFrame.cpp:209 > + xrView->setProjectionMatrix(projection); WTFMove(projection)? > Source/WebCore/Modules/webxr/WebXRFrame.cpp:213 > + ++index; We could replace index by checking xrViews size > Source/WebCore/Modules/webxr/WebXRFrame.cpp:237 > + RefPtr<WebXRPose> pose = WebXRPose::create(WebXRRigidTransform::create(populateValue->transform), populateValue->emulatedPosition); directly return here? > Source/WebCore/Modules/webxr/WebXRSession.cpp:493 > + auto frame = WebXRFrame::create(makeRef(*this), true /*isAnimationFrame*/); Would read better by using an enum instead of a bool. > Source/WebCore/Modules/webxr/WebXRSession.cpp:550 > + // 2. If the request does not originate from document, return false. It does not seem like you implement the check here. > Source/WebCore/Modules/webxr/WebXRSpace.cpp:41 > +WebXRSpace::WebXRSpace(Document& document, WeakPtr<WebXRSession>&& session, Ref<WebXRRigidTransform>&& offset) I would pass a WebXRSession& > Source/WebCore/Modules/webxr/WebXRView.cpp:50 > +void WebXRView::setProjectionMatrix(const std::array<float, 16>& projection) Might be left to a follow-up but why not passing directly a Ref<Float32Array>&& here? > Source/WebCore/Modules/webxr/WebXRView.h:50 > + const Float32Array& projectionMatrix() const { return *m_projection; } How can we guarantee that m_projection is not null? Is there a way we could pass a Ref<Float32Array> in the constructor? > Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:606 > + const double upTan = tan(fovUp); why const? upTan is only used once so I am not sure it adds much. > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:308 > + XrPosef identityPose = { s / = // > LayoutTests/platform/wpe/TestExpectations:641 > +imported/w3c/web-platform-tests/webxr/xrFrame_getViewerPose_getPose.https.html [ Pass ] Could they be enabled cross-platform?

Comment on attachment 420125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420125&action=review >> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:41 >> +Ref<WebXRBoundedReferenceSpace> WebXRBoundedReferenceSpace::create(Document& document, WeakPtr<WebXRSession>&& session, XRReferenceSpaceType type) > > I would pass a WebXRSession& if possible. I don't think it's safe because JS can hold XRSpace instances after a XRSession is destroyed. >> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:82 >> + Ref<WebXRRigidTransform> offset = WebXRRigidTransform::create(m_originOffset->rawTransform() * offsetTransform.rawTransform()); > > auto done >> Source/WebCore/Modules/webxr/WebXRFrame.cpp:49 >> + , m_session(WTFMove(session)) > > Would be slightly more consistent to initialise m_session before m_IsAnimationFrame. > Also s/m_IsAnimationFrame/m_isAnimationFrame/ done >> Source/WebCore/Modules/webxr/WebXRFrame.cpp:58 >> + auto isOutsideNativeBoundsOfBoundedReferenceSpace = [] (const WebXRSpace& space, const WebXRSpace&) { > > Might be better as a free function. done >> Source/WebCore/Modules/webxr/WebXRFrame.cpp:87 >> + // than 15m (suggested by specs) return true. > > Should we return true here? Only if the distance is greater than 15m. I want to implement the limits logic there and in other paths in a separate follow-up patch to not make this one more complex. >> Source/WebCore/Modules/webxr/WebXRFrame.cpp:97 >> + bool emulatedPosition; > > Might be good to initialise the value if possible done >> Source/WebCore/Modules/webxr/WebXRFrame.cpp:192 >> + const double near = m_session->renderState().depthNear(); > > s/const // done >> Source/WebCore/Modules/webxr/WebXRFrame.cpp:203 >> + aspect = (double) layer->framebufferWidth() / (double) layer->framebufferHeight(); > > static_cast done >> Source/WebCore/Modules/webxr/WebXRFrame.cpp:205 >> + const double far = m_session->renderState().depthFar(); > > s/const // done >> Source/WebCore/Modules/webxr/WebXRFrame.cpp:209 >> + xrView->setProjectionMatrix(projection); > > WTFMove(projection)? done, moved to the constructor >> Source/WebCore/Modules/webxr/WebXRFrame.cpp:213 >> + ++index; > > We could replace index by checking xrViews size done >> Source/WebCore/Modules/webxr/WebXRFrame.cpp:237 >> + RefPtr<WebXRPose> pose = WebXRPose::create(WebXRRigidTransform::create(populateValue->transform), populateValue->emulatedPosition); > > directly return here? done >> Source/WebCore/Modules/webxr/WebXRSession.cpp:493 >> + auto frame = WebXRFrame::create(makeRef(*this), true /*isAnimationFrame*/); > > Would read better by using an enum instead of a bool. done >> Source/WebCore/Modules/webxr/WebXRView.cpp:50 >> +void WebXRView::setProjectionMatrix(const std::array<float, 16>& projection) > > Might be left to a follow-up but why not passing directly a Ref<Float32Array>&& here? I moved it to the constructor >> Source/WebCore/Modules/webxr/WebXRView.h:50 >> + const Float32Array& projectionMatrix() const { return *m_projection; } > > How can we guarantee that m_projection is not null? > Is there a way we could pass a Ref<Float32Array> in the constructor? done >> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:606 >> + const double upTan = tan(fovUp); > > why const? upTan is only used once so I am not sure it adds much. Done. I'm used to default immutability in Rust so I'll tend to add const by default :) >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:308 >> + XrPosef identityPose = { > > s / = // done >> LayoutTests/platform/wpe/TestExpectations:641 >> +imported/w3c/web-platform-tests/webxr/xrFrame_getViewerPose_getPose.https.html [ Pass ] > > Could they be enabled cross-platform? I'll address that as a follow-up

(In reply to Imanol Fernandez from comment #6) > Comment on attachment 420125 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=420125&action=review > > >> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:41 > >> +Ref<WebXRBoundedReferenceSpace> WebXRBoundedReferenceSpace::create(Document& document, WeakPtr<WebXRSession>&& session, XRReferenceSpaceType type) > > > > I would pass a WebXRSession& if possible. > > I don't think it's safe because JS can hold XRSpace instances after a > XRSession is destroyed. I was meaning to pass a WebXRSession& but store a WeakPtr<>. This makes it clear for instance that session cannot be null.

Created attachment 420304 [details] Patch Address review feedback

(In reply to youenn fablet from comment #7) > (In reply to Imanol Fernandez from comment #6) > > Comment on attachment 420125 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=420125&action=review > > > > >> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:41 > > >> +Ref<WebXRBoundedReferenceSpace> WebXRBoundedReferenceSpace::create(Document& document, WeakPtr<WebXRSession>&& session, XRReferenceSpaceType type) > > > > > > I would pass a WebXRSession& if possible. > > > > I don't think it's safe because JS can hold XRSpace instances after a > > XRSession is destroyed. > > I was meaning to pass a WebXRSession& but store a WeakPtr<>. > This makes it clear for instance that session cannot be null. ah ok. I tried to use WebXRSession& initially but there is a problem with getOffsetReferencespace. In that case we'd need to convert the WeakPtr<WebXRSession> to WebXRSession to create the new XRReferenceSpace. The spec does not define any exception for that case: https://immersive-web.github.io/webxr/#dom-xrreferencespace-getoffsetreferencespace The exceptions are handled in getPose or getViewerPose calls

WebXRSpace was previously taking a Ref<WebXRSession> and this patch is changing it to a WeakPtr<WebXRSession>. With this change, WebXRSpace needs to handle the case of a null WebXRSession, while previously, it was not possible. Can you describe why this is good to do so? Are we getting closer to spec? It seems this particular change could potentially be observable (before GC, WebXRSpace does something, after GC it does something different).

Created attachment 420336 [details] Patch Fix build error in wincairo: near and far are non-standard extension keywords

(In reply to youenn fablet from comment #10) > WebXRSpace was previously taking a Ref<WebXRSession> and this patch is > changing it to a WeakPtr<WebXRSession>. > With this change, WebXRSpace needs to handle the case of a null > WebXRSession, while previously, it was not possible. Can you describe why > this is good to do so? > > Are we getting closer to spec? It seems this particular change could > potentially be observable (before GC, WebXRSpace does something, after GC it > does something different). I made the change when I added https://immersive-web.github.io/webxr/#xrsession-viewer-reference-space in order to avoid a cycle between Session and the viewer reference space.

(In reply to Imanol Fernandez from comment #12) > (In reply to youenn fablet from comment #10) > > WebXRSpace was previously taking a Ref<WebXRSession> and this patch is > > changing it to a WeakPtr<WebXRSession>. > > With this change, WebXRSpace needs to handle the case of a null > > WebXRSession, while previously, it was not possible. Can you describe why > > this is good to do so? > > > > Are we getting closer to spec? It seems this particular change could > > potentially be observable (before GC, WebXRSpace does something, after GC it > > does something different). > > I made the change when I added > https://immersive-web.github.io/webxr/#xrsession-viewer-reference-space in > order to avoid a cycle between Session and the viewer reference space. From the spec I can see: - Each XRSpace has a session which is set to the XRSession that created the XRSpace. - Each XRSession has a viewer reference space, which is an XRReferenceSpace of type "viewer" with an identity transform origin offset. It seems that as long as XRSpace is alive, its XRSession should be alive. If XRSpace is never changing of XRSession, one possibility is to have: - XRSession is RefCounted<XRSession> - XRSpace is no longer a RefCounted<XRSpace> - XRSpace stores a XRSession& - XRSpace implements its ref/deref methods by calling ref/deref of its XRSession.

(In reply to youenn fablet from comment #13) > (In reply to Imanol Fernandez from comment #12) > > (In reply to youenn fablet from comment #10) > > > WebXRSpace was previously taking a Ref<WebXRSession> and this patch is > > > changing it to a WeakPtr<WebXRSession>. > > > With this change, WebXRSpace needs to handle the case of a null > > > WebXRSession, while previously, it was not possible. Can you describe why > > > this is good to do so? > > > > > > Are we getting closer to spec? It seems this particular change could > > > potentially be observable (before GC, WebXRSpace does something, after GC it > > > does something different). > > > > I made the change when I added > > https://immersive-web.github.io/webxr/#xrsession-viewer-reference-space in > > order to avoid a cycle between Session and the viewer reference space. > > From the spec I can see: > - Each XRSpace has a session which is set to the XRSession that created the > XRSpace. > - Each XRSession has a viewer reference space, which is an XRReferenceSpace > of type "viewer" with an identity transform origin offset. > > It seems that as long as XRSpace is alive, its XRSession should be alive. > If XRSpace is never changing of XRSession, one possibility is to have: > - XRSession is RefCounted<XRSession> > - XRSpace is no longer a RefCounted<XRSpace> > - XRSpace stores a XRSession& > - XRSpace implements its ref/deref methods by calling ref/deref of its > XRSession. That's a good option. I'm going to update the patch

Created attachment 420452 [details] Patch Use WebXRSession& in WebXRSpace

If you want to keep the design of using only WebXRSession ref, you will need to keep all spaces in WebXRSession (see how XMLHtppRequestUpload uses the ref/deref mechanism I mentioned before). Looking further the spec (I am not familiar with it), there is not a 1-1 relationship between session and space. - Spaces always need a reference to their session. I think having a Ref<WebXRSession> is good. - One space, the viewer space, is creating a circular reference as its session needs to ref it. It is not totally clear to me whether the viewer space is exposed somehow to scripts. One possibility would be to have the session keep a WeakPtr<> of the viewer space if it is easy to recreate it as needed. Another possibility is for the viewer space to be a different class, not refcounted at all, but fully owned by the WebXRSession as a unique_ptr.

(In reply to youenn fablet from comment #16) > If you want to keep the design of using only WebXRSession ref, you will need > to keep all spaces in WebXRSession (see how XMLHtppRequestUpload uses the > ref/deref mechanism I mentioned before). > > Looking further the spec (I am not familiar with it), there is not a 1-1 > relationship between session and space. > - Spaces always need a reference to their session. I think having a > Ref<WebXRSession> is good. > - One space, the viewer space, is creating a circular reference as its > session needs to ref it. > > It is not totally clear to me whether the viewer space is exposed somehow to > scripts. > > One possibility would be to have the session keep a WeakPtr<> of the viewer > space if it is easy to recreate it as needed. > Another possibility is for the viewer space to be a different class, not > refcounted at all, but fully owned by the WebXRSession as a unique_ptr. The viewer space owned by the WebXRSession is not exposed to script. It's only used in WebXRFrame::getViewerPose(). Another option would be to create a separate populatePose method in WebXRFrame for getViewerPose calls. That would avoid creating a new class at all or the ref/deref proxy solution but at the cost of some code reusability in WebXRFrame::populatePose(); Thinking more about the pros/cons of all the 4 solutions I like the one about creating a separate class for viewer space: - It's simpler to understand than the ref/deref proxy solution. - It doesn't require to recreate the viewer space if WeakPtr<> is not alive. XRFrame::getViewerPose is tipically called once per frame so I prefer to avoid allocations as much as possible - It allows us to reuse the WebXRFrame::populatePose() logic.

Created attachment 420521 [details] Patch Create a different class to hold WebXRSession's viewer-space

Comment on attachment 420521 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420521&action=review > Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:78 > + ASSERT(is<Document>(scriptExecutionContext())); I guess you could do something like: auto* document = downcast<Document>(scriptExecutionContext()); if (!document) ... > Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.h:40 > WTF_MAKE_ISO_ALLOCATED(WebXRBoundedReferenceSpace); Can WebXRBoundedReferenceSpace be set as final? > Source/WebCore/Modules/webxr/WebXRFrame.cpp:39 > + Unnecessary > Source/WebCore/Modules/webxr/WebXRFrame.cpp:142 > + if (limit) { We could probably compute limit closer to its actual use. > Source/WebCore/Modules/webxr/WebXRFrame.h:68 > + explicit WebXRFrame(Ref<WebXRSession>&&, IsAnimationFrame); explici not needed. > Source/WebCore/Modules/webxr/WebXRFrame.h:69 > + WebXRFrame(WebXRSession&, bool isAnimationFrame); To remove? > Source/WebCore/Modules/webxr/WebXRFrame.idl:37 > + [MayThrowException, CallWith=Document] WebXRPose? getPose(WebXRSpace space, WebXRSpace baseSpace); Should we use the Document here, or the document that created the WebXRSession? > Source/WebCore/Modules/webxr/WebXRReferenceSpace.cpp:102 > ASSERT(is<Document>(scriptExecutionContext())); ditto here, we can probably simplify a tiny bit. > Source/WebCore/Modules/webxr/WebXRReferenceSpace.cpp:126 > + not needed > Source/WebCore/Modules/webxr/WebXRReferenceSpace.h:53 > + WebXRReferenceSpace(Document&, Ref<WebXRSession>&&, Ref<WebXRRigidTransform>&&, XRReferenceSpaceType); Should be either protected or private, not public though probably since we have create static method > Source/WebCore/Modules/webxr/WebXRSpace.h:86 > + WebXRSession& m_session; A ref here is probably not safe since WebXRViewerSpace is refcounted, it could potentially outlive its WebXRSession. If we cannot make WebXRViewerSpace a unique_ptr, let's have a WeakPtr there. The alternative would be for WebXRSpace to be RefCounted. Or to introduce a WebXRSpaceBase which is not refcounted from which would derive WebXRViewerSpace and WebXRSpace.

Comment on attachment 420521 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420521&action=review >> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:78 >> + ASSERT(is<Document>(scriptExecutionContext())); > > I guess you could do something like: > auto* document = downcast<Document>(scriptExecutionContext()); > if (!document) > ... done >> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.h:40 >> WTF_MAKE_ISO_ALLOCATED(WebXRBoundedReferenceSpace); > > Can WebXRBoundedReferenceSpace be set as final? yes, done >> Source/WebCore/Modules/webxr/WebXRFrame.cpp:39 >> + > > Unnecessary done >> Source/WebCore/Modules/webxr/WebXRFrame.cpp:142 >> + if (limit) { > > We could probably compute limit closer to its actual use. done >> Source/WebCore/Modules/webxr/WebXRFrame.h:68 >> + explicit WebXRFrame(Ref<WebXRSession>&&, IsAnimationFrame); > > explici not needed. done >> Source/WebCore/Modules/webxr/WebXRFrame.h:69 >> + WebXRFrame(WebXRSession&, bool isAnimationFrame); > > To remove? yes, removed >> Source/WebCore/Modules/webxr/WebXRFrame.idl:37 >> + [MayThrowException, CallWith=Document] WebXRPose? getPose(WebXRSpace space, WebXRSpace baseSpace); > > Should we use the Document here, or the document that created the WebXRSession? We need both. I had to add it to implement a security check from the spec: https://immersive-web.github.io/webxr/#poses-may-be-reported "If session’s relevant global object is not the current global object, return false." >> Source/WebCore/Modules/webxr/WebXRReferenceSpace.cpp:102 >> ASSERT(is<Document>(scriptExecutionContext())); > > ditto here, we can probably simplify a tiny bit. done >> Source/WebCore/Modules/webxr/WebXRReferenceSpace.cpp:126 >> + > > not needed done >> Source/WebCore/Modules/webxr/WebXRReferenceSpace.h:53 >> + WebXRReferenceSpace(Document&, Ref<WebXRSession>&&, Ref<WebXRRigidTransform>&&, XRReferenceSpaceType); > > Should be either protected or private, not public though probably since we have create static method done >> Source/WebCore/Modules/webxr/WebXRSpace.h:86 >> + WebXRSession& m_session; > > A ref here is probably not safe since WebXRViewerSpace is refcounted, it could potentially outlive its WebXRSession. > If we cannot make WebXRViewerSpace a unique_ptr, let's have a WeakPtr there. > The alternative would be for WebXRSpace to be RefCounted. > Or to introduce a WebXRSpaceBase which is not refcounted from which would derive WebXRViewerSpace and WebXRSpace. Thanks for the review. I have changed WebXRSpace to be not RefCounted and make WebXRReferenceSpace RefCounted instead. Now WebXRViewerSpace is not RefCounted and uses a unique_ptr in WebXRSession.

Created attachment 420660 [details] Patch Address review feedback

Comment on attachment 420660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420660&action=review > Source/WebCore/Modules/webxr/WebXRFrame.cpp:134 > + const bool emulatedPosition = m_data.isPositionEmulated || !m_data.isPositionValid; why const bool? > Source/WebCore/Modules/webxr/WebXRSession.cpp:60 > + , m_viewerReferenceSpace(new WebXRViewerSpace(document, *this)) makeUnique<WebXRViewerSpace>(document, this) > Source/WebCore/Modules/webxr/WebXRSpace.h:58 > ScriptExecutionContext* scriptExecutionContext() const override { return ContextDestructionObserver::scriptExecutionContext(); } final? > Source/WebCore/Modules/webxr/WebXRSpace.h:60 > + Ref<WebXRRigidTransform> m_originOffset; Can we make it private and add a protected WebXRRigidTransform& getter? > Source/WebCore/Modules/webxr/WebXRSpace.h:81 > + void derefEventTarget() final { RELEASE_ASSERT_NOT_REACHED(); } This is fine like this. Otherwise, you could do m_session.ref()/m_session.deref();

Comment on attachment 420660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420660&action=review >> Source/WebCore/Modules/webxr/WebXRFrame.cpp:134 >> + const bool emulatedPosition = m_data.isPositionEmulated || !m_data.isPositionValid; > > why const bool? IMO it's a good practice to use const on local variables that don't change (e.g some discussion about this here: https://www.bfilipek.com/2016/12/please-declare-your-variables-as-const.html). I wish C++ had implicit const by default :) I removed the const to follow the same style as existing code. This could be a https://webkit.org/code-style-guidelines/ follow-up discussion >> Source/WebCore/Modules/webxr/WebXRSession.cpp:60 >> + , m_viewerReferenceSpace(new WebXRViewerSpace(document, *this)) > > makeUnique<WebXRViewerSpace>(document, this) done >> Source/WebCore/Modules/webxr/WebXRSpace.h:58 >> ScriptExecutionContext* scriptExecutionContext() const override { return ContextDestructionObserver::scriptExecutionContext(); } > > final? done >> Source/WebCore/Modules/webxr/WebXRSpace.h:60 >> + Ref<WebXRRigidTransform> m_originOffset; > > Can we make it private and add a protected WebXRRigidTransform& getter? yes, done >> Source/WebCore/Modules/webxr/WebXRSpace.h:81 >> + void derefEventTarget() final { RELEASE_ASSERT_NOT_REACHED(); } > > This is fine like this. Otherwise, you could do m_session.ref()/m_session.deref(); perfect, thanks

Created attachment 420818 [details] Patch for landing Patch for landing

> IMO it's a good practice to use const on local variables that don't change > (e.g some discussion about this here: > https://www.bfilipek.com/2016/12/please-declare-your-variables-as-const. > html). I wish C++ had implicit const by default :) > > I removed the const to follow the same style as existing code. This could be > a https://webkit.org/code-style-guidelines/ follow-up discussion Agreed this adds some nice properties but this makes reading the code is a bit more tedious. Starting a webkit thread might be indeed be a good idea.

Committed r273132: <https://commits.webkit.org/r273132> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420818 [details].

Comment on attachment 420818 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=420818&action=review > Source/WebCore/testing/WebFakeXRDevice.cpp:170 > + m_device.scheduleOnNextFrame([this]() { This likes a use-after-free bug waiting to happen. I think the XR code could use a good audit to make sure there aren't pointer safety or garbage collection issues.

(In reply to Alex Christensen from comment #27) > Comment on attachment 420818 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=420818&action=review > > > Source/WebCore/testing/WebFakeXRDevice.cpp:170 > > + m_device.scheduleOnNextFrame([this]() { > > This likes a use-after-free bug waiting to happen. I think the XR code > could use a good audit to make sure there aren't pointer safety or garbage > collection issues. True that it does not look great, though I think it is probably fine now since m_device is owned by WebFakeXRDevice and uses a timer to execute the pending updates. @Imanol, it would be nice to do a refactoring there to get this code more robust in the long run. It seems that scheduleOnNextFrame could be implemented by WebFakeXRDevice.

We can also probably get rid of the scheduleOnNextFrame calls by splitting the fake frame state into currentFrame and nextFrame, and just copy the next on the current on each fake frame. I'll try this apporach and submit some patch for review