Bug 217172 – [WebXR] Make enumerateImmersiveXRDevices() asynchronnous

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

[Sergio Villar Senin]

[WebXR] Make enumerateImmersiveXRDevices() asynchronnous

[Sergio Villar Senin]

Created attachment 410216 [details]
Patch

[Sergio Villar Senin]

Gentle ping for reviewers.

[youenn fablet]

Comment on attachment 410216 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=410216&action=review

> Source/WebCore/Modules/webxr/WebXRSystem.cpp:88
> +    platformXR.enumerateImmersiveXRDevices([this, isFirstXRDevicesEnumeration, callback = WTFMove(callback)](const Vector<std::unique_ptr<PlatformXR::Device>>& immersiveXRDevices) mutable {

There is no guarantee this is valid. We might want to protect it.
s/const Vector<std::unique_ptr<PlatformXR::Device>>/auto&/

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:3934
> +    xrSystem.ensureImmersiveXRDeviceIsSelected([this, promise = WTFMove(promise), &xrSystem]() mutable {

There is no guarantee that 'this' stay alive in the callback.
For xrSystem, it is guaranteed by ensureImmersiveXRDeviceIsSelected internals, so it might be best to not catch xrSystem by reference and either recompute it inside the callback, or ref it.

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:237
> +    callback(m_immersiveXRDevices);

It does not seem callback will be called in case of RETURN_IF_FAILED(result, "xrGetSystem", m_impl->xrInstance());.

Also it does not seem callback is called asynchronously here so it does not seem necessary.
I would guess you might want to run that task in a WorkQueue if this is blocking main thread?

[Sergio Villar Senin]

Created attachment 410641 [details]
Patch

[Sergio Villar Senin]

(In reply to youenn fablet from comment #3)
> Also it does not seem callback is called asynchronously here so it does not
> seem necessary.
> I would guess you might want to run that task in a WorkQueue if this is
> blocking main thread?

Right, this is why I mention in the ChangeLog that I'll post a follow up patch which will move all the OpenXR calls out of the main thread because they're blocking.

[youenn fablet]

Comment on attachment 410641 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=410641&action=review

> Source/WebCore/Modules/webxr/WebXRSystem.cpp:378
> +    obtainCurrentDevice(mode, init.requiredFeatures, init.optionalFeatures, [this, protectedDocument = makeRef(document), immersive, init, mode, promise = WTFMove(promise)](PlatformXR::Device* device) mutable {

COulbe auto*

> Source/WebCore/Modules/webxr/WebXRSystem.cpp:380
> +            promise.reject(Exception { NotSupportedError });

Might be fine for now. But NotSupportedError might not be good in all early return cases.

> Source/WebCore/Modules/webxr/WebXRSystem.cpp:421
> +        promise.resolve(session);

Could WTFMove session

[Radar WebKit Bug Importer]

<rdar://problem/70087114>

[Sergio Villar Senin]

Committed r268255: <https://trac.webkit.org/changeset/268255>

[Kenneth Russell]

Note there's now a "WebXR" component as of Bug 217195. This bug's been recategorized into it; please feel free to do the same for other WebXR bugs.