Ensure that every RequestFrame call has a matching FrameEnd call

When a frame is requested from the runtime, a frame has to be submitted
back to the runtime.  Even if that frame is a "missing" frame, without
any updated data.  This change refactors the logic around requesting a
frame to ensure that a frame isn't requested unless the callbacks would
be serviced.  If the state changes in the middle of a frame, then some
base layer should still be present to end the frame/submit the missing
frame back before it is removed.

If these conditions aren't met, the frame loop could stall out and never
be in a recoverable state.

This change also adds some tests around these conditions.

Bug: 1001730
Change-Id: I3b97322a7a8b1c15b37743c873382755fca7ec42
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1814102Reviewed-by: Jacob DeWitt <jacde@chromium.org>
Reviewed-by: Brian Sheedy <bsheedy@chromium.org>
Reviewed-by: Brandon Jones <bajones@chromium.org>
Commit-Queue: Alexander Cooper <alcooper@chromium.org>
Cr-Commit-Position: refs/heads/master@{#699037}

chrome/browser/vr/BUILD.gn

@@ -763,6 +763,7 @@ if (!is_android) {
       # Tests.
       sources += [
         "webxr_vr_consent_dialog_browser_test.cc",
+        "webxr_vr_frame_loop_browser_test.cc",
         "webxr_vr_frame_pose_browser_test.cc",
         "webxr_vr_indicators_browser_test.cc",
         "webxr_vr_input_browser_test.cc",

chrome/browser/vr/webxr_vr_frame_loop_browser_test.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <memory>
+
+#include "base/environment.h"
+#include "base/run_loop.h"
+#include "chrome/browser/vr/test/mock_xr_device_hook_base.h"
+#include "chrome/browser/vr/test/multi_class_browser_test.h"
+#include "chrome/browser/vr/test/ui_utils.h"
+#include "chrome/browser/vr/test/webxr_vr_browser_test.h"
+
+namespace vr {
+
+namespace {
+
+class MyXRMock : public MockXRDeviceHookBase {
+ public:
+  void OnFrameSubmitted(
+      device_test::mojom::SubmittedFrameDataPtr frame_data,
+      device_test::mojom::XRTestHook::OnFrameSubmittedCallback callback) final;
+
+  // The test waits for a submitted frame before returning.
+  void WaitForFrames(int count) {
+    DCHECK(!wait_loop_);
+    wait_frame_count_ = num_frames_submitted_ + count;
+
+    base::RunLoop* wait_loop =
+        new base::RunLoop(base::RunLoop::Type::kNestableTasksAllowed);
+    wait_loop_ = wait_loop;
+    wait_loop->Run();
+    delete wait_loop;
+  }
+
+  int FramesSubmitted() const { return num_frames_submitted_; }
+
+ private:
+  // Set to null on background thread after calling Quit(), so we can ensure we
+  // only call Quit once.
+  base::RunLoop* wait_loop_ = nullptr;
+
+  int wait_frame_count_ = 0;
+  int num_frames_submitted_ = 0;
+};
+
+void MyXRMock::OnFrameSubmitted(
+    device_test::mojom::SubmittedFrameDataPtr frame_data,
+    device_test::mojom::XRTestHook::OnFrameSubmittedCallback callback) {
+  num_frames_submitted_++;
+  if (num_frames_submitted_ >= wait_frame_count_ && wait_frame_count_ > 0 &&
+      wait_loop_) {
+    wait_loop_->Quit();
+    wait_loop_ = nullptr;
+  }
+
+  std::move(callback).Run();
+}
+
+}  // namespace
+
+WEBXR_VR_ALL_RUNTIMES_BROWSER_TEST_F(TestNoStalledFrameLoop) {
+  MyXRMock my_mock;
+
+  // Load the test page, and enter presentation.
+  t->LoadUrlAndAwaitInitialization(
+      t->GetFileUrlForHtmlTestFile("webxr_no_stalled_frame_loop"));
+  t->EnterSessionWithUserGestureOrFail();
+
+  // Wait for 2 frames to be submitted back to the device, but the js frame loop
+  // should've only been called once.
+  my_mock.WaitForFrames(2);
+  ASSERT_TRUE(t->RunJavaScriptAndExtractBoolOrFail("frame_count === 1"));
+
+  // Now restart the frame loop and wait for another frame to get submitted.
+  t->RunJavaScriptOrFail("setBaseLayer()");
+  t->PollJavaScriptBooleanOrFail("frame_count >= 2",
+                                 XrBrowserTestBase::kPollTimeoutMedium);
+
+  t->AssertNoJavaScriptErrors();
+}
+
+WEBXR_VR_ALL_RUNTIMES_BROWSER_TEST_F(TestLateSetOfBaseLayer) {
+  MyXRMock my_mock;
+
+  // Load the test page, and enter presentation.
+  t->LoadUrlAndAwaitInitialization(
+      t->GetFileUrlForHtmlTestFile("webxr_set_base_layer_late"));
+  t->EnterSessionWithUserGestureOrFail();
+
+  // Poll and have the javascript yield for 500 ms.  This should give us enough
+  // time for any frame requests that were going to propagate to propagate.
+  t->RunJavaScriptOrFail("delayMilliseconds(500)");
+  t->PollJavaScriptBooleanOrFail("delay_ended");
+
+  // No frames should have been submitted to either the JS or the runtime.
+  ASSERT_TRUE(t->RunJavaScriptAndExtractBoolOrFail("frame_count === 0"));
+  ASSERT_EQ(my_mock.FramesSubmitted(), 0);
+
+  // Now restart the frame loop and wait for a frame to get submitted.
+  t->RunJavaScriptOrFail("setBaseLayer()");
+  t->PollJavaScriptBooleanOrFail("frame_count >= 1",
+                                 XrBrowserTestBase::kPollTimeoutMedium);
+
+  t->AssertNoJavaScriptErrors();
+}
+
+}  // namespace vr

chrome/test/data/xr/e2e_test_files/html/webxr_no_stalled_frame_loop.html

+<!doctype html>
+<!--
+WebXR page that only submits one frame.
+-->
+<html>
+  <head>
+    <link rel="stylesheet" type="text/css" href="../resources/webxr_e2e.css">
+  </head>
+  <body>
+    <canvas id="webgl-canvas"></canvas>
+    <script src="../resources/webxr_e2e.js"></script>
+    <script src="../resources/webxr_boilerplate.js"></script>
+    <script src="../../../../../../third_party/blink/web_tests/resources/testharness.js"></script>
+    <script>
+      var frame_count = 0;
+      var frame_expected = true;
+      onImmersiveXRFrameCallback = function() {
+        assert_true(frame_expected);
+        frame_count++;
+
+        if (frame_count === 1) {
+          // No need to request an animationFrame because it should've been
+          // requested previously.
+          sessionInfos[sessionTypes.IMMERSIVE].session.updateRenderState({ baseLayer: null });
+          frame_expected = false;
+        }
+      };
+
+      function setBaseLayer() {
+        let session = sessionInfos[sessionTypes.IMMERSIVE].session;
+        session.updateRenderState({ baseLayer: new XRWebGLLayer(session, gl) });
+        frame_expected = true;
+      }
+    </script>
+  </body>
+</html>

chrome/test/data/xr/e2e_test_files/html/webxr_set_base_layer_late.html

+<!doctype html>
+<!--
+WebXR page that only submits one frame.
+-->
+<html>
+  <head>
+    <link rel="stylesheet" type="text/css" href="../resources/webxr_e2e.css">
+  </head>
+  <body>
+    <canvas id="webgl-canvas"></canvas>
+    <script src="../resources/webxr_e2e.js"></script>
+    <script src="../resources/webxr_boilerplate.js"></script>
+    <script src="../../../../../../third_party/blink/web_tests/resources/testharness.js"></script>
+    <script>
+      shouldSetBaseLayer = false;
+      var frame_count = 0;
+      var frame_expected = false;
+      var delay_ended = false;
+      onImmersiveXRFrameCallback = function() {
+        assert_true(frame_expected);
+        frame_count++;
+      };
+
+      function setBaseLayer() {
+        let session = sessionInfos[sessionTypes.IMMERSIVE].session;
+        session.updateRenderState({ baseLayer: new XRWebGLLayer(session, gl) });
+        frame_expected = true;
+      }
+
+      function delayMilliseconds(waitTime) {
+        delay_ended = false;
+        window.setTimeout(() => { delay_ended = true }, waitTime);
+      }
+    </script>
+  </body>
+</html>

chrome/test/data/xr/e2e_test_files/resources/webxr_boilerplate.js

@@ -24,6 +24,7 @@ var onPoseCallback = null;
 var shouldSubmitFrame = true;
 var hasPresentedFrame = false;
 var arSessionRequestWouldTriggerPermissionPrompt = null;
+var shouldSetBaseLayer = true;
 
 var sessionTypes = Object.freeze({
   IMMERSIVE: 1,
@@ -164,7 +165,10 @@ function onSessionStarted(session) {
     onSessionStartedCallback(session);
   }
 
-  session.updateRenderState({ baseLayer: new XRWebGLLayer(session, gl) });
+  if (shouldSetBaseLayer) {
+    session.updateRenderState({ baseLayer: new XRWebGLLayer(session, gl) });
+  }
+
   session.requestReferenceSpace(referenceSpaceMap[sessionType])
       .then( (refSpace) => {
         sessionInfos[sessionType].currentRefSpace = refSpace;

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

@@ -253,36 +253,20 @@ void XRSession::updateRenderState(XRRenderStateInit* init,
     return;
   }
 
-  if (init->hasBaseLayer() && init->baseLayer()) {
-    // Validate that any baseLayer provided was created with this session.
-    if (init->baseLayer()->session() != this) {
-      exception_state.ThrowDOMException(DOMExceptionCode::kInvalidStateError,
-                                        kIncompatibleLayer);
-      return;
-    }
-
-    // If the baseLayer was previously null and there are outstanding rAF
-    // callbacks, kick off a new frame request to flush them out.
-    if (!render_state_->baseLayer() && !pending_frame_ &&
-        !callback_collection_->IsEmpty()) {
-      // Kick off a request for a new XR frame.
-      xr_->frameProvider()->RequestFrame(this);
-      pending_frame_ = true;
-    }
-
-    if (!immersive() && init->baseLayer()->output_canvas()) {
-      // If the output canvas was previously null and there are outstanding rAF
-      // callbacks, kick off a new frame request to flush them out.
-      if (!render_state_->output_canvas() && !pending_frame_ &&
-          !callback_collection_->IsEmpty()) {
-        // Kick off a request for a new XR frame.
-        xr_->frameProvider()->RequestFrame(this);
-        pending_frame_ = true;
-      }
-    }
+  // Validate that any baseLayer provided was created with this session.
+  if (init->hasBaseLayer() && init->baseLayer() &&
+      init->baseLayer()->session() != this) {
+    exception_state.ThrowDOMException(DOMExceptionCode::kInvalidStateError,
+                                      kIncompatibleLayer);
+    return;
   }
 
   pending_render_state_.push_back(init);
+
+  // Updating our render state may have caused us to be in a state where we
+  // should be requesting frames again. Kick off a new frame request in case
+  // there are any pending callbacks to flush them out.
+  MaybeRequestFrame();
 }
 
 void XRSession::updateWorldTrackingState(
@@ -518,11 +502,7 @@ int XRSession::requestAnimationFrame(V8XRFrameRequestCallback* callback) {
     return 0;
 
   int id = callback_collection_->RegisterCallback(callback);
-  if (!pending_frame_) {
-    // Kick off a request for a new XR frame.
-    xr_->frameProvider()->RequestFrame(this);
-    pending_frame_ = true;
-  }
+  MaybeRequestFrame();
   return id;
 }
 
@@ -846,20 +826,35 @@ void XRSession::UpdateVisibilityState() {
   if (visibility_state_ != state) {
     visibility_state_ = state;
 
-    // If the visibility state was changed to something other than hidden and
-    // there are rAF callbacks still waiting to resolve kickstart the rAF loop
-    // again.
-    if (visibility_state_ != XRVisibilityState::HIDDEN && !pending_frame_ &&
-        !callback_collection_->IsEmpty()) {
-      xr_->frameProvider()->RequestFrame(this);
-      pending_frame_ = true;
-    }
+    // If the visibility state was changed to something other than hidden, we
+    // may be able to restart the frame loop.
+    MaybeRequestFrame();
 
     DispatchEvent(
         *XRSessionEvent::Create(event_type_names::kVisibilitychange, this));
   }
 }
 
+void XRSession::MaybeRequestFrame() {
+  bool will_have_base_layer = !!render_state_->baseLayer();
+  for (const auto& init : pending_render_state_) {
+    if (init->hasBaseLayer()) {
+      will_have_base_layer = !!init->baseLayer();
+    }
+  }
+
+  // We can request a frame if we're not hidden, we don't already have a pending
+  // frame, we have pending callbacks, and we will have a base layer when it
+  // resolves.
+  bool can_request_frame =
+      visibility_state_ != XRVisibilityState::HIDDEN && !pending_frame_ &&
+      !callback_collection_->IsEmpty() && will_have_base_layer;
+  if (can_request_frame) {
+    xr_->frameProvider()->RequestFrame(this);
+    pending_frame_ = true;
+  }
+}
+
 void XRSession::DetachOutputCanvas(HTMLCanvasElement* canvas) {
   if (!canvas)
     return;
@@ -876,8 +871,9 @@ void XRSession::DetachOutputCanvas(HTMLCanvasElement* canvas) {
 }
 
 void XRSession::ApplyPendingRenderState() {
+  DCHECK(!prev_base_layer_);
   if (pending_render_state_.size() > 0) {
-    XRWebGLLayer* prev_base_layer = render_state_->baseLayer();
+    prev_base_layer_ = render_state_->baseLayer();
     HTMLCanvasElement* prev_ouput_canvas = render_state_->output_canvas();
     update_views_next_frame_ = true;
 
@@ -890,7 +886,7 @@ void XRSession::ApplyPendingRenderState() {
     // If this is an inline session and the base layer has changed, give it an
     // opportunity to update it's drawing buffer size.
     if (!immersive() && render_state_->baseLayer() &&
-        render_state_->baseLayer() != prev_base_layer) {
+        render_state_->baseLayer() != prev_base_layer_) {
       render_state_->baseLayer()->OnResize();
     }
 
@@ -961,6 +957,7 @@ void XRSession::OnFrame(
     return;
 
   // If there are pending render state changes, apply them now.
+  prev_base_layer_ = nullptr;
   ApplyPendingRenderState();
 
   if (pending_frame_) {
@@ -969,8 +966,17 @@ void XRSession::OnFrame(
     // Don't allow frames to be processed if there's no layers attached to the
     // session. That would allow tracking with no associated visuals.
     XRWebGLLayer* frame_base_layer = render_state_->baseLayer();
-    if (!frame_base_layer)
+    if (!frame_base_layer) {
+      // If we previously had a frame base layer, we need to still attempt to
+      // submit a frame back to the runtime, as all "GetFrameData" calls need a
+      // matching submit.
+      if (prev_base_layer_) {
+        prev_base_layer_->OnFrameStart(output_mailbox_holder);
+        prev_base_layer_->OnFrameEnd();
+        prev_base_layer_ = nullptr;
+      }
       return;
+    }
 
     // Don't allow frames to be processed if an inline session doesn't have an
     // output canvas.
@@ -1312,6 +1318,7 @@ void XRSession::Trace(blink::Visitor* visitor) {
   visitor->Trace(create_anchor_promises_);
   visitor->Trace(reference_spaces_);
   visitor->Trace(anchor_ids_to_anchors_);
+  visitor->Trace(prev_base_layer_);
   EventTargetWithInlineData::Trace(visitor);
 }
 

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

@@ -45,6 +45,7 @@ class XRRenderStateInit;
 class XRRigidTransform;
 class XRSpace;
 class XRViewData;
+class XRWebGLLayer;
 class XRWorldInformation;
 class XRWorldTrackingState;
 class XRWorldTrackingStateInit;
@@ -243,6 +244,8 @@ class XRSession final
   void UpdateCanvasDimensions(Element*);
   void ApplyPendingRenderState();
 
+  void MaybeRequestFrame();
+
   void OnInputStateChangeInternal(
       int16_t frame_id,
       base::span<const device::mojom::blink::XRInputSourceStatePtr>
@@ -293,6 +296,7 @@ class XRSession final
   WTF::Vector<XRViewData> views_;
 
   Member<XRInputSourceArray> input_sources_;
+  Member<XRWebGLLayer> prev_base_layer_;
   Member<ResizeObserver> resize_observer_;
   Member<XRCanvasInputProvider> canvas_input_provider_;
   bool environment_error_handler_subscribed_ = false;

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

@@ -10,10 +10,16 @@ let testName = "Calling end during an input callback stops processing at the rig
 
 let fakeDeviceInitParams = TRACKED_IMMERSIVE_DEVICE;
 
+let gl = null;
+
 function requestImmersiveSession() {
   return new Promise((resolve, reject) => {
     navigator.xr.test.simulateUserActivation(() => {
       navigator.xr.requestSession('immersive-vr').then((session) => {
+        session.updateRenderState({
+          baseLayer: new XRWebGLLayer(session, gl)
+        });
+
         resolve(session);
       }, (err) => {
         reject(err);
@@ -22,8 +28,8 @@ function requestImmersiveSession() {
   });
 }
 
-let testFunction = function(session, fakeDeviceController, t) {
-
+let testFunction = function(session, fakeDeviceController, t, sessionObjects) {
+  gl = sessionObjects.gl;
   // helper method to send a click and then request a dummy animation frame to
   // ensure that the click propagates.  We're doing everything in these tests
   // from event watchers, we just need to trigger the add/click to make the