Bug 231482 – [WebXR] Replace the session reference in WebXRSpace subclasses with weak pointers

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

[Dean Jackson]

[WebXR] Replace the session reference in WebXRSpace subclasses with weak pointers

[Dean Jackson]

Created attachment 440718 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/84066341>

[Sam Weinig]

Comment on attachment 440718 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        WebXRSpace had a pure virtual session() accessor that returned
> +        a reference to a WebXRSession. This made subclasses hold strong
> +        references to the WebXRSession, and would become problematic
> +        for a WebXRSpace subclass that was (indirectly) owned by the
> +        WebXRSession (found in the Hand Input module).

Pretty sure this was already causing leaks elsewhere for other XRSpaces, like the targetRaySpace and gripSpace in WebXRInputSource.

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

This feels a bit weird to me. It at least needs a comment explaining itself, but I would assume this would propagate an Exception to the caller.

> Source/WebCore/Modules/webxr/WebXRReferenceSpace.cpp:120
> +    if (!m_session)
> +        return { };

Same as above.

> Source/WebCore/Modules/webxr/WebXRSpace.cpp:61
> +    WebXRSession* xrSession = session();
> +    if (!xrSession)
> +        return false;

Seems like this should be an exception rather than just returning false.

[Dean Jackson]

Created attachment 440732 [details]
EWS test

[Dean Jackson]

Comment on attachment 440718 [details]
Patch

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

>> Source/WebCore/Modules/webxr/WebXRReferenceSpace.cpp:72
>> +        return identity;
> 
> This feels a bit weird to me. It at least needs a comment explaining itself, but I would assume this would propagate an Exception to the caller.

Good point. I changed ::nativeOrigin to return an optional<TransformationMatrix>, updated all the subclasses, and return an Exception at the calling site if the session has disappeared.

>> Source/WebCore/Modules/webxr/WebXRReferenceSpace.cpp:120
>> +        return { };
> 
> Same as above.

Ditto.

>> Source/WebCore/Modules/webxr/WebXRSpace.cpp:61
>> +        return false;
> 
> Seems like this should be an exception rather than just returning false.

Done. Similar solution - this returns an optional that ends up returning an exception in the caller.

[Dean Jackson]

Committed r283880 (242757@main): <https://commits.webkit.org/242757@main>