Bug 223580 – Use frameData instead of scheduleOnNextFrame calls in WebFakeXRDevice

• Status: RESOLVED
• Resolution: FIXED
• Component: WebXR

[Imanol Fernandez]

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.

[Imanol Fernandez]

Created attachment 423896 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/75956780>

[youenn fablet]

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>&)

[Imanol Fernandez]

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.

[Imanol Fernandez]

Created attachment 425381 [details]
Patch for landing

[EWS]

Committed r275603: <https://commits.webkit.org/r275603>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425381 [details].