Bug 225025 - [WebXR] Use weak pointers to reference WebXRSessions
Summary: [WebXR] Use weak pointers to reference WebXRSessions
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks: 208988
  Show dependency treegraph
 
Reported: 2021-04-24 17:07 PDT by Dean Jackson
Modified: 2021-04-27 02:14 PDT (History)
7 users (show)

See Also:


Attachments
Patch (38.41 KB, patch)
2021-04-24 17:22 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (55.04 KB, patch)
2021-04-26 14:48 PDT, Dean Jackson
dino: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2021-04-24 17:07:15 PDT
[WebXR] Use weak pointers to reference WebXRSessions
Comment 1 Radar WebKit Bug Importer 2021-04-24 17:09:57 PDT
<rdar://problem/77111217>
Comment 2 Dean Jackson 2021-04-24 17:22:50 PDT
Created attachment 426993 [details]
Patch
Comment 3 Dean Jackson 2021-04-24 17:27:48 PDT
I'm surprised this patch builds on gtk/wpe!
Comment 4 Sam Weinig 2021-04-25 09:24:06 PDT
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.
Comment 5 Chris Dumez 2021-04-25 09:33:41 PDT
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.
Comment 6 Chris Dumez 2021-04-25 09:37:30 PDT
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.
Comment 7 Chris Dumez 2021-04-25 09:44:12 PDT
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.
Comment 8 Dean Jackson 2021-04-25 11:20:51 PDT
(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.
Comment 9 Sam Weinig 2021-04-25 11:43:44 PDT
(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?
Comment 10 Sam Weinig 2021-04-25 11:45:31 PDT
(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.
Comment 11 Sam Weinig 2021-04-25 11:46:07 PDT
(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.
Comment 12 Chris Dumez 2021-04-25 12:25:41 PDT
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.
Comment 13 Chris Dumez 2021-04-25 12:31:04 PDT
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.
Comment 14 Dean Jackson 2021-04-25 13:53:12 PDT
(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.
Comment 15 Dean Jackson 2021-04-25 13:53:56 PDT
Yep. That's what I'm doing. (I should have reloaded before commenting)
Comment 16 Ryosuke Niwa 2021-04-25 14:09:46 PDT
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?
Comment 17 Imanol Fernandez 2021-04-26 01:58:20 PDT
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.
Comment 18 Imanol Fernandez 2021-04-26 02:11:28 PDT
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.
Comment 19 Ryosuke Niwa 2021-04-26 02:16:41 PDT
(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.
Comment 20 Chris Dumez 2021-04-26 08:26:41 PDT
(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<>.
Comment 21 Imanol Fernandez 2021-04-26 08:39:52 PDT
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.
Comment 22 Dean Jackson 2021-04-26 14:48:54 PDT
Created attachment 427094 [details]
Patch
Comment 23 Imanol Fernandez 2021-04-27 02:14:45 PDT
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?