Bug 220979

Created attachment 418419 [details] Patch

Created attachment 418423 [details] Add co-authors to the changelog messages

Comment on attachment 418423 [details] Add co-authors to the changelog messages View in context: https://bugs.webkit.org/attachment.cgi?id=418423&action=review > Source/WebCore/platform/xr/PlatformXR.h:81 > + }; To what space are the position and orientation relative to? Any thoughts on how to handle getting poses relative to different reference spaces later?

Comment on attachment 418423 [details] Add co-authors to the changelog messages View in context: https://bugs.webkit.org/attachment.cgi?id=418423&action=review > Source/WebCore/Modules/webxr/WebXRFrame.h:63 > + bool m_animationFrame; `m_animationFrame` is set but never used. This can trigger an unused private field warning.

Comment on attachment 418423 [details] Add co-authors to the changelog messages View in context: https://bugs.webkit.org/attachment.cgi?id=418423&action=review >> Source/WebCore/Modules/webxr/WebXRFrame.h:63 >> + bool m_animationFrame; > > `m_animationFrame` is set but never used. This can trigger an unused private field warning. It actually is, see below if (m_callbacks.size() == 1 && !m_animationFrame->isActive()) >> Source/WebCore/platform/xr/PlatformXR.h:81 >> + }; > > To what space are the position and orientation relative to? Any thoughts on how to handle getting poses relative to different reference spaces later? The user must request a reference space with requestRerefenceSpace(). https://immersive-web.github.io/webxr/#dom-xrsession-requestreferencespace

Comment on attachment 418423 [details] Add co-authors to the changelog messages View in context: https://bugs.webkit.org/attachment.cgi?id=418423&action=review >>> Source/WebCore/Modules/webxr/WebXRFrame.h:63 >>> + bool m_animationFrame; >> >> `m_animationFrame` is set but never used. This can trigger an unused private field warning. > > It actually is, see below > > if (m_callbacks.size() == 1 && !m_animationFrame->isActive()) Ah sorry, I replied without checking the context. You're right. BTW a bool should not be called like this, better use something like the parameter m_isAnimationFrame.

Comment on attachment 418423 [details] Add co-authors to the changelog messages View in context: https://bugs.webkit.org/attachment.cgi?id=418423&action=review >>> Source/WebCore/platform/xr/PlatformXR.h:81 >>> + }; >> >> To what space are the position and orientation relative to? Any thoughts on how to handle getting poses relative to different reference spaces later? > > The user must request a reference space with requestRerefenceSpace(). https://immersive-web.github.io/webxr/#dom-xrsession-requestreferencespace The poses from the devices are in local coordinates. Then they can be converted to other reference spaces. For local-floor we'll need to expose the floor transform. I'm going to push a new patch today with all the getViewerPose, Reference space, etc math and more tests passed. This patch is more focused on the animation loop. The FrameData is a placeholder, is going to be completed in the next patch

Created attachment 418924 [details] Patch

Rebased & fixed the m_IsAnimationFrame unused private field. If you want to check more context, I have pushed the almost complete follow-up patch that implements getPose, getViewerPose and the reference space transformations in https://bugs.webkit.org/show_bug.cgi?id=221225. The idea of this patch is to focus on the render loop for both immersive and inline sessions. Note that the diff of 221225 also includes this patch. I wish bugs.webkit.org supported dependant patches :)

<rdar://problem/73878240>

