Bug 225025 – [WebXR] Remove reference cycle in WebXRSession

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dean Jackson]

[WebXR] Use weak pointers to reference WebXRSessions

[Radar WebKit Bug Importer]

<rdar://problem/77111217>

[Dean Jackson]

Created attachment 426993 [details]
Patch

[Dean Jackson]

I'm surprised this patch builds on gtk/wpe!

[Sam Weinig]

Comment on attachment 426993 [details]
Patch

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

This all makes me think the bindings are likely to not have marking being done correctly for these classes either, so we will need to make sure to work on that.

> Source/WebCore/ChangeLog:10
> +        WebXRSession was attempting to create a WebXRInputSourceArray in
> +        its constructor initialization that held a strong reference to
> +        itself, which caused a crash. This pattern was used elsewhere too.

...and was a memory leak.

> Source/WebCore/Modules/webxr/WebXRFrame.h:52
> +    static Ref<WebXRFrame> create(WeakPtr<WebXRSession>, IsAnimationFrame);

All the places you are passing a WeakPtr<WebXRSession> should probably be WeakPtr<WebXRSession>&&.

> Source/WebCore/Modules/webxr/WebXRInputSourceArray.cpp:72
> +    if (!m_session)
> +        return;

Perhaps to be safe that nothing causes m_session to become null in the function (and some others), it might make sense to do the makeRefPtr upfront?

auto session = makeRefPtr(m_session)
if (!session)
    return;

?

