Enforce active lifetime of XRFrame objects

Reland of https://chromium-review.googlesource.com/c/chromium/src/+/1343065
Incorporates fixes made by jacde@chromium.org (Thanks!)

Deactivates XRFrame objects once the relevant callback returns, which
causes future calls to the object methods to throw an exception.

Bug: 906842
Change-Id: Ib635f56ed28b728434a08347b9d9fb98d1ecd694
Reviewed-on: https://chromium-review.googlesource.com/c/1363836
Commit-Queue: Brandon Jones <bajones@chromium.org>
Reviewed-by: Klaus Weidner <klausw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#633845}

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

@@ -17,6 +17,7 @@ test can query for whether each submitted frame used the correct pose.
     <script>
       var frame_id = 0;
       var frame_data_array = {};
+      var pose_array = {};
       // We exit presentation before checking stuff that needs the frame of
       // reference, so we need to cache its value.
       var cached_frame_of_ref = null;
@@ -42,13 +43,12 @@ test can query for whether each submitted frame used the correct pose.
 
       function checkFrameView(frame_id, eye, expected) {
         let frame_data = frame_data_array[frame_id];
-        let pose = frame_data.getViewerPose(cached_frame_of_ref);
+        let pose = pose_array[frame_id];
         return MatrixCompare(pose.views[eye].viewMatrix, expected);
       }
 
       function checkFramePose(frame_id, expected) {
-        let frame_data = frame_data_array[frame_id];
-        let pose = frame_data.getViewerPose(cached_frame_of_ref);
+        let pose = pose_array[frame_id];
         if (!pose) {
           console.log("unexpected - null pose");
           return false;
@@ -63,6 +63,7 @@ test can query for whether each submitted frame used the correct pose.
         frame_id++;
         frame_data_array[frame_id] = frame;
         cached_frame_of_ref = sessionInfos[sessionTypes.IMMERSIVE].currentRefSpace;
+        pose_array[frame_id] = frame.getViewerPose(cached_frame_of_ref);
 
         var encoded_frame_id = {};
         encoded_frame_id.r = frame_id % 256;

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

@@ -10,12 +10,26 @@
 #include "third_party/blink/renderer/modules/xr/xr_session.h"
 #include "third_party/blink/renderer/modules/xr/xr_view.h"
 #include "third_party/blink/renderer/modules/xr/xr_viewer_pose.h"
+#include "third_party/blink/renderer/platform/bindings/exception_state.h"
 
 namespace blink {
 
+namespace {
+
+const char kInactiveFrame[] =
+    "XRFrame access outside the callback that produced it is invalid.";
+}
+
 XRFrame::XRFrame(XRSession* session) : session_(session) {}
 
-XRViewerPose* XRFrame::getViewerPose(XRReferenceSpace* reference_space) const {
+XRViewerPose* XRFrame::getViewerPose(XRReferenceSpace* reference_space,
+                                     ExceptionState& exception_state) const {
+  if (!active_) {
+    exception_state.ThrowDOMException(DOMExceptionCode::kInvalidStateError,
+                                      kInactiveFrame);
+    return nullptr;
+  }
+
   session_->LogGetPose();
 
   // Must use a reference space created from the same session.
@@ -46,7 +60,14 @@ XRViewerPose* XRFrame::getViewerPose(XRReferenceSpace* reference_space) const {
 }
 
 XRInputPose* XRFrame::getInputPose(XRInputSource* input_source,
-                                   XRReferenceSpace* reference_space) const {
+                                   XRReferenceSpace* reference_space,
+                                   ExceptionState& exception_state) const {
+  if (!active_) {
+    exception_state.ThrowDOMException(DOMExceptionCode::kInvalidStateError,
+                                      kInactiveFrame);
+    return nullptr;
+  }
+
   if (!input_source || !reference_space) {
     return nullptr;
   }
@@ -123,6 +144,10 @@ void XRFrame::SetBasePoseMatrix(const TransformationMatrix& base_pose_matrix) {
   base_pose_matrix_ = TransformationMatrix::Create(base_pose_matrix);
 }
 
+void XRFrame::Deactivate() {
+  active_ = false;
+}
+
 void XRFrame::Trace(blink::Visitor* visitor) {
   visitor->Trace(session_);
   ScriptWrappable::Trace(visitor);

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

@@ -14,6 +14,7 @@
 
 namespace blink {
 
+class ExceptionState;
 class XRViewerPose;
 class XRInputPose;
 class XRInputSource;
@@ -28,16 +29,21 @@ class XRFrame final : public ScriptWrappable {
 
   XRSession* session() const { return session_; }
 
-  XRViewerPose* getViewerPose(XRReferenceSpace*) const;
-  XRInputPose* getInputPose(XRInputSource*, XRReferenceSpace*) const;
+  XRViewerPose* getViewerPose(XRReferenceSpace*, ExceptionState&) const;
+  XRInputPose* getInputPose(XRInputSource*,
+                            XRReferenceSpace*,
+                            ExceptionState&) const;
 
   void SetBasePoseMatrix(const TransformationMatrix&);
 
   void Trace(blink::Visitor*) override;
 
+  void Deactivate();
+
  private:
   const Member<XRSession> session_;
   std::unique_ptr<TransformationMatrix> base_pose_matrix_;
+  bool active_ = true;
 };
 
 }  // namespace blink

third_party/blink/renderer/modules/xr/xr_frame.idl

@@ -10,7 +10,7 @@
 ] interface XRFrame {
   readonly attribute XRSession session;
 
-  XRViewerPose? getViewerPose(XRReferenceSpace referenceSpace);
-  XRInputPose? getInputPose(XRInputSource inputSource,
+  [RaisesException] XRViewerPose? getViewerPose(XRReferenceSpace referenceSpace);
+  [RaisesException] XRInputPose? getInputPose(XRInputSource inputSource,
                             XRReferenceSpace referenceSpace);
 };

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

@@ -613,6 +613,9 @@ void XRSession::OnFrame(
     // OnFrameEnd if it's still valid.
     if (!ended_)
       frame_base_layer->OnFrameEnd();
+
+    // Ensure the XRFrame cannot be used outside the callbacks.
+    presentation_frame->Deactivate();
   }
 }
 
@@ -725,6 +728,9 @@ void XRSession::OnSelectStart(XRInputSource* input_source) {
 
   if (event->defaultPrevented())
     input_source->selection_cancelled = true;
+
+  // Ensure the frame cannot be used outside of the event handler.
+  event->frame()->Deactivate();
 }
 
 void XRSession::OnSelectEnd(XRInputSource* input_source) {
@@ -747,6 +753,9 @@ void XRSession::OnSelectEnd(XRInputSource* input_source) {
 
   if (event->defaultPrevented())
     input_source->selection_cancelled = true;
+
+  // Ensure the frame cannot be used outside of the event handler.
+  event->frame()->Deactivate();
 }
 
 void XRSession::OnSelect(XRInputSource* input_source) {
@@ -764,6 +773,9 @@ void XRSession::OnSelect(XRInputSource* input_source) {
     XRInputSourceEvent* event =
         CreateInputSourceEvent(event_type_names::kSelect, input_source);
     DispatchEvent(*event);
+
+    // Ensure the frame cannot be used outside of the event handler.
+    event->frame()->Deactivate();
   }
 }
 

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

+<!DOCTYPE html>
+<body>
+  <script src=/resources/testharness.js></script>
+  <script src=/resources/testharnessreport.js></script>
+  <script src="resources/webxr_util.js"></script>
+  <canvas></canvas>
+
+  <script>
+    let immersiveTestName = "XRFrame methods throw exceptions outside of the " +
+      "requestAnimationFrame callback for immersive sessions";
+    let nonImmersiveTestName = "XRFrame methods throw exceptions outside of the " +
+      "requestAnimationFrame callback for non-immersive sessions";
+
+    let fakeDeviceInitParams = { supportsImmersive:true };
+
+    let immersiveSessionOptions = { mode: 'immersive-vr' };
+    let nonImmersiveSessionOptions = { outputContext: getOutputContext() };
+
+    let testFunction = (testSession, testController, t) => new Promise((resolve) => {
+      let staleFrame = null;
+      let currentReferenceSpace = null;
+
+      function onFrame(time, xrFrame) {
+        t.step(() => {
+          assert_true(xrFrame instanceof XRFrame);
+        });
+
+        staleFrame = xrFrame;
+        step_timeout(afterFrame, 0);
+      }
+
+      function afterFrame() {
+        t.step(() => {
+          // Attempting to call a method on the frame outside the callback that
+          // originally provided it should cause it to throw an exception.
+          assert_throws('InvalidStateError', () => staleFrame.getViewerPose(currentReferenceSpace));
+        });
+
+        // Test does not complete until the this function has executed.
+        resolve();
+      }
+
+      testSession.requestReferenceSpace({ type: 'stationary', subtype: 'eye-level' }).then((referenceSpace) => {
+        currentReferenceSpace = referenceSpace;
+        testSession.requestAnimationFrame(onFrame);
+      });
+    });
+
+    xr_session_promise_test(immersiveTestName, testFunction,
+      fakeDeviceInitParams, immersiveSessionOptions);
+    xr_session_promise_test(nonImmersiveTestName, testFunction,
+      fakeDeviceInitParams, nonImmersiveSessionOptions);
+
+  </script>
+</body>
\ No newline at end of file