Bug 225025

<rdar://problem/77111217>

Created attachment 426993 [details] Patch

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 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 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 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.

(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.

(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?

(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.

(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 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 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.

(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.

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

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 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 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.

(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.

(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 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.

Created attachment 427094 [details] Patch

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?

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

Created attachment 427945 [details] Patch

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?

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.

Created attachment 428071 [details] Patch

Created attachment 428073 [details] Patch

Created attachment 428121 [details] Patch

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.

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

Reopening for WPE buildfix.

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