Bug 217172 - [WebXR] Make enumerateImmersiveXRDevices() asynchronnous
Summary: [WebXR] Make enumerateImmersiveXRDevices() asynchronnous
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebXR (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords: InRadar
Depends on:
Blocks: 217195
  Show dependency treegraph
 
Reported: 2020-10-01 03:36 PDT by Sergio Villar Senin
Modified: 2020-10-29 17:59 PDT (History)
14 users (show)

See Also:


Attachments
Patch (24.14 KB, patch)
2020-10-01 03:49 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (24.46 KB, patch)
2020-10-06 08:48 PDT, Sergio Villar Senin
youennf: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2020-10-01 03:36:18 PDT
[WebXR] Make enumerateImmersiveXRDevices() asynchronnous
Comment 1 Sergio Villar Senin 2020-10-01 03:49:40 PDT
Created attachment 410216 [details]
Patch
Comment 2 Sergio Villar Senin 2020-10-05 12:27:58 PDT
Gentle ping for reviewers.
Comment 3 youenn fablet 2020-10-06 01:07:53 PDT
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?
Comment 4 Sergio Villar Senin 2020-10-06 08:48:28 PDT
Created attachment 410641 [details]
Patch
Comment 5 Sergio Villar Senin 2020-10-06 08:50:29 PDT
(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.
Comment 6 youenn fablet 2020-10-08 01:21:56 PDT
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
Comment 7 Radar WebKit Bug Importer 2020-10-08 03:37:17 PDT
<rdar://problem/70087114>
Comment 8 Sergio Villar Senin 2020-10-09 04:49:18 PDT
Committed r268255: <https://trac.webkit.org/changeset/268255>
Comment 9 Kenneth Russell 2020-10-29 17:59:26 PDT
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.