Limit OpenXR backend to use only a single XrInstance

This change shares a single XrInstance between the XRRuntimeProvider,
OpenXrStatics, OpenXrDevice, OpenXrRenderLoop, and OpenXrApiWrapper.
The instance is actually owned by the OpenXrStatics, which is in turn
owned by the OpenXrDevice when one exists, and the XRRuntimeProvider
otherwise.

In terms of thead safety, the XrInstance will only be destroyed by the
OpenXrDevice destructor, which also owns the OpenXrRenderLoop and
explicitly shuts it down. Most methods which take an instance don't have
any threading concerns, but even so once we have a render loop we don't
make any calls against the instance from the OpenXrDevice thread until
the render loop is shut down.

Bug: 1100577
Change-Id: I2fc326e17c1faf3915e8eafddf23055ccd9a5639
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2278534
Commit-Queue: Brandon Jones <bajones@chromium.org>
Reviewed-by: Klaus Weidner <klausw@chromium.org>
Reviewed-by: Patrick To <patrto@microsoft.com>
Reviewed-by: Alexander Cooper <alcooper@chromium.org>
Cr-Commit-Position: refs/heads/master@{#786174}

device/vr/openxr/openxr_api_wrapper.cc

@@ -39,11 +39,12 @@ constexpr base::TimeDelta kTimeBetweenPollingEvents =
 
 }  // namespace
 