Comment on attachment 418924 [details] Patch I am not sure about lifetime safety here, especially since we are using raw pointers. It would be best to either use Ref or WeakPtr. Or have a very simple ownership model. View in context: https://bugs.webkit.org/attachment.cgi?id=418924&action=review > Source/WebCore/Modules/webxr/WebXRFrame.cpp:44 > +WebXRFrame::WebXRFrame(WebXRSession* session) Can we tighten it by passing a WebXRSession&? > Source/WebCore/Modules/webxr/WebXRFrame.cpp:45 > + : m_active(false) Might be better to set it in header file. > Source/WebCore/Modules/webxr/WebXRFrame.cpp:46 > + , m_session(session) WebXRFrame is refcounted so could potentially be kept alive longer than its session. Might be better to keep a ref or use a weakptr. > Source/WebCore/Modules/webxr/WebXRFrame.cpp:55 > + return *m_session; How can we guarantee this? > Source/WebCore/Modules/webxr/WebXRFrame.h:60 > + WebXRFrame(WebXRSession*); explicit > Source/WebCore/Modules/webxr/WebXRSession.cpp:59 > + , m_animationFrame(WebXRFrame::create(this)) Seems like we could use *this there. > Source/WebCore/Modules/webxr/WebXRSession.cpp:208 > + scriptExecutionContext()->postTask([this, promise = WTFMove(promise), type](auto& context) mutable { There is no guarantee this will be valid within the callback. > Source/WebCore/Modules/webxr/WebXRSession.cpp:221 > + queueTaskKeepingObjectAlive(*this, TaskSource::WebXR, [this, type, promise = WTFMove(promise), protectedDocument = makeRef(downcast<Document>(context))]() mutable { Why keeping a protected document here? I guess context is equal to this->scriptExecutionContext(). Can we just use that? > Source/WebCore/Modules/webxr/WebXRSession.cpp:463 > + m_device->requestFrame([this, protectedThis = makeRef(*this)](PlatformXR::Device::FrameData frameData) s/PlatformXR::Device::FrameDat/auto/ > Source/WebCore/Modules/webxr/WebXRSession.cpp:464 > + { Should be at the end of the next line. > Source/WebCore/Modules/webxr/WebXRSession.cpp:479 > + DOMHighResTimeStamp now = (MonotonicTime::now() - m_timeOrigin).milliseconds(); auto? > Source/WebCore/Modules/webxr/WebXRSession.cpp:483 > + m_animationFrame->setTime(static_cast<DOMHighResTimeStamp>(frameData.predictedDisplayTime)); Is there a way to not have to do the static_cast. > Source/WebCore/Modules/webxr/WebXRSession.cpp:512 > + callback->handleEvent(now, m_animationFrame.get()); Is there a chance the event will cancel everything (in which case we should stop everything), or will allow changing m_runningCallbacks synchronously? > Source/WebCore/Modules/webxr/WebXRSystem.cpp:475 > +class InlineRequestAnimationFrameCallback: public RequestAnimationFrameCallback { Should it be final? > Source/WebCore/Modules/webxr/WebXRSystem.cpp:482 > + InlineRequestAnimationFrameCallback(ScriptExecutionContext& scriptExecutionContext, Function<void()>&& callback) Please move this to private. Otherwise people can create it outside of create. > Source/WebCore/Modules/webxr/WebXRSystem.cpp:487 > + CallbackResult<void> handleEvent(double highResTimeMs) override final? Remove highResTimeMs > Source/WebCore/Modules/webxr/WebXRSystem.cpp:507 > + auto document = downcast<Document>(m_scriptExecutionContext); What is the guarantee m_scriptExecutionContext will be a Document? Should constructor take a Document? What is the guarantee for m_scriptExecutionContext to be valid here? > Source/WebCore/Modules/webxr/WebXRSystem.h:109 > + DummyInlineDevice(ScriptExecutionContext&); explicit > Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:205 > + HTMLCanvasElement* c = WTF::switchOn(m_context, [](const RefPtr<WebGLRenderingContextBase>& baseContext) { return WTF::switchOn(m_context, [](const RefPtr<WebGLRenderingContextBase>& baseContext) -> HTMLCanvasElement* { maybe? > Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:212 > + [](RefPtr<OffscreenCanvas>) { const RefPtr<OffscreenCanvas>&? > Source/WebCore/platform/xr/PlatformXR.h:96 > + using RequestFrameCallback = WTF::Function<void(FrameData)>; No need for WTF:: Probably also want to pass a FrameData&& > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:438 > + requestFrame([](FrameData) { }); s/FrameData/auto > Source/WebCore/testing/WebFakeXRDevice.cpp:46 > + m_supportsOrientationTracking = true; Why not: , m_supportsOrientationTracking(true) Or even better in the header declaration directly. > Source/WebCore/testing/WebFakeXRDevice.cpp:76 > + runningCallbacks.swap(m_callbacks); Could be written as: auto callbacks = WTFMove(m_callbacks)? > Source/WebCore/testing/WebFakeXRDevice.cpp:85 > + m_frameTimer.startOneShot(15_ms); Might be good to use a constant

