WebXR: ensure magic window sessions get disconnected

Ensure that magic window sessions get disconnected when VRServiceImpl is
destroyed. The default destructors don't do so, and this resulted in ID
collisions when the metrics helper is reused across page navigation.

Also add DVLOG(2) lifecycle logging to help with debugging in case we
run into similar issues in the future, the initialization sequence
is a bit complicated.

Bug: 1017959
Change-Id: Ib7876ec10c0db7076b599ec212762cb455ec5c0c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1884373
Commit-Queue: Klaus Weidner <klausw@chromium.org>
Reviewed-by: Alexander Cooper <alcooper@chromium.org>
Cr-Commit-Position: refs/heads/master@{#710012}

chrome/browser/vr/metrics/session_metrics_helper.cc

@@ -168,6 +168,7 @@ SessionMetricsHelper* SessionMetricsHelper::CreateForWebContents(
 
 SessionMetricsHelper::SessionMetricsHelper(content::WebContents* contents,
                                            Mode initial_mode) {
+  DVLOG(2) << __func__;
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(contents);
 
@@ -192,7 +193,9 @@ SessionMetricsHelper::SessionMetricsHelper(content::WebContents* contents,
   UpdateMode();
 }
 
-SessionMetricsHelper::~SessionMetricsHelper() = default;
+SessionMetricsHelper::~SessionMetricsHelper() {
+  DVLOG(2) << __func__;
+}
 
 void SessionMetricsHelper::UpdateMode() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);

chrome/browser/vr/service/browser_xr_runtime.cc

@@ -226,6 +226,7 @@ BrowserXRRuntime::BrowserXRRuntime(
     : id_(id),
       runtime_(std::move(runtime)),
       display_info_(ValidateVRDisplayInfo(display_info.get(), id)) {
+  DVLOG(2) << __func__ << ": id=" << id;
   // Unretained is safe because we are calling through an InterfacePtr we own,
   // so we won't be called after runtime_ is destroyed.
   runtime_->ListenToDeviceChanges(
@@ -234,9 +235,12 @@ BrowserXRRuntime::BrowserXRRuntime(
                      base::Unretained(this)));
 }
 
-BrowserXRRuntime::~BrowserXRRuntime() = default;
+BrowserXRRuntime::~BrowserXRRuntime() {
+  DVLOG(2) << __func__ << ": id=" << id_;
+}
 
 void BrowserXRRuntime::ExitActiveImmersiveSession() {
+  DVLOG(2) << __func__;
   auto* service = GetServiceWithActiveImmersiveSession();
   if (service) {
     service->ExitPresent();
@@ -411,6 +415,7 @@ void BrowserXRRuntime::OnVisibilityStateChanged(
 }
 
 void BrowserXRRuntime::OnInitialized() {
+  DVLOG(2) << __func__;
   for (auto& callback : pending_initialization_callbacks_) {
     std::move(callback).Run(display_info_.Clone());
   }
@@ -418,10 +423,12 @@ void BrowserXRRuntime::OnInitialized() {
 }
 
 void BrowserXRRuntime::OnServiceAdded(VRServiceImpl* service) {
+  DVLOG(2) << __func__ << ": id=" << id_;
   services_.insert(service);
 }
 
 void BrowserXRRuntime::OnServiceRemoved(VRServiceImpl* service) {
+  DVLOG(2) << __func__ << ": id=" << id_;
   DCHECK(service);
   services_.erase(service);
   if (service == presenting_service_) {
@@ -436,6 +443,7 @@ void BrowserXRRuntime::OnServiceRemoved(VRServiceImpl* service) {
 }
 
 void BrowserXRRuntime::ExitPresent(VRServiceImpl* service) {
+  DVLOG(2) << __func__ << ": id=" << id_;
   if (service == presenting_service_) {
     StopImmersiveSession();
   }
@@ -454,6 +462,7 @@ void BrowserXRRuntime::RequestSession(
     VRServiceImpl* service,
     const device::mojom::XRRuntimeSessionOptionsPtr& options,
     RequestSessionCallback callback) {
+  DVLOG(2) << __func__ << ": id=" << id_;
   // base::Unretained is safe because we won't be called back after runtime_ is
   // destroyed.
   runtime_->RequestSession(
@@ -471,6 +480,7 @@ void BrowserXRRuntime::OnRequestSessionResult(
     mojo::PendingRemote<device::mojom::XRSessionController>
         immersive_session_controller) {
   if (session && service) {
+    DVLOG(2) << __func__ << ": id=" << id_;
     if (options->immersive) {
       presenting_service_ = service.get();
       immersive_session_controller_.Bind(
@@ -499,6 +509,7 @@ void BrowserXRRuntime::OnRequestSessionResult(
 }
 
 void BrowserXRRuntime::OnImmersiveSessionError() {
+  DVLOG(2) << __func__ << ": id=" << id_;
   StopImmersiveSession();
 }
 
@@ -517,6 +528,7 @@ void BrowserXRRuntime::UpdateListeningForActivate(VRServiceImpl* service) {
 void BrowserXRRuntime::InitializeAndGetDisplayInfo(
     content::RenderFrameHost* render_frame_host,
     device::mojom::VRService::GetImmersiveVRDisplayInfoCallback callback) {
+  DVLOG(2) << __func__ << ": id=" << id_;
   device::mojom::VRDisplayInfoPtr device_info = GetVRDisplayInfo();
   if (device_info) {
     std::move(callback).Run(std::move(device_info));

chrome/browser/vr/service/vr_service_impl.cc

@@ -105,6 +105,7 @@ VRServiceImpl::VRServiceImpl(content::RenderFrameHost* render_frame_host)
       render_frame_host_(render_frame_host),
       in_focused_frame_(render_frame_host->GetView()->HasFocus()) {
   DCHECK(render_frame_host_);
+  DVLOG(2) << __func__;
 
   runtime_manager_ = XRRuntimeManager::GetOrCreateInstance();
   runtime_manager_->AddService(this);
@@ -118,17 +119,27 @@ VRServiceImpl::VRServiceImpl(content::RenderFrameHost* render_frame_host)
 // Constructor for testing.
 VRServiceImpl::VRServiceImpl(util::PassKey<XRRuntimeManagerTest>)
     : render_frame_host_(nullptr) {
+  DVLOG(2) << __func__;
   runtime_manager_ = XRRuntimeManager::GetOrCreateInstance();
   runtime_manager_->AddService(this);
 }
 
 VRServiceImpl::~VRServiceImpl() {
+  DVLOG(2) << __func__;
+  // Ensure that any active magic window sessions are disconnected to avoid
+  // collisions when a new session starts. See https://crbug.com/1017959, the
+  // disconnect handler doesn't get called automatically on page navigation.
+  for (auto it = magic_window_controllers_.begin();
+       it != magic_window_controllers_.end(); ++it) {
+    OnInlineSessionDisconnected(it.id());
+  }
   runtime_manager_->RemoveService(this);
 }
 
 void VRServiceImpl::Create(
     content::RenderFrameHost* render_frame_host,
     mojo::PendingReceiver<device::mojom::VRService> receiver) {
+  DVLOG(2) << __func__;
   std::unique_ptr<VRServiceImpl> vr_service_impl =
       std::make_unique<VRServiceImpl>(render_frame_host);
 
@@ -140,6 +151,7 @@ void VRServiceImpl::Create(
 void VRServiceImpl::InitializationComplete() {
   // After initialization has completed, we can correctly answer
   // supportsSession, and can provide correct display capabilities.
+  DVLOG(2) << __func__;
   initialization_complete_ = true;
 
   ResolvePendingRequests();
@@ -152,6 +164,7 @@ void VRServiceImpl::SetClient(
     return;
   }
 
+  DVLOG(2) << __func__;
   service_client_.Bind(std::move(service_client));
 }
 
@@ -172,6 +185,7 @@ void VRServiceImpl::OnDisplayInfoChanged() {
 }
 
 void VRServiceImpl::RuntimesChanged() {
+  DVLOG(2) << __func__;
   OnDisplayInfoChanged();
 
   if (service_client_) {
@@ -245,6 +259,8 @@ void VRServiceImpl::OnInlineSessionCreated(
   controller->SetFrameDataRestricted(!in_focused_frame_);
 
   auto id = magic_window_controllers_.Add(std::move(controller));
+  DVLOG(2) << __func__ << ": session_id=" << id.GetUnsafeValue()
+           << " runtime_id=" << session_runtime_id;
 
   // Note: We might be recording an inline session that was created by WebVR.
   GetSessionMetricsHelper()->RecordInlineSessionStart(id.GetUnsafeValue());
@@ -255,6 +271,7 @@ void VRServiceImpl::OnInlineSessionCreated(
 
 void VRServiceImpl::OnInlineSessionDisconnected(
     mojo::RemoteSetElementId session_id) {
+  DVLOG(2) << __func__ << ": session_id=" << session_id.GetUnsafeValue();
   // Notify metrics helper that inline session was stopped.
   auto* metrics_helper = GetSessionMetricsHelper();
   metrics_helper->RecordInlineSessionStop(session_id.GetUnsafeValue());
@@ -280,6 +297,8 @@ void VRServiceImpl::OnSessionCreated(
     device::mojom::VRService::RequestSessionCallback callback,
     const std::set<device::mojom::XRSessionFeature>& enabled_features,
     device::mojom::XRSessionPtr session) {
+  DVLOG(2) << __func__ << ": session_runtime_id=" << session_runtime_id;
+
   if (!session) {
     std::move(callback).Run(
         device::mojom::RequestSessionResult::NewFailureReason(
@@ -307,6 +326,7 @@ void VRServiceImpl::OnSessionCreated(
 void VRServiceImpl::RequestSession(
     device::mojom::XRSessionOptionsPtr options,
     device::mojom::VRService::RequestSessionCallback callback) {
+  DVLOG(2) << __func__;
   DCHECK(options);
 
   // Queue the request to get to when initialization has completed.
@@ -479,6 +499,7 @@ void VRServiceImpl::DoRequestSession(
     device::mojom::VRService::RequestSessionCallback callback,
     BrowserXRRuntime* runtime,
     std::set<device::mojom::XRSessionFeature> enabled_features) {
+  DVLOG(2) << __func__;
   // Get the runtime we'll be creating a session with.
   DCHECK(runtime);
 
@@ -590,6 +611,7 @@ void VRServiceImpl::GetImmersiveVRDisplayInfo(
 }
 
 void VRServiceImpl::OnExitPresent() {
+  DVLOG(2) << __func__;
   for (auto& client : session_clients_)
     client->OnExitPresent();
 }
@@ -603,12 +625,14 @@ void VRServiceImpl::OnVisibilityStateChanged(
 
 void VRServiceImpl::OnActivate(device::mojom::VRDisplayEventReason reason,
                                base::OnceCallback<void(bool)> on_handled) {
+  DVLOG(2) << __func__;
   if (display_client_) {
     display_client_->OnActivate(reason, std::move(on_handled));
   }
 }
 
 void VRServiceImpl::OnDeactivate(device::mojom::VRDisplayEventReason reason) {
+  DVLOG(2) << __func__;
   if (display_client_) {
     display_client_->OnDeactivate(reason);
   }

chrome/browser/vr/service/xr_runtime_manager.cc

@@ -117,6 +117,7 @@ void XRRuntimeManager::ExitImmersivePresentation() {
 
 void XRRuntimeManager::AddService(VRServiceImpl* service) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DVLOG(2) << __func__;
 
   // Loop through any currently active runtimes and send Connected messages to
   // the service. Future runtimes that come online will send a Connected message
@@ -131,6 +132,7 @@ void XRRuntimeManager::AddService(VRServiceImpl* service) {
 
 void XRRuntimeManager::RemoveService(VRServiceImpl* service) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DVLOG(2) << __func__;
   services_.erase(service);
 
   for (const auto& runtime : runtimes_) {

device/vr/orientation/orientation_device.cc

@@ -57,6 +57,7 @@ VROrientationDevice::VROrientationDevice(mojom::SensorProvider* sensor_provider,
                                          base::OnceClosure ready_callback)
     : VRDeviceBase(mojom::XRDeviceId::ORIENTATION_DEVICE_ID),
       ready_callback_(std::move(ready_callback)) {
+  DVLOG(2) << __func__;
   sensor_provider->GetSensor(kOrientationSensorType,
                              base::BindOnce(&VROrientationDevice::SensorReady,
                                             base::Unretained(this)));
@@ -64,7 +65,9 @@ VROrientationDevice::VROrientationDevice(mojom::SensorProvider* sensor_provider,
   SetVRDisplayInfo(CreateVRDisplayInfo(GetId()));
 }
 
-VROrientationDevice::~VROrientationDevice() = default;
+VROrientationDevice::~VROrientationDevice() {
+  DVLOG(2) << __func__;
+}
 
 void VROrientationDevice::SensorReady(
     device::mojom::SensorCreationResult,
@@ -76,6 +79,7 @@ void VROrientationDevice::SensorReady(
     return;
   }
 
+  DVLOG(2) << __func__;
   constexpr size_t kReadBufferSize = sizeof(device::SensorReadingSharedBuffer);
 
   DCHECK_EQ(0u, params->buffer_offset % kReadBufferSize);
@@ -132,6 +136,7 @@ void VROrientationDevice::HandleSensorError() {
 void VROrientationDevice::RequestSession(
     mojom::XRRuntimeSessionOptionsPtr options,
     mojom::XRRuntime::RequestSessionCallback callback) {
+  DVLOG(2) << __func__;
   DCHECK(!options->immersive);
 
   // TODO(http://crbug.com/695937): Perform a check to see if sensors are
@@ -156,6 +161,7 @@ void VROrientationDevice::RequestSession(
 }
 
 void VROrientationDevice::EndMagicWindowSession(VROrientationSession* session) {
+  DVLOG(2) << __func__;
   base::EraseIf(magic_window_sessions_,
                 [session](const std::unique_ptr<VROrientationSession>& item) {
                   return item.get() == session;