-std::unique_ptr<OpenXrApiWrapper> OpenXrApiWrapper::Create() {
+std::unique_ptr<OpenXrApiWrapper> OpenXrApiWrapper::Create(
+    XrInstance instance) {
   std::unique_ptr<OpenXrApiWrapper> openxr =
       std::make_unique<OpenXrApiWrapper>();
 
-  if (!openxr->Initialize()) {
+  if (!openxr->Initialize(instance)) {
     return nullptr;
   }
 
@@ -76,7 +77,7 @@ void OpenXrApiWrapper::Reset() {
   layer_projection_views_.clear();
 }
 
-bool OpenXrApiWrapper::Initialize() {
+bool OpenXrApiWrapper::Initialize(XrInstance instance) {
   Reset();
   session_running_ = false;
   pending_frame_ = false;
@@ -84,9 +85,9 @@ bool OpenXrApiWrapper::Initialize() {
   // call ProcessEvents, which will update this variable from there on.
   last_process_events_time_ = base::TimeTicks::Min();
 
-  if (XR_FAILED(CreateInstance(&instance_, &instance_metadata_))) {
-    return false;
-  }
+  DCHECK(instance != XR_NULL_HANDLE);
+  instance_ = instance;
+
   DCHECK(HasInstance());
 
   if (XR_FAILED(InitializeSystem())) {
@@ -117,11 +118,13 @@ bool OpenXrApiWrapper::IsInitialized() const {
 }
 
 void OpenXrApiWrapper::Uninitialize() {
-  // Destroying an instance in OpenXr also destroys all child objects of that
-  // instance (including the session, swapchain, and spaces objects),
+  // The instance is owned by the OpenXRDevice, so don't destroy it here.
+
+  // Destroying an session in OpenXr also destroys all child objects of that
+  // instance (including the swapchain, and spaces objects),
   // so they don't need to be manually destroyed.
-  if (HasInstance()) {
-    xrDestroyInstance(instance_);
+  if (HasSession()) {
+    xrDestroySession(session_);
   }
 
   if (test_hook_)
@@ -270,7 +273,9 @@ XrResult OpenXrApiWrapper::InitSession(
   CreateSpace(XR_REFERENCE_SPACE_TYPE_STAGE, &stage_space_);
   UpdateStageBounds();
 
-  if (instance_metadata_.unboundedReferenceSpaceSupported) {
+  OpenXrExtensionHelper extension_helper;
+  if (extension_helper.ExtensionSupported(
+          XR_MSFT_UNBOUNDED_REFERENCE_SPACE_EXTENSION_NAME)) {
     RETURN_IF_XR_FAILED(
         CreateSpace(XR_REFERENCE_SPACE_TYPE_UNBOUNDED_MSFT, &unbounded_space_));
   }

device/vr/openxr/openxr_api_wrapper.h

@@ -40,7 +40,7 @@ class OpenXrApiWrapper {
   ~OpenXrApiWrapper();
   bool IsInitialized() const;
 
-  static std::unique_ptr<OpenXrApiWrapper> Create();
+  static std::unique_ptr<OpenXrApiWrapper> Create(XrInstance instance);
 
   static VRTestHook* GetTestHook();
 
@@ -74,7 +74,7 @@ class OpenXrApiWrapper {
 
  private:
   void Reset();
-  bool Initialize();
+  bool Initialize(XrInstance instance);
   void Uninitialize();
 
   XrResult InitializeSystem();
@@ -124,7 +124,6 @@ class OpenXrApiWrapper {
 
   // These objects are valid on successful initialization.
   XrInstance instance_;
-  OpenXRInstanceMetadata instance_metadata_;
   XrSystemId system_;
   std::vector<XrViewConfigurationView> view_configs_;
   XrEnvironmentBlendMode blend_mode_;

device/vr/openxr/openxr_device.cc

@@ -59,6 +59,7 @@ mojom::VRDisplayInfoPtr CreateFakeVRDisplayInfo(device::mojom::XRDeviceId id) {
 // The OpenXrStatics object is owned by IsolatedXRRuntimeProvider.
 OpenXrDevice::OpenXrDevice(OpenXrStatics* openxr_statics)
     : VRDeviceBase(device::mojom::XRDeviceId::OPENXR_DEVICE_ID),
+      instance_(openxr_statics->GetXrInstance()),
       weak_ptr_factory_(this) {
   mojom::VRDisplayInfoPtr display_info = CreateFakeVRDisplayInfo(GetId());
   SetVRDisplayInfo(std::move(display_info));
@@ -86,8 +87,8 @@ void OpenXrDevice::EnsureRenderLoop() {
   if (!render_loop_) {
     auto on_info_changed = base::BindRepeating(&OpenXrDevice::SetVRDisplayInfo,
                                                weak_ptr_factory_.GetWeakPtr());
-    render_loop_ =
-        std::make_unique<OpenXrRenderLoop>(std::move(on_info_changed));
+    render_loop_ = std::make_unique<OpenXrRenderLoop>(
+        std::move(on_info_changed), instance_);
   }
 }
 

device/vr/openxr/openxr_device.h

@@ -14,6 +14,7 @@
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/receiver.h"
+#include "third_party/openxr/src/include/openxr/openxr.h"
 
 namespace device {
 
@@ -50,6 +51,7 @@ class DEVICE_VR_EXPORT OpenXrDevice
                               mojom::XRSessionPtr session);
   void OnPresentingControllerMojoConnectionError();
 
+  XrInstance instance_;
   std::unique_ptr<OpenXrRenderLoop> render_loop_;
 
   mojo::Receiver<mojom::XRSessionController> exclusive_controller_receiver_{

device/vr/openxr/openxr_render_loop.cc

@@ -16,9 +16,13 @@ namespace device {
 
 OpenXrRenderLoop::OpenXrRenderLoop(
     base::RepeatingCallback<void(mojom::VRDisplayInfoPtr)>
-        on_display_info_changed)
+        on_display_info_changed,
+    XrInstance instance)
     : XRCompositorCommon(),
-      on_display_info_changed_(std::move(on_display_info_changed)) {}
+      instance_(instance),
+      on_display_info_changed_(std::move(on_display_info_changed)) {
+  DCHECK(instance_ != XR_NULL_HANDLE);
+}
 
 OpenXrRenderLoop::~OpenXrRenderLoop() {
   Stop();
@@ -78,6 +82,7 @@ mojom::XRFrameDataPtr OpenXrRenderLoop::GetNextFrameData() {
 }
 
 bool OpenXrRenderLoop::StartRuntime() {
+  DCHECK(instance_ != XR_NULL_HANDLE);
   DCHECK(!openxr_);
   DCHECK(!input_helper_);
   DCHECK(!current_display_info_);
@@ -86,7 +91,8 @@ bool OpenXrRenderLoop::StartRuntime() {
   // openxr_ so that the local unique_ptr cleans up the object if starting
   // a session fails. openxr_ is set later in this method once we know
   // starting the session succeeds.
-  std::unique_ptr<OpenXrApiWrapper> openxr = OpenXrApiWrapper::Create();
+  std::unique_ptr<OpenXrApiWrapper> openxr =
+      OpenXrApiWrapper::Create(instance_);
   if (!openxr)
     return false;
 

device/vr/openxr/openxr_render_loop.h

@@ -23,7 +23,8 @@ class OpenXRInputHelper;
 class OpenXrRenderLoop : public XRCompositorCommon {
  public:
   OpenXrRenderLoop(base::RepeatingCallback<void(mojom::VRDisplayInfoPtr)>
-                       on_display_info_changed);
+                       on_display_info_changed,
+                   XrInstance instance);
   ~OpenXrRenderLoop() override;
 
  private:
@@ -46,6 +47,9 @@ class OpenXrRenderLoop : public XRCompositorCommon {
                  mojom::VREyeParametersPtr* eye) const;
   bool UpdateStageParameters();
 
+  // Owned by OpenXrStatics
+  XrInstance instance_;
+
   std::unique_ptr<OpenXrApiWrapper> openxr_;
   std::unique_ptr<OpenXRInputHelper> input_helper_;
   XrExtent2Df current_stage_bounds_;

device/vr/openxr/openxr_statics.cc

@@ -3,7 +3,6 @@
 // found in the LICENSE file.
 
 #include "device/vr/openxr/openxr_statics.h"
-
 #include "device/vr/openxr/openxr_util.h"
 
 namespace device {
@@ -17,8 +16,15 @@ OpenXrStatics::~OpenXrStatics() {
   }
 }
 
-bool OpenXrStatics::IsHardwareAvailable() {
+XrInstance OpenXrStatics::GetXrInstance() {
   if (instance_ == XR_NULL_HANDLE && XR_FAILED(CreateInstance(&instance_))) {
+    return XR_NULL_HANDLE;
+  }
+  return instance_;
+}
+
+bool OpenXrStatics::IsHardwareAvailable() {
+  if (GetXrInstance() == XR_NULL_HANDLE) {
     return false;
   }
 
@@ -27,15 +33,14 @@ bool OpenXrStatics::IsHardwareAvailable() {
 }
 
 bool OpenXrStatics::IsApiAvailable() {
-  return instance_ != XR_NULL_HANDLE ||
-         XR_SUCCEEDED(CreateInstance(&instance_));
+  return GetXrInstance() != XR_NULL_HANDLE;
 }
 
 #if defined(OS_WIN)
 // Returns the LUID of the adapter the OpenXR runtime is on. Returns {0, 0} if
 // the LUID could not be determined.
 LUID OpenXrStatics::GetLuid() {
-  if (!IsApiAvailable())
+  if (GetXrInstance() == XR_NULL_HANDLE)
     return {0, 0};
 
   XrSystemId system;

device/vr/openxr/openxr_statics.h

@@ -15,11 +15,15 @@
 
 namespace device {
 
+// OpenXrStatics must outlive all other OpenXR objects. It owns the XrInstance
+// and will destroy it in the destructor.
 class DEVICE_VR_EXPORT OpenXrStatics {
  public:
   OpenXrStatics();
   ~OpenXrStatics();
 
+  XrInstance GetXrInstance();
+
   bool IsHardwareAvailable();
   bool IsApiAvailable();
 
@@ -33,4 +37,4 @@ class DEVICE_VR_EXPORT OpenXrStatics {
 
 }  // namespace device
 
-#endif  // DEVICE_VR_WINDOWS_MIXED_REALITY_MIXED_REALITY_STATICS_H_
+#endif  // DEVICE_VR_OPENXR_OPENXR_STATICS_H_

device/vr/openxr/openxr_util.cc

@@ -56,8 +56,29 @@ bool IsRunningInWin32AppContainer() {
 }
 #endif
 
-XrResult CreateInstance(XrInstance* instance,
-                        OpenXRInstanceMetadata* metadata) {
+OpenXrExtensionHelper::OpenXrExtensionHelper() {
+  uint32_t extension_count;
+  if (XR_SUCCEEDED(xrEnumerateInstanceExtensionProperties(
+          nullptr, 0, &extension_count, nullptr))) {
+    extension_properties_.resize(extension_count,
+                                 {XR_TYPE_EXTENSION_PROPERTIES});
+    xrEnumerateInstanceExtensionProperties(nullptr, extension_count,
+                                           &extension_count,
+                                           extension_properties_.data());
+  }
+}
+
+OpenXrExtensionHelper::~OpenXrExtensionHelper() = default;
+
+bool OpenXrExtensionHelper::ExtensionSupported(const char* extension_name) {
+  return std::find_if(
+             extension_properties_.begin(), extension_properties_.end(),
+             [&extension_name](const XrExtensionProperties& properties) {
+               return strcmp(properties.extensionName, extension_name) == 0;
+             }) != extension_properties_.end();
+}
+
+XrResult CreateInstance(XrInstance* instance) {
   XrInstanceCreateInfo instance_create_info = {XR_TYPE_INSTANCE_CREATE_INFO};
 
   std::string application_name = version_info::GetProductName() + " " +
@@ -85,14 +106,6 @@ XrResult CreateInstance(XrInstance* instance,
 
   instance_create_info.applicationInfo.apiVersion = XR_CURRENT_API_VERSION;
 
-  uint32_t extensionCount;
-  RETURN_IF_XR_FAILED(xrEnumerateInstanceExtensionProperties(
-      nullptr, 0, &extensionCount, nullptr));
-  std::vector<XrExtensionProperties> extensionProperties(
-      extensionCount, {XR_TYPE_EXTENSION_PROPERTIES});
-  RETURN_IF_XR_FAILED(xrEnumerateInstanceExtensionProperties(
-      nullptr, extensionCount, &extensionCount, extensionProperties.data()));
-
   // xrCreateInstance validates the list of extensions and returns
   // XR_ERROR_EXTENSION_NOT_PRESENT if an extension is not supported,
   // so we don't need to call xrEnumerateInstanceExtensionProperties
@@ -113,25 +126,14 @@ XrResult CreateInstance(XrInstance* instance,
   // XR_MSFT_UNBOUNDED_REFERENCE_SPACE_EXTENSION_NAME, is required for optional
   // functionality (unbounded reference spaces) and thus only requested if it is
   // available.
-  auto extensionSupported = [&extensionProperties](const char* extensionName) {
-    return std::find_if(
-               extensionProperties.begin(), extensionProperties.end(),
-               [&extensionName](const XrExtensionProperties& properties) {
-                 return strcmp(properties.extensionName, extensionName) == 0;
-               }) != extensionProperties.end();
-  };
-
+  OpenXrExtensionHelper extension_helper;
   const bool unboundedSpaceExtensionSupported =
-      extensionSupported(XR_MSFT_UNBOUNDED_REFERENCE_SPACE_EXTENSION_NAME);
+      extension_helper.ExtensionSupported(
+          XR_MSFT_UNBOUNDED_REFERENCE_SPACE_EXTENSION_NAME);
   if (unboundedSpaceExtensionSupported) {
     extensions.push_back(XR_MSFT_UNBOUNDED_REFERENCE_SPACE_EXTENSION_NAME);
   }
 
-  if (metadata != nullptr) {
-    metadata->unboundedReferenceSpaceSupported =
-        unboundedSpaceExtensionSupported;
-  }
-
   instance_create_info.enabledExtensionCount =
       static_cast<uint32_t>(extensions.size());
   instance_create_info.enabledExtensionNames = extensions.data();

device/vr/openxr/openxr_util.h

@@ -5,12 +5,21 @@
 #ifndef DEVICE_VR_OPENXR_OPENXR_UTIL_H_
 #define DEVICE_VR_OPENXR_OPENXR_UTIL_H_
 
+#include <vector>
+
 #include "base/logging.h"
 #include "third_party/openxr/src/include/openxr/openxr.h"
 
 namespace device {
-struct OpenXRInstanceMetadata {
-  bool unboundedReferenceSpaceSupported;
+class OpenXrExtensionHelper {
+ public:
+  OpenXrExtensionHelper();
+  ~OpenXrExtensionHelper();
+
+  bool ExtensionSupported(const char* extension_name);
+
+ private:
+  std::vector<XrExtensionProperties> extension_properties_;
 };
 
 // These macros aren't common in Chromium and generally discouraged, so define
@@ -49,8 +58,7 @@ XrPosef PoseIdentity();
 
 XrResult GetSystem(XrInstance instance, XrSystemId* system);
 
-XrResult CreateInstance(XrInstance* instance,
-                        OpenXRInstanceMetadata* metadata = nullptr);
+XrResult CreateInstance(XrInstance* instance);
 
 }  // namespace device
 

device/vr/openxr/test/openxr_test_helper.cc

@@ -35,8 +35,7 @@ OpenXrTestHelper::OpenXrTestHelper()
     // since openxr_statics is created first, so the first instance returned
     // should be a fake one since openxr_statics does not need to use
     // test_hook_;
-    : create_fake_instance_(true),
-      system_id_(0),
+    : system_id_(0),
       session_(XR_NULL_HANDLE),
       swapchain_(XR_NULL_HANDLE),
       session_state_(XR_SESSION_STATE_UNKNOWN),
@@ -54,7 +53,6 @@ void OpenXrTestHelper::Reset() {
   swapchain_ = XR_NULL_HANDLE;
   session_state_ = XR_SESSION_STATE_UNKNOWN;
 
-  create_fake_instance_ = true;
   system_id_ = 0;
   frame_begin_ = false;
   d3d_device_ = nullptr;
@@ -193,16 +191,6 @@ XrSwapchain OpenXrTestHelper::GetSwapchain() {
 }
 
 XrInstance OpenXrTestHelper::CreateInstance() {
-  // Return the test helper object back to the OpenXrAPIWrapper so it can use
-  // it as the TestHookRegistration.However we have to return different instance
-  // for openxr_statics since openxr loader records instance created and
-  // detroyed. The first instance is used by openxr_statics which does not need
-  // to use test_hook_, So we can give it an arbitrary instance as long as
-  // ValidateInstance method remember it's an valid option.
-  if (create_fake_instance_) {
-    create_fake_instance_ = false;
-    return reinterpret_cast<XrInstance>(this + 1);
-  }
   return reinterpret_cast<XrInstance>(this);
 }
 
@@ -884,8 +872,7 @@ XrResult OpenXrTestHelper::ValidateInstance(XrInstance instance) const {
   // The Fake OpenXr Runtime returns this global OpenXrTestHelper object as the
   // instance value on xrCreateInstance.
 
-  RETURN_IF(reinterpret_cast<OpenXrTestHelper*>(instance) != this &&
-                reinterpret_cast<OpenXrTestHelper*>(instance) != (this + 1),
+  RETURN_IF(reinterpret_cast<OpenXrTestHelper*>(instance) != this,
             XR_ERROR_HANDLE_INVALID, "XrInstance invalid");
 
   return XR_SUCCESS;

device/vr/openxr/test/openxr_test_helper.h

@@ -182,8 +182,6 @@ class OpenXrTestHelper : public device::ServiceTestHook {
       device_test::mojom::InteractionProfileType type);
   bool IsSessionRunning() const;
 
-  bool create_fake_instance_;
-
   // Properties of the mock OpenXR runtime that doesn't change throughout the
   // lifetime of the instance. However, these aren't static because they are
   // initialized to an invalid value and set to their actual value in their