Comment on attachment 418924 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418924&action=review Looking pretty good! Thanks Immanol and Sergio. > Source/WebCore/ChangeLog:9 > + - Properly implement the render loop for immersive and inline XR sessions > + - Apply pending render state changes It would be nice to have some more info in the ChangeLog. Maybe even per-file notes, for areas where there was a lot of change. >> Source/WebCore/Modules/webxr/WebXRFrame.cpp:44 >> +WebXRFrame::WebXRFrame(WebXRSession* session) > > Can we tighten it by passing a WebXRSession&? See question below. Why did this become a raw pointer rather than a Ref? >> Source/WebCore/Modules/webxr/WebXRFrame.cpp:46 >> + , m_session(session) > > WebXRFrame is refcounted so could potentially be kept alive longer than its session. > Might be better to keep a ref or use a weakptr. Since you can access the session from the XRFrame, I think this need to be a ref. The session itself might get disconnected, but the object still needs to exist. >> Source/WebCore/Modules/webxr/WebXRFrame.cpp:55 >> + return *m_session; > > How can we guarantee this? +1 If the session can never be null, then we shouldn't be storing a pointer. > Source/WebCore/Modules/webxr/WebXRFrame.h:62 > + bool m_active; add { false } > Source/WebCore/Modules/webxr/WebXRFrame.h:66 > - Ref<WebXRSession> m_session; > + // Session owns the frame. > + WebXRSession* m_session; Why is this now a pointer rather than a Ref? When can a Frame be created without a Session? >> Source/WebCore/Modules/webxr/WebXRSession.cpp:208 >> + scriptExecutionContext()->postTask([this, promise = WTFMove(promise), type](auto& context) mutable { > > There is no guarantee this will be valid within the callback. weakThis = makeWeakPtr(this) then check the value in the callback > Source/WebCore/Modules/webxr/WebXRSession.cpp:248 > + if (m_callbacks.size() == 1 && !m_animationFrame->isActive()) Why does the number of callbacks need to equal 1? Shouldn't this just be a non-zero test? >> Source/WebCore/Modules/webxr/WebXRSession.cpp:463 >> + m_device->requestFrame([this, protectedThis = makeRef(*this)](PlatformXR::Device::FrameData frameData) > > s/PlatformXR::Device::FrameDat/auto/ Oh cool. I didn't know you could do that. >> Source/WebCore/Modules/webxr/WebXRSession.cpp:512 >> + callback->handleEvent(now, m_animationFrame.get()); > > Is there a chance the event will cancel everything (in which case we should stop everything), or will allow changing m_runningCallbacks synchronously? The script here can do anything, but it looks like runningCallbacks is only used here, so it might be safe. But that makes me ask why it is exposed as a member variable rather than just a local variable in this block. > Source/WebCore/platform/xr/PlatformXR.h:87 > + float x, y, z, w; Separate line for each of these. > Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:435 > + // Ensure that requestFrame() is called at least one more time to properly end the session. > + callOnMainThread([this, weakThis = makeWeakPtr(this)]() { Why is this necessary? Is this just causing the pending callbacks to be flushed? Wouldn't it be better to delete them directly?

Comment on attachment 418924 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418924&action=review >> Source/WebCore/ChangeLog:9 >> + - Apply pending render state changes > > It would be nice to have some more info in the ChangeLog. Maybe even per-file notes, for areas where there was a lot of change. I improved the changelog in the new patch >>> Source/WebCore/Modules/webxr/WebXRFrame.cpp:46 >>> + , m_session(session) >> >> WebXRFrame is refcounted so could potentially be kept alive longer than its session. >> Might be better to keep a ref or use a weakptr. > > Since you can access the session from the XRFrame, I think this need to be a ref. The session itself might get disconnected, but the object still needs to exist. One of the problems was that session holds a Ref<XRFrame>, and XRFrame another Ref<XRSession> that could cause reference cycles. The WebXR spec states that session from the XRFrame is always valid so I have added a Ref<XRSession> there, because it needs to keep the session alive. In XRSession I have changed Ref<XRFrame> to RefPtr<XRFrame>. This RefPtr is cleaned when the session is ended. I added m_ended bailouts and asserts to make sure that we never access a empty m_animationFrame. >>> Source/WebCore/Modules/webxr/WebXRFrame.cpp:55 >>> + return *m_session; >> >> How can we guarantee this? > > +1 > > If the session can never be null, then we shouldn't be storing a pointer. Not needed anymore after switching to Ref<XRSession> >> Source/WebCore/Modules/webxr/WebXRFrame.h:60 >> + WebXRFrame(WebXRSession*); > > explicit done >> Source/WebCore/Modules/webxr/WebXRFrame.h:62 >> + bool m_active; > > add { false } done >> Source/WebCore/Modules/webxr/WebXRFrame.h:66 >> + WebXRSession* m_session; > > Why is this now a pointer rather than a Ref? When can a Frame be created without a Session? Fixes as mentioned in prev comments >> Source/WebCore/Modules/webxr/WebXRSession.cpp:59 >> + , m_animationFrame(WebXRFrame::create(this)) > > Seems like we could use *this there. switched to Ref<> >>> Source/WebCore/Modules/webxr/WebXRSession.cpp:208 >>> + scriptExecutionContext()->postTask([this, promise = WTFMove(promise), type](auto& context) mutable { >> >> There is no guarantee this will be valid within the callback. > > weakThis = makeWeakPtr(this) > then check the value in the callback fixed >> Source/WebCore/Modules/webxr/WebXRSession.cpp:221 >> + queueTaskKeepingObjectAlive(*this, TaskSource::WebXR, [this, type, promise = WTFMove(promise), protectedDocument = makeRef(downcast<Document>(context))]() mutable { > > Why keeping a protected document here? > I guess context is equal to this->scriptExecutionContext(). > Can we just use that? done >> Source/WebCore/Modules/webxr/WebXRSession.cpp:248 >> + if (m_callbacks.size() == 1 && !m_animationFrame->isActive()) > > Why does the number of callbacks need to equal 1? Shouldn't this just be a non-zero test? I added some comments to make it clearer. Script can add many RAFs but whe only need to request/schedule the frame once. As the callback is being added before to the vector the size for the check is 1. We could move m_callbacks.append() and use isEmpty() test but even if the frame runs on a next tick, it felt a bit weird to request a callback before all the data was ready (just thinking on the risk of a port calling the lambda on a sync way) >>> Source/WebCore/Modules/webxr/WebXRSession.cpp:463 >>> + m_device->requestFrame([this, protectedThis = makeRef(*this)](PlatformXR::Device::FrameData frameData) >> >> s/PlatformXR::Device::FrameDat/auto/ > > Oh cool. I didn't know you could do that. That's cool, nice C++14 feature :) >> Source/WebCore/Modules/webxr/WebXRSession.cpp:464 >> + { > > Should be at the end of the next line. fixed >> Source/WebCore/Modules/webxr/WebXRSession.cpp:479 >> + DOMHighResTimeStamp now = (MonotonicTime::now() - m_timeOrigin).milliseconds(); > > auto? done >> Source/WebCore/Modules/webxr/WebXRSession.cpp:483 >> + m_animationFrame->setTime(static_cast<DOMHighResTimeStamp>(frameData.predictedDisplayTime)); > > Is there a way to not have to do the static_cast. The problem is that most SDKs provide the data using a long timestamp but JS doesn't support 64 bit integers. I think is better to make sure we have the most accurate data in the ports and only potentially lose precision in JS. >>> Source/WebCore/Modules/webxr/WebXRSession.cpp:512 >>> + callback->handleEvent(now, m_animationFrame.get()); >> >> Is there a chance the event will cancel everything (in which case we should stop everything), or will allow changing m_runningCallbacks synchronously? > > The script here can do anything, but it looks like runningCallbacks is only used here, so it might be safe. But that makes me ask why it is exposed as a member variable rather than just a local variable in this block. runningCallbacks is also accessed from cancelRequestAnimationFrame in order to cancel ongoing rafs. Only the `cancel` flag is modified so multiple vector item modifications are not possible. >> Source/WebCore/Modules/webxr/WebXRSystem.cpp:475 >> +class InlineRequestAnimationFrameCallback: public RequestAnimationFrameCallback { > > Should it be final? done >> Source/WebCore/Modules/webxr/WebXRSystem.cpp:482 >> + InlineRequestAnimationFrameCallback(ScriptExecutionContext& scriptExecutionContext, Function<void()>&& callback) > > Please move this to private. > Otherwise people can create it outside of create. done >> Source/WebCore/Modules/webxr/WebXRSystem.cpp:487 >> + CallbackResult<void> handleEvent(double highResTimeMs) override > > final? > Remove highResTimeMs done >> Source/WebCore/Modules/webxr/WebXRSystem.cpp:507 >> + auto document = downcast<Document>(m_scriptExecutionContext); > > What is the guarantee m_scriptExecutionContext will be a Document? > Should constructor take a Document? > What is the guarantee for m_scriptExecutionContext to be valid here? It uses the same m_scriptExecutionContext from the WebXRSystem, which holds the DummyInlineDevice. The scriptExecutionContext is already downcasted to document in other places in (e.g. WebXRSystem::isSessionSupported) I have added a make the access to it safer. >> Source/WebCore/Modules/webxr/WebXRSystem.h:109 >> + DummyInlineDevice(ScriptExecutionContext&); > > explicit done >> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:205 >> + HTMLCanvasElement* c = WTF::switchOn(m_context, [](const RefPtr<WebGLRenderingContextBase>& baseContext) { > > return WTF::switchOn(m_context, [](const RefPtr<WebGLRenderingContextBase>& baseContext) -> HTMLCanvasElement* { > maybe? done >> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:212 >> + [](RefPtr<OffscreenCanvas>) { > > const RefPtr<OffscreenCanvas>&? done >> Source/WebCore/platform/xr/PlatformXR.h:87 >> + float x, y, z, w; > > Separate line for each of these. done >> Source/WebCore/platform/xr/PlatformXR.h:96 >> + using RequestFrameCallback = WTF::Function<void(FrameData)>; > > No need for WTF:: > Probably also want to pass a FrameData&& done >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:435 >> + callOnMainThread([this, weakThis = makeWeakPtr(this)]() { > > Why is this necessary? Is this just causing the pending callbacks to be flushed? Wouldn't it be better to delete them directly? OpenXR needs to wait for the XR_SESSION_STATE_STOPPING state to properly end the session. We were reusing the frame loop which already checked this event. I agree that is confusing. I have done some refactor to improve the event handling and added a separate function that handle the shutdown process (which also uses the sessionDidEnd() notification now) >> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:438 >> + requestFrame([](FrameData) { }); > > s/FrameData/auto not needed anymore >> Source/WebCore/testing/WebFakeXRDevice.cpp:46 >> + m_supportsOrientationTracking = true; > > Why not: > , m_supportsOrientationTracking(true) > Or even better in the header declaration directly. in this case is a protected member from base class >> Source/WebCore/testing/WebFakeXRDevice.cpp:76 >> + runningCallbacks.swap(m_callbacks); > > Could be written as: > auto callbacks = WTFMove(m_callbacks)? done >> Source/WebCore/testing/WebFakeXRDevice.cpp:85 >> + m_frameTimer.startOneShot(15_ms); > > Might be good to use a constant done

Created attachment 419157 [details] Patch New patch that addresses the review feedback

Created attachment 419267 [details] Patch fix nit: move all members of FrameData fov struct to separate lines

Created attachment 419579 [details] Patch Improve OpenXR event handling

Created attachment 419584 [details] Patch Rebase and fix changelog nit

Comment on attachment 419584 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419584&action=review > Source/WebCore/Modules/webxr/WebXRSession.cpp:80 > + m_animationFrame = WebXRFrame::create(makeRef(*this)); This creates a ref cycle. Is there a way to not create a ref cycle? Do we need to store the animation frame? As a ref? If not, how are we making sure to break it? It seems shutdown will break the ref cycle but I am not sure we will always go there. > Source/WebCore/Modules/webxr/WebXRSession.cpp:231 > + auto document = makeRef(downcast<Document>(*scriptExecutionContext())); There is no guarantee scriptExecutionContext() will not be null, please exit early if null. Also why refing the document? > Source/WebCore/Modules/webxr/WebXRSession.cpp:522 > + m_runningCallbacks.swap(m_callbacks); Do we need m_runningCallbacks? Can we just do auto runningCallbacks = WTFMove(m_callbacks); or std::exchange > Source/WebCore/Modules/webxr/WebXRSession.cpp:536 > + callback->handleEvent(now, *m_animationFrame); This is still a bit weak to have handleEvent here and m_runningCallbacks be accessed elsewhere. I wonder how we could make this more robust. > Source/WebCore/Modules/webxr/WebXRSession.h:94 > + explicit WebXRSession(Document&, WebXRSystem&, XRSessionMode, PlatformXR::Device&); No need for explicit. > Source/WebCore/Modules/webxr/WebXRSystem.h:109 > + DummyInlineDevice(ScriptExecutionContext&); explicit > Source/WebCore/Modules/webxr/WebXRSystem.h:118 > + ScriptExecutionContext* m_scriptExecutionContext; nullptr here just in case. > Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:214 > + return c; Can you try to do just: return switchOn(...)

Comment on attachment 419584 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419584&action=review >> Source/WebCore/Modules/webxr/WebXRSession.cpp:80 >> + m_animationFrame = WebXRFrame::create(makeRef(*this)); > > This creates a ref cycle. Is there a way to not create a ref cycle? > Do we need to store the animation frame? As a ref? > If not, how are we making sure to break it? It seems shutdown will break the ref cycle but I am not sure we will always go there. One thing we could do is create a new XRFrame every time, that way we don't need to hold any references here. This could work well as long as GC/JavaScriptCore correctly handles short lived objects. There were some problems in the spec on some browsers due to creating many new objects in RAFs, being XRFrame one of them. Check this for more context: https://github.com/immersive-web/webxr/issues/1010. The working group decided to allow reusing XRFrame but it's not mandatory. >> Source/WebCore/Modules/webxr/WebXRSession.cpp:536 >> + callback->handleEvent(now, *m_animationFrame); > > This is still a bit weak to have handleEvent here and m_runningCallbacks be accessed elsewhere. > I wonder how we could make this more robust. The problem is that XRSession::cancelAnimationFrame needs to mark a pending running callback as cancelled based on the JS code from that RAF specific callback. I could create a separate set with the pending callbackIds to cancel instead of using m_runningCallbacks vector. I don't see a way to do it without a extra data structure that is accessed from both cancelAnimationFrame and this loop. Any more ideas?

Comment on attachment 419584 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419584&action=review >>> Source/WebCore/Modules/webxr/WebXRSession.cpp:80 >>> + m_animationFrame = WebXRFrame::create(makeRef(*this)); >> >> This creates a ref cycle. Is there a way to not create a ref cycle? >> Do we need to store the animation frame? As a ref? >> If not, how are we making sure to break it? It seems shutdown will break the ref cycle but I am not sure we will always go there. > > One thing we could do is create a new XRFrame every time, that way we don't need to hold any references here. This could work well as long as GC/JavaScriptCore correctly handles short lived objects. There were some problems in the spec on some browsers due to creating many new objects in RAFs, being XRFrame one of them. Check this for more context: https://github.com/immersive-web/webxr/issues/1010. The working group decided to allow reusing XRFrame but it's not mandatory. I have updated the patch to create a new XRFrame every time. We don't need to hold it this way. If we find GC issues in the future we can address them in follow-up PRs. Anyway, XRFrame is only created once per frame, while other objects (e.g. XRRigidTransform) can be created many more times, so the impact of XRFrame is really small compared to others. >> Source/WebCore/Modules/webxr/WebXRSession.cpp:231 >> + auto document = makeRef(downcast<Document>(*scriptExecutionContext())); > > There is no guarantee scriptExecutionContext() will not be null, please exit early if null. > Also why refing the document? fixed >> Source/WebCore/Modules/webxr/WebXRSession.cpp:522 >> + m_runningCallbacks.swap(m_callbacks); > > Do we need m_runningCallbacks? Can we just do > auto runningCallbacks = WTFMove(m_callbacks); or std::exchange Updated to follow the same pattern as DOMWindow::requestAnimationFrame and DOMWindow::cancelAnimationFrame >>> Source/WebCore/Modules/webxr/WebXRSession.cpp:536 >>> + callback->handleEvent(now, *m_animationFrame); >> >> This is still a bit weak to have handleEvent here and m_runningCallbacks be accessed elsewhere. >> I wonder how we could make this more robust. > > The problem is that XRSession::cancelAnimationFrame needs to mark a pending running callback as cancelled based on the JS code from that RAF specific callback. > > I could create a separate set with the pending callbackIds to cancel instead of using m_runningCallbacks vector. I don't see a way to do it without a extra data structure that is accessed from both cancelAnimationFrame and this loop. Any more ideas? In the latest patch I have followed the same pattern as the implementation in DOMWindow::requestAnimationFrame and DOMWindow::cancelAnimationFrame. We only have one m_callbacks vector now, and it's copied before iteration. >> Source/WebCore/Modules/webxr/WebXRSession.h:94 >> + explicit WebXRSession(Document&, WebXRSystem&, XRSessionMode, PlatformXR::Device&); > > No need for explicit. done >> Source/WebCore/Modules/webxr/WebXRSystem.h:109 >> + DummyInlineDevice(ScriptExecutionContext&); > > explicit done >> Source/WebCore/Modules/webxr/WebXRSystem.h:118 >> + ScriptExecutionContext* m_scriptExecutionContext; > > nullptr here just in case. done >> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:214 >> + return c; > > Can you try to do just: return switchOn(...) done

Created attachment 419699 [details] Patch

Created attachment 419709 [details] Patch Rebase onto main

Created attachment 419713 [details] Patch Add missing setFiredOrCancelled() call

Comment on attachment 419713 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419713&action=review > Source/WebCore/Modules/webxr/WebXRSession.cpp:505 > + auto callbacks = m_callbacks; Can we do: auto callbacks = WTFMove(m_callbacks). > Source/WebCore/Modules/webxr/WebXRSession.cpp:508 > + frame->setActive(true); We went from a model where we were reusing the same frame over and over to creating a new frame each time. This is observable from JS so we should add a test for it. And verify other browsers are doing the same. > Source/WebCore/Modules/webxr/WebXRSession.cpp:527 > + }); Not needed if we can clear m_callbacks above. > Source/WebCore/Modules/webxr/WebXRSystem.h:118 > + ScriptExecutionContext* m_scriptExecutionContext { nullptr }; This is probably incorrect. Let's make it a ContextDesutrctionObserver. > Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:205 > + return WTF::switchOn(m_context, [](const RefPtr<WebGLRenderingContextBase>& baseContext) { What about #if ENABLE(WEBGL2)?