> Source/WebCore/Modules/webxr/WebXRSession.h:56
> +class WebXRSession final : public RefCounted<WebXRSession>, public CanMakeWeakPtr<WebXRSession>, public EventTargetWithInlineData, public ActiveDOMObject, public PlatformXR::TrackingAndRenderingClient {

This gives WebXRSession two CanMakeWeakPtr<*> parents:
- This new one CanMakeWeakPtr<WebXRSession>
- And EventTargetWithInlineData -> EventTarget -> CanMakeWeakPtr<EventTarget>

I honestly have not worked enough with the weak pointer code in WebCore to know if this makes sense so someone else should review this.

[Chris Dumez]

Comment on attachment 426993 [details]
Patch

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

> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:57
> +WebXRBoundedReferenceSpace::WebXRBoundedReferenceSpace(Document& document, WeakPtr<WebXRSession> session, Ref<WebXRRigidTransform>&& offset, XRReferenceSpaceType type)

I believe we don’t usually pass a WeakPtr as parameter. Instead we pass a reference or pointer as usual and we only call makeWeakPtr when we store it as a data member.

>> Source/WebCore/Modules/webxr/WebXRSession.h:56
>> +class WebXRSession final : public RefCounted<WebXRSession>, public CanMakeWeakPtr<WebXRSession>, public EventTargetWithInlineData, public ActiveDOMObject, public PlatformXR::TrackingAndRenderingClient {
> 
> This gives WebXRSession two CanMakeWeakPtr<*> parents:
> - This new one CanMakeWeakPtr<WebXRSession>
> - And EventTargetWithInlineData -> EventTarget -> CanMakeWeakPtr<EventTarget>
> 
> I honestly have not worked enough with the weak pointer code in WebCore to know if this makes sense so someone else should review this.

Good catch. If we already subclass CanMakeWeakPtr from EventTarget then we should not subclass it again here. This  is wasteful.

[Chris Dumez]

Comment on attachment 426993 [details]
Patch

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

> Source/WebCore/Modules/webxr/WebXRSpace.h:86
> +    WebXRSession* m_session;

A raw pointer data member should be avoided at all costs. Nowadays we almost always use WeakPtr for these.

[Chris Dumez]

Comment on attachment 426993 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        its constructor initialization that held a strong reference to

Note that this was also fixable in one line I believe by calling relaxAdoptionRequirements().

>> Source/WebCore/ChangeLog:10
>> +        itself, which caused a crash. This pattern was used elsewhere too.
> 
> ...and was a memory leak.

But indeed, if we has a cycle that didn’t get broken, a WeakPtr to break the cycle is suitable. I don’t know enough about this WebAPI to know if it is OK that all these objects no longer keep alive the session alive though. I know for example that in WebAudio, AudioNodes have to keep their AudioContext alive for the API to keep functioning after a GC.

[Dean Jackson]

(In reply to Chris Dumez from comment #7)
> Comment on attachment 426993 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426993&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        its constructor initialization that held a strong reference to
> 
> Note that this was also fixable in one line I believe by calling
> relaxAdoptionRequirements().

Thanks :)

In that case I might land that change first before any of this.

[Sam Weinig]

(In reply to Chris Dumez from comment #7)
> Comment on attachment 426993 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426993&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        its constructor initialization that held a strong reference to
> 
> Note that this was also fixable in one line I believe by calling
> relaxAdoptionRequirements().
> 
> >> Source/WebCore/ChangeLog:10
> >> +        itself, which caused a crash. This pattern was used elsewhere too.
> > 
> > ...and was a memory leak.
> 
> But indeed, if we has a cycle that didn’t get broken, a WeakPtr to break the
> cycle is suitable. I don’t know enough about this WebAPI to know if it is OK
> that all these objects no longer keep alive the session alive though. I know
> for example that in WebAudio, AudioNodes have to keep their AudioContext
> alive for the API to keep functioning after a GC.

How do you resolve that in the WebAudio API? With custom marking?

[Sam Weinig]

(In reply to Chris Dumez from comment #5)
> Comment on attachment 426993 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426993&action=review
> 
> > Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:57
> > +WebXRBoundedReferenceSpace::WebXRBoundedReferenceSpace(Document& document, WeakPtr<WebXRSession> session, Ref<WebXRRigidTransform>&& offset, XRReferenceSpaceType type)
> 
> I believe we don’t usually pass a WeakPtr as parameter. Instead we pass a
> reference or pointer as usual and we only call makeWeakPtr when we store it
> as a data member.
> 

What is the goal of this idiom?  WeakPtr<>&& seems like it is quite clear about what is going on.

[Sam Weinig]

(In reply to Dean Jackson from comment #8)
> (In reply to Chris Dumez from comment #7)
> > Comment on attachment 426993 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=426993&action=review
> > 
> > > Source/WebCore/ChangeLog:9
> > > +        its constructor initialization that held a strong reference to
> > 
> > Note that this was also fixable in one line I believe by calling
> > relaxAdoptionRequirements().
> 
> Thanks :)
> 
> In that case I might land that change first before any of this.

Please don't. It seems clearly wrong to create a ref cycle here.

[Chris Dumez]

Comment on attachment 426993 [details]
Patch

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

>>>>> Source/WebCore/ChangeLog:9
>>>>> +        its constructor initialization that held a strong reference to
>>>> 
>>>> Note that this was also fixable in one line I believe by calling relaxAdoptionRequirements().
>>> 
>>> Thanks :)
>>> 
>>> In that case I might land that change first before any of this.
>> 
>> How do you resolve that in the WebAudio API? With custom marking?
> 
> Please don't. It seems clearly wrong to create a ref cycle here.

Right, I just looked at the code and there is a clear Ref cycle here With the WebXRSession ref'ing itself via m_inputSources. The minimal fix would be to use a WeakPtr for WebXRInputSourceArray::m_session.

[Chris Dumez]

Comment on attachment 426993 [details]
Patch

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

>>>>>> Source/WebCore/ChangeLog:9
>>>>>> +        its constructor initialization that held a strong reference to
>>>>> 
>>>>> Note that this was also fixable in one line I believe by calling relaxAdoptionRequirements().
>>>> 
>>>> Thanks :)
>>>> 
>>>> In that case I might land that change first before any of this.
>>> 
>>> How do you resolve that in the WebAudio API? With custom marking?
>> 
>> Please don't. It seems clearly wrong to create a ref cycle here.
> 
> Right, I just looked at the code and there is a clear Ref cycle here With the WebXRSession ref'ing itself via m_inputSources. The minimal fix would be to use a WeakPtr for WebXRInputSourceArray::m_session.

... and WebXRInputSource::m_session.

[Dean Jackson]

(In reply to Dean Jackson from comment #8)
> (In reply to Chris Dumez from comment #7)
> > Comment on attachment 426993 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=426993&action=review
> > 
> > > Source/WebCore/ChangeLog:9
> > > +        its constructor initialization that held a strong reference to
> > 
> > Note that this was also fixable in one line I believe by calling
> > relaxAdoptionRequirements().
> 
> Thanks :)
> 
> In that case I might land that change first before any of this.

Not quite a one line fix. The problem is that WebXRSession has a Ref<WebXRInputSourceArray> and WebXRInputSourceArray keeps a Ref<WebXRSession>.

Even if that were ok in one direction, I can't call relaxAdoptionRequirement() until the WebXRSession constructor, but the Ref needs to be initialised first.

[Dean Jackson]

Yep. That's what I'm doing. (I should have reloaded before commenting)

[Ryosuke Niwa]

Comment on attachment 426993 [details]
Patch

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

>>> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:57
>>> +WebXRBoundedReferenceSpace::WebXRBoundedReferenceSpace(Document& document, WeakPtr<WebXRSession> session, Ref<WebXRRigidTransform>&& offset, XRReferenceSpaceType type)
>> 
>> I believe we don’t usually pass a WeakPtr as parameter. Instead we pass a reference or pointer as usual and we only call makeWeakPtr when we store it as a data member.
> 
> What is the goal of this idiom?  WeakPtr<>&& seems like it is quite clear about what is going on.

Well, in the case, the caller uses l has WeakPtr, that might make sense.
In this case, "this" pointer is the one for which we're creating WeakPtr
so it's probably more appropriate to pass in WebXRSession&.
That would make it clear that the object isn't nullptr at the point of the creation.

> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:90
>  // https://immersive-web.github.io/webxr/#dom-xrboundedreferencespace-boundsgeometry

Below this code, WebXRBoundedReferenceSpace::updateIfNeeded() indiscriminately dereferences m_session.
How does that work?
It seems to me that we need to keep WebXRSession alive whenever we have one of those *Space objects around.
See my comment for WebXRFrame for potential solutions.

> Source/WebCore/Modules/webxr/WebXRFrame.h:55
> -    const WebXRSession& session() const { return m_session.get(); }
> +    const WebXRSession* session() const { return m_session.get(); }

Since this is exposed to JS, we need to make sure we keep WebXRSession alive when JS wrapper for this object is alive.
There are a few ways to do this. I think the easiest two options are:
1. We can make WebXRSession::hasPendingActivity return true whenever there is a WebXRFrame
2. Make WebXRFrame add its WebXRSession as an opaque root, and make WebXRSession's opaque root itself.

> Source/WebCore/Modules/webxr/WebXRInputSource.cpp:73
>          else if (auto* document = downcast<Document>(m_session->scriptExecutionContext()))

What guarantees that m_session is still alive?

[Imanol Fernandez]

Comment on attachment 426993 [details]
Patch

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

>>> Source/WebCore/ChangeLog:10
>>> +        itself, which caused a crash. This pattern was used elsewhere too.
>> 
>> ...and was a memory leak.
> 
> But indeed, if we has a cycle that didn’t get broken, a WeakPtr to break the cycle is suitable. I don’t know enough about this WebAPI to know if it is OK that all these objects no longer keep alive the session alive though. I know for example that in WebAudio, AudioNodes have to keep their AudioContext alive for the API to keep functioning after a GC.

The idea with the current code was the Ref<WebXRSession> & Ref<WebXRInputsourceArray> cycle is broken in WebXRSession::shutdown or end, which should always be called by the WebXRSystem if users don't call it manually. The spec doesn't mention if the session must be kept alive if users hold a WebXRInputsourceArray, but that's seems to be the case with other objects such as XRSpaces, XRReferenceSpaces or XRFrames. Even in we use a WeakPtr<XRSession> in WebXRInputSourceArray the ref cycle will continue with the XRSpaces hold by each XRInputSource contained by WebXRInputSourceArray.

Anyway, creating the Ref<WebXRSession> in the constructor is crearly wrong. Another pattern I have seen to solve that is create a initialize() function, which would initialize the WebXRInputSourceArray.

>> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:90
>>  // https://immersive-web.github.io/webxr/#dom-xrboundedreferencespace-boundsgeometry
> 
> Below this code, WebXRBoundedReferenceSpace::updateIfNeeded() indiscriminately dereferences m_session.
> How does that work?
> It seems to me that we need to keep WebXRSession alive whenever we have one of those *Space objects around.
> See my comment for WebXRFrame for potential solutions.

I had the same problem when I tried to use weak pointers here. The spec doesn't mention any exception for not valid XRSession. For example when calling getOffsetReferencesSpace, it needs to reuse the current session member to create a new offsetReferenceSpace. So if we use WebxRSession& ref pattern in the constructor it's not possible to create the object.
So according to the spec XRSpaces and XRFrames keep the session alive. We could create a issue in the spec to clarify that, because it's not explicitly mentioned but implicit based on some constraints.

[Imanol Fernandez]

Comment on attachment 426993 [details]
Patch

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

>>>> Source/WebCore/ChangeLog:10
>>>> +        itself, which caused a crash. This pattern was used elsewhere too.
>>> 
>>> ...and was a memory leak.
>> 
>> But indeed, if we has a cycle that didn’t get broken, a WeakPtr to break the cycle is suitable. I don’t know enough about this WebAPI to know if it is OK that all these objects no longer keep alive the session alive though. I know for example that in WebAudio, AudioNodes have to keep their AudioContext alive for the API to keep functioning after a GC.
> 
> The idea with the current code was the Ref<WebXRSession> & Ref<WebXRInputsourceArray> cycle is broken in WebXRSession::shutdown or end, which should always be called by the WebXRSystem if users don't call it manually. The spec doesn't mention if the session must be kept alive if users hold a WebXRInputsourceArray, but that's seems to be the case with other objects such as XRSpaces, XRReferenceSpaces or XRFrames. Even in we use a WeakPtr<XRSession> in WebXRInputSourceArray the ref cycle will continue with the XRSpaces hold by each XRInputSource contained by WebXRInputSourceArray.
> 
> Anyway, creating the Ref<WebXRSession> in the constructor is crearly wrong. Another pattern I have seen to solve that is create a initialize() function, which would initialize the WebXRInputSourceArray.

btw why hasn't this crash been reproduced in the build bots? is it because the assert it's only enabled in debug builds and currently WebXR tests are only run in WPE release? I created bug 222317 to enable WebXR tests cross platform , we should revisit it to detect this kind of issues before landing.

[Ryosuke Niwa]

(In reply to Imanol Fernandez from comment #17)
> Comment on attachment 426993 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426993&action=review
> 
> >>> Source/WebCore/ChangeLog:10
> >>> +        itself, which caused a crash. This pattern was used elsewhere too.
> >> 
> >> ...and was a memory leak.
> > 
> > But indeed, if we has a cycle that didn’t get broken, a WeakPtr to break the cycle is suitable. I don’t know enough about this WebAPI to know if it is OK that all these objects no longer keep alive the session alive though. I know for example that in WebAudio, AudioNodes have to keep their AudioContext alive for the API to keep functioning after a GC.
> 
> The idea with the current code was the Ref<WebXRSession> &
> Ref<WebXRInputsourceArray> cycle is broken in WebXRSession::shutdown or end,
> which should always be called by the WebXRSystem if users don't call it
> manually. The spec doesn't mention if the session must be kept alive if
> users hold a WebXRInputsourceArray, but that's seems to be the case with
> other objects such as XRSpaces, XRReferenceSpaces or XRFrames. Even in we
> use a WeakPtr<XRSession> in WebXRInputSourceArray the ref cycle will
> continue with the XRSpaces hold by each XRInputSource contained by
> WebXRInputSourceArray.

Note that keeping C++ object alive isn't same as keeping its wrapper object alive. Namely, it's possible for JS wrapper to be collected without its C++ backing object from being deleted. To solve that problem, we need to use opaque root and/or ActiveDOMObject to group a set of objects that need to be kept alive at the same time.

[Chris Dumez]

(In reply to Imanol Fernandez from comment #17)
> Comment on attachment 426993 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426993&action=review
> 
> >>> Source/WebCore/ChangeLog:10
> >>> +        itself, which caused a crash. This pattern was used elsewhere too.
> >> 
> >> ...and was a memory leak.
> > 
> > But indeed, if we has a cycle that didn’t get broken, a WeakPtr to break the cycle is suitable. I don’t know enough about this WebAPI to know if it is OK that all these objects no longer keep alive the session alive though. I know for example that in WebAudio, AudioNodes have to keep their AudioContext alive for the API to keep functioning after a GC.
> 
> The idea with the current code was the Ref<WebXRSession> &
> Ref<WebXRInputsourceArray> cycle is broken in WebXRSession::shutdown or end,
> which should always be called by the WebXRSystem if users don't call it
> manually.

Clearly the current code does NOT break the cycle in WebXRSession::shutdown(). Just look at WebXRSession::m_inputSources & WebXRInputSourceArray::m_session, Both Ref<>.

[Imanol Fernandez]

Comment on attachment 426993 [details]
Patch

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

>>>>>>> Source/WebCore/ChangeLog:10
>>>>>>> +        itself, which caused a crash. This pattern was used elsewhere too.
>>>>>> 
>>>>>> ...and was a memory leak.
>>>>> 
>>>>> But indeed, if we has a cycle that didn’t get broken, a WeakPtr to break the cycle is suitable. I don’t know enough about this WebAPI to know if it is OK that all these objects no longer keep alive the session alive though. I know for example that in WebAudio, AudioNodes have to keep their AudioContext alive for the API to keep functioning after a GC.
>>>> 
>>>> The idea with the current code was the Ref<WebXRSession> & Ref<WebXRInputsourceArray> cycle is broken in WebXRSession::shutdown or end, which should always be called by the WebXRSystem if users don't call it manually. The spec doesn't mention if the session must be kept alive if users hold a WebXRInputsourceArray, but that's seems to be the case with other objects such as XRSpaces, XRReferenceSpaces or XRFrames. Even in we use a WeakPtr<XRSession> in WebXRInputSourceArray the ref cycle will continue with the XRSpaces hold by each XRInputSource contained by WebXRInputSourceArray.
>>>> 
>>>> Anyway, creating the Ref<WebXRSession> in the constructor is crearly wrong. Another pattern I have seen to solve that is create a initialize() function, which would initialize the WebXRInputSourceArray.
>>> 
>>> btw why hasn't this crash been reproduced in the build bots? is it because the assert it's only enabled in debug builds and currently WebXR tests are only run in WPE release? I created bug 222317 to enable WebXR tests cross platform , we should revisit it to detect this kind of issues before landing.
>> 
>> Note that keeping C++ object alive isn't same as keeping its wrapper object alive. Namely, it's possible for JS wrapper to be collected without its C++ backing object from being deleted. To solve that problem, we need to use opaque root and/or ActiveDOMObject to group a set of objects that need to be kept alive at the same time.
> 
> Clearly the current code does NOT break the cycle in WebXRSession::shutdown(). Just look at WebXRSession::m_inputSources & WebXRInputSourceArray::m_session, Both Ref<>.

ah yes, you are right, I remember setting it to nullptr but it probably was done in a older patch using RefPtr which I changed later. Right now it clears the contents (internal input sources list) but not the reference itself.

[Dean Jackson]

Created attachment 427094 [details]
Patch

[Imanol Fernandez]

Comment on attachment 427094 [details]
Patch

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

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:-107
> -    if (&space.session() != m_session.ptr())

We should check the case of both space.session and m_session being nullptr here and below.

> Source/WebCore/Modules/webxr/WebXRInputSource.cpp:65
> +    if (!m_session)

Would it make sense to use the pattern that Sam mentioned?

auto session = makeRefPtr(m_session)
if (!session)
    return;

> Source/WebCore/Modules/webxr/WebXRInputSourceArray.cpp:71
> +    if (!m_session)

Would it make sense to use the pattern that Sam mentioned?

auto session = makeRefPtr(m_session)
if (!session)
    return;

> Source/WebCore/Modules/webxr/WebXRSession.cpp:545
> +            /*

Why is this commented out?

> Source/WebCore/Modules/webxr/WebXRSession.cpp:584
> +/*

Why is this commented out?

> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:144
> +/*

Why is this commented out?

[Dean Jackson]

Comment on attachment 427094 [details]
Patch

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

>> Source/WebCore/Modules/webxr/WebXRFrame.cpp:-107
>> -    if (&space.session() != m_session.ptr())
> 
> We should check the case of both space.session and m_session being nullptr here and below.

I added a check that m_session is not null. That should cover the space.session() case too, with the above line, as well as baseSpace.session().

>> Source/WebCore/Modules/webxr/WebXRInputSource.cpp:65
>> +    if (!m_session)
> 
> Would it make sense to use the pattern that Sam mentioned?
> 
> auto session = makeRefPtr(m_session)
> if (!session)
>     return;

Yes.

>> Source/WebCore/Modules/webxr/WebXRInputSourceArray.cpp:71
>> +    if (!m_session)
> 
> Would it make sense to use the pattern that Sam mentioned?
> 
> auto session = makeRefPtr(m_session)
> if (!session)
>     return;

Done.

>> Source/WebCore/Modules/webxr/WebXRSession.cpp:545
>> +            /*
> 
> Why is this commented out?

This is a mistake.

>> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:144
>> +/*
> 
> Why is this commented out?

This was a mistake

[Dean Jackson]

Created attachment 427945 [details]
Patch

[Sam Weinig]

Comment on attachment 427945 [details]
Patch

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

> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:47
> +Ref<WebXRBoundedReferenceSpace> WebXRBoundedReferenceSpace::create(Document& document, WebXRSession* session, XRReferenceSpaceType type)

This should take a WebXRSession& not a WebXRSession*. The only caller knows session is non-null on construction.

> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:52
> +Ref<WebXRBoundedReferenceSpace> WebXRBoundedReferenceSpace::create(Document& document, WebXRSession* session, Ref<WebXRRigidTransform>&& offset, XRReferenceSpaceType type)

Also should probably be a reference. Is this called? Maybe it can be removed.

> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:57
> +WebXRBoundedReferenceSpace::WebXRBoundedReferenceSpace(Document& document, WebXRSession* session, Ref<WebXRRigidTransform>&& offset, XRReferenceSpaceType type)

Should be a reference.

> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.h:43
> +    static Ref<WebXRBoundedReferenceSpace> create(Document&, WebXRSession*, XRReferenceSpaceType);
> +    static Ref<WebXRBoundedReferenceSpace> create(Document&, WebXRSession*, Ref<WebXRRigidTransform>&&, XRReferenceSpaceType);

Is this actually needed? Does the WebXRSession own a WebXRBoundedReferenceSpace?

> Source/WebCore/Modules/webxr/WebXRFrame.h:82
> -    Ref<WebXRSession> m_session;
> +    WeakPtr<WebXRSession> m_session;

Same question. Is this needed? Does the session own the frame?

> Source/WebCore/Modules/webxr/WebXRSession.h:132
> -    Ref<WebXRInputSourceArray> m_inputSources;
> +    RefPtr<WebXRInputSourceArray> m_inputSources;

What is this change for?

[Dean Jackson]

Comment on attachment 427945 [details]
Patch

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

> Source/WebCore/Modules/webxr/WebXRInputSource.cpp:50
> +WebXRInputSource::WebXRInputSource(Document& document, WebXRSession* session, double timestamp, const PlatformXR::Device::FrameData::InputSource& source)

This can be a reference.

[Dean Jackson]

Created attachment 428071 [details]
Patch

[Dean Jackson]

Created attachment 428073 [details]
Patch

[Dean Jackson]

Created attachment 428121 [details]
Patch

[Sam Weinig]

Comment on attachment 428121 [details]
Patch

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

> Source/WebCore/Modules/webxr/WebXRInputSourceArray.cpp:54
> +
> +void WebXRInputSourceArray::ref()

Accidental extra newline.

[Dean Jackson]

Committed r277251 (237520@main): <https://commits.webkit.org/237520@main>

[Lauro Moura]

Reopening for WPE buildfix.

[Lauro Moura]

Committed r277254 (237523@main): <https://commits.webkit.org/237523@main>