Created attachment 406833 [details]
Patch

Committed r265850: <https://trac.webkit.org/changeset/265850>

<rdar://problem/67399148>

Reverted r265850 for reason:

Tests crash on debug builds

Committed r265959: <https://trac.webkit.org/changeset/265959>

Comment on attachment 406833 [details]
Patch

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

> Source/WebCore/Modules/webxr/WebXRSession.cpp:198
>      // 2. Run the following steps in parallel:

When a spec says "in parallel", that does actually mean they have to be run on separate threads, it's really just an event loop related hint that the steps don't need to be run immediately in a specific order (https://html.spec.whatwg.org/#in-parallel).

In WebCore, most of the DOM is not intended to be used from a non-main thread.

Sam said:

> that does actually mean

He meant:

> that does *not* actually mean

(In reply to Darin Adler from comment #6)
> Sam said:
> 
> > that does actually mean
> 
> He meant:
> 
> > that does *not* actually mean

Oh gosh, yeah. That is a pretty major typo. Indeed!

(In reply to Sam Weinig from comment #5)
> Comment on attachment 406833 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406833&action=review
> 
> > Source/WebCore/Modules/webxr/WebXRSession.cpp:198
> >      // 2. Run the following steps in parallel:
> 
> When a spec says "in parallel", that does actually mean they have to be run
> on separate threads, it's really just an event loop related hint that the
> steps don't need to be run immediately in a specific order
> (https://html.spec.whatwg.org/#in-parallel).

Sure, I didn't imply that "in parallel" only means in a different thread/process, but it's definitely a possibility, isn't it? The standard explicitly mentions "This standard does not define the precise mechanism by which this is achieved, be it time-sharing cooperative multitasking, fibers, threads, processes, using different hyperthreads, cores, CPUs, machines, etc"

> 
> In WebCore, most of the DOM is not intended to be used from a non-main
> thread.

Right but in this case I thought it really did because we're talking about accessing devices in a synchronous (blocking) way. We don't want to stop the main thread because of that.

See for example this algorithm: https://immersive-web.github.io/webxr/#dom-xrsystem-requestsession

Step 5. asks implementors to run the following steps in parallel. In that case I considered that it really meant off the main thread for 2 reasons:
a) Step 5.3 is about retrieving the current immersive device which might force us to enumerate the list of immersive devices which is a synchronous operation in which we don't have to block the main thread
b) Step 5.4 does not make much sense IMHO if we're not in a different thread, why would we want to queue another task? My interpretation was that we want to do it to run the next steps in the main thread after retrieving the current device in another thread.

Granted we could think about a different way to solve it which is to implement https://immersive-web.github.io/webxr/#obtain-the-current-device and in consequence https://immersive-web.github.io/webxr/#ensure-an-immersive-xr-device-is-selected as async methods with a callback, and then perform the next steps in those callbacks. Would that make more sense? I guess that would still match the "in parallel" interpretation you made.

Note that we'd still have to implement the platform code to be run asynchronously either using a different explicit thread, a work queue or the like, wouldn't we?

What typically happens is that you hop to a background thread when doing I/O stuff like reading/writing files.
Usually this should be hidden by APIs that would be asynchronous and take a callback.
In WebCore DOM code, you would just call this API passing a callback capturing the promise instead of calling a synchronous API and resolving the promise with the result of the synchronous API.

A typical case where you would like to run things in parallel is if you have to enumerate WebXR devices. This might be done once for the whole process or by IPCing to UIProcess. In any case, getting the list of devices would be done through a callback based API.

Created attachment 407891 [details]
Patch

Comment on attachment 407891 [details]
Patch

Not asking for review yet

Closing this.

See https://bugs.webkit.org/show_bug.cgi?id=217172