Comment on attachment 419713 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419713&action=review >> Source/WebCore/Modules/webxr/WebXRSession.cpp:505 >> + auto callbacks = m_callbacks; > > Can we do: > auto callbacks = WTFMove(m_callbacks). We can't because m_callbacks is used in cancelAnimationFrame() in order to set up the fired or cancelled flag. So we need a copy here. >> Source/WebCore/Modules/webxr/WebXRSession.cpp:508 >> + frame->setActive(true); > > We went from a model where we were reusing the same frame over and over to creating a new frame each time. > This is observable from JS so we should add a test for it. > And verify other browsers are doing the same. Chromium is also creating a new XRFrame each time. Gecko is using a pool of XRFrames. I'll create a follow-up issue to track the GC impact of all the potentially short living objects. I know that V8 handles it well because of their three generation GC. SpiderMonkey didn't handle it as well, so it had to rely on object pooling in general. We can do some profiles in WebKit, but based on my initial tests running some WebXR samples locally it does a good job. I tested it on desktop though, I'll test it on mobile OpenXR when possible. >> Source/WebCore/Modules/webxr/WebXRSession.cpp:527 >> + }); > > Not needed if we can clear m_callbacks above. We can't clear m_callbacks above >> Source/WebCore/Modules/webxr/WebXRSystem.h:118 >> + ScriptExecutionContext* m_scriptExecutionContext { nullptr }; > > This is probably incorrect. Let's make it a ContextDesutrctionObserver. done >> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:205 >> + return WTF::switchOn(m_context, [](const RefPtr<WebGLRenderingContextBase>& baseContext) { > > What about #if ENABLE(WEBGL2)? What's the problem with WebGL2? we are covering both WebGL1 and WebGL2 with the WebGLRenderingContextBase base class

Created attachment 419993 [details] Patch for landing Add ContextDestructionObserver

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