Delay restarting inline rAF loop until page is notified of session end

Currently the inline rAF loop gets restarted before the page is
notified that the immersive session has ended. This causes some flaky
test cases where the inline callback is serviced before the page knows
that it's okay, since the immersive session has ended. This change
splits the FrameProvider's OnSessionEnded logic such that it can be
notified of the fact that there is no longer an immersive session before
the page is (so that the page is not blocked from requesting a new
immersive session), while only re-starting the inline rAF loops after
the page has been notified.

Fixed: 1062195
Change-Id: I2ccf63d476b7f857bc53e9c15bedc07a13fdfed6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2354537Reviewed-by: Brandon Jones <bajones@chromium.org>
Commit-Queue: Alexander Cooper <alcooper@chromium.org>
Cr-Commit-Position: refs/heads/master@{#797812}

third_party/blink/renderer/modules/xr/xr_frame_provider.cc

@@ -156,23 +156,26 @@ void XRFrameProvider::OnSessionEnded(XRSession* session) {
         session->GetExecutionContext(),
         session->GetExecutionContext()->GetTaskRunner(
             TaskType::kMiscPlatformAPI));
-
-    // When we no longer have an active immersive session schedule all the
-    // outstanding frames that were requested while the immersive session was
-    // active.
-    if (requesting_sessions_.size() > 0) {
-      for (auto& session : requesting_sessions_) {
-        RequestNonImmersiveFrameData(session.key.Get());
-      }
-
-      ScheduleNonImmersiveFrame(nullptr);
-    }
   } else {
     non_immersive_data_providers_.erase(session);
     requesting_sessions_.erase(session);
   }
 }
 
+void XRFrameProvider::RestartNonImmersiveFrameLoop() {
+  // When we no longer have an active immersive session schedule all the
+  // outstanding frames that were requested while the immersive session was
+  // active.
+  if (immersive_session_ || requesting_sessions_.size() == 0)
+    return;
+
+  for (auto& session : requesting_sessions_) {
+    RequestNonImmersiveFrameData(session.key.Get());
+  }
+
+  ScheduleNonImmersiveFrame(nullptr);
+}
+
 // Schedule a session to be notified when the next XR frame is available.
 void XRFrameProvider::RequestFrame(XRSession* session) {
   DVLOG(3) << __FUNCTION__;

third_party/blink/renderer/modules/xr/xr_frame_provider.h

@@ -32,7 +32,13 @@ class XRFrameProvider final : public GarbageCollected<XRFrameProvider> {
 
   void OnSessionStarted(XRSession* session,
                         device::mojom::blink::XRSessionPtr session_ptr);
+
+  // The FrameProvider needs to be notified before the page does that the
+  // session has been ended so that requesting a new session is possible.
+  // However, the non-immersive frame loop shouldn't start until after the page
+  // has been notified.
   void OnSessionEnded(XRSession* session);
+  void RestartNonImmersiveFrameLoop();
 
   void RequestFrame(XRSession*);
 

third_party/blink/renderer/modules/xr/xr_session.cc

@@ -1254,7 +1254,10 @@ void XRSession::HandleShutdown() {
     return;
   }
 
-  // Notify the frame provider that we've ended
+  // Notify the frame provider that we've ended. Do this before notifying the
+  // page, so that if the page tries (and is able to) create a session within
+  // either the promise or the event callback, it's not blocked by the frame
+  // provider thinking there's still an active immersive session.
   xr_->frameProvider()->OnSessionEnded(this);
 
   if (end_session_resolver_) {
@@ -1265,6 +1268,11 @@ void XRSession::HandleShutdown() {
 
   DispatchEvent(*XRSessionEvent::Create(event_type_names::kEnd, this));
   DVLOG(3) << __func__ << ": session end event dispatched";
+
+  // Now that we've notified the page that we've ended, try to restart the non-
+  // immersive frame loop. Note that if the page was able to request a new
+  // session in the end event, this may be a no-op.
+  xr_->frameProvider()->RestartNonImmersiveFrameLoop();
 }
 
 double XRSession::NativeFramebufferScale() const {

third_party/blink/web_tests/external/wpt/webxr/xrSession_requestSessionDuringEnd.https.html

+<!DOCTYPE html>
+<body>
+  <script src=/resources/testharness.js></script>
+  <script src=/resources/testharnessreport.js></script>
+  <script src="resources/webxr_util.js"></script>
+  <script src="resources/webxr_test_constants.js"></script>
+  <canvas></canvas>
+
+  <script>
+    function testFunctionGenerator(createSessionFromEventCallback) {
+        return function(session, testDeviceController, t) {
+            let done = false;
+
+            function createSession() {
+              return new Promise((resolve) => {
+                  navigator.xr.requestSession("immersive-vr")
+                  .then((new_session) => {
+                    // The test framework ensures that the created session ends,
+                    // but will not do cleanup for this session, so if it gets
+                    // created, we need to ensure that it gets cleaned up.
+                    return new_session.end();
+                  }).then(() => {
+                    done = true;
+                    resolve();
+                  }).catch((err) => {
+                    // Only one catch is needed for the whole promise chain.
+                    // If ending the new session throws, it's fine to fail as
+                    // we'd otherwise end up in a bad state.
+                    t.step(() => {
+                      assert_unreached("Session creation should not throw: " + err);
+                    });
+                  });
+              });
+            }
+
+            function onSessionEnd() {
+                if (createSessionFromEventCallback) {
+                  createSession();
+                }
+            }
+
+            session.addEventListener("end", onSessionEnd, false);
+
+            // We need to simulate the user activation before we call end as
+            // otherwise (depending on the implementation) it can interfere with
+            // the scheduling of the dispatched event/promise, and make session
+            // creation succeed even when there may be bugs preventing it from
+            // doing so in real scenarios.
+            navigator.xr.test.simulateUserActivation(() => {
+              session.end().then(() => {
+                if (!createSessionFromEventCallback) {
+                  createSession();
+                }
+              });
+            });
+
+            return t.step_wait(() => done);
+        };
+    }
+
+    xr_session_promise_test("Create new session in OnSessionEnded event",
+      testFunctionGenerator(/*createSessionFromEventCallback=*/true),
+      TRACKED_IMMERSIVE_DEVICE, 'immersive-vr');
+
+    xr_session_promise_test("Create mew session in end promise",
+      testFunctionGenerator(/*createSessionFromEventCallback=*/false),
+      TRACKED_IMMERSIVE_DEVICE, 'immersive-vr');
+  </script>
+</body>