Follow-up from conversations in https://bugs.webkit.org/show_bug.cgi?id=221225 There are some concerns of potential use-after-free in WebFakeXRDevice. We can make the code more robust by getting rid of the scheduleOnNextFrame calls and use a frameData struct instead.
Created attachment 423896 [details] Patch
<rdar://problem/75956780>
Comment on attachment 423896 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423896&action=review > Source/WebCore/testing/WebFakeXRDevice.cpp:63 > +void SimulatedXRDevice::setViews(Vector<Ref<FakeXRView>>&& views) Does it need to be a &&? > Source/WebCore/testing/WebFakeXRDevice.cpp:84 > +void SimulatedXRDevice::setViewerOrigin(Optional<FrameData::Pose>&& origin) const Optional<>& > Source/WebCore/testing/WebFakeXRDevice.cpp:89 > + m_frameData.isTrackingValid = true; return here. > Source/WebCore/testing/WebFakeXRDevice.cpp:138 > + FrameData data = m_frameData; Should we do FrameData data = WTFMove(m_frameData)? > Source/WebCore/testing/WebFakeXRDevice.cpp:202 > + deviceViews.append(view.releaseReturnValue()); I would tend to remove the other setViews and create the FrameData::View object here. > Source/WebCore/testing/WebXRTest.cpp:57 > + simulatedDevice.setViews(WTFMove(views)); Cannot we directly call setViews(const Vector<FakeXRViewInit>&)
Comment on attachment 423896 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423896&action=review >> Source/WebCore/testing/WebFakeXRDevice.cpp:63 >> +void SimulatedXRDevice::setViews(Vector<Ref<FakeXRView>>&& views) > > Does it need to be a &&? I changed it to Vector<FrameData::View>&& >> Source/WebCore/testing/WebFakeXRDevice.cpp:84 >> +void SimulatedXRDevice::setViewerOrigin(Optional<FrameData::Pose>&& origin) > > const Optional<>& done >> Source/WebCore/testing/WebFakeXRDevice.cpp:89 >> + m_frameData.isTrackingValid = true; > > return here. done >> Source/WebCore/testing/WebFakeXRDevice.cpp:138 >> + FrameData data = m_frameData; > > Should we do FrameData data = WTFMove(m_frameData)? No, because we want m_fameData to keep the previous values. >> Source/WebCore/testing/WebFakeXRDevice.cpp:202 >> + deviceViews.append(view.releaseReturnValue()); > > I would tend to remove the other setViews and create the FrameData::View object here. Done >> Source/WebCore/testing/WebXRTest.cpp:57 >> + simulatedDevice.setViews(WTFMove(views)); > > Cannot we directly call setViews(const Vector<FakeXRViewInit>&) yes, done.
Created attachment 425381 [details] Patch for landing
Committed r275603: <https://commits.webkit.org/r275603> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425381 [details].