Reject WebXR session requests with consistent errors

The service_ pointer was getting nulled-out asynchronously, so we'd
sometimes reject early based on the service_ being null and sometimes
pass that check and reject with no user gesture.

The fix makes things consistent but not yet spec compliant - issue
962991 will make things spec compliant.

Bug: 962685
Change-Id: I69bae46f02e0027ef69365c1c8c47d72af0dee3b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1630719Reviewed-by: David Dorwin <ddorwin@chromium.org>
Commit-Queue: Bill Orr <billorr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664383}

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

@@ -284,15 +284,6 @@ ScriptPromise XR::requestSession(ScriptState* script_state,
                                            kFeaturePolicyBlocked));
   }
 
-  // If we no longer have a valid service connection reject the request, unless
-  // it was for an inline mode.  In which case, we'll end up creating the
-  // session in OnRequestSessionReturned.
-  if (!service_ && session_mode != XRSession::kModeInline) {
-    return ScriptPromise::RejectWithDOMException(
-        script_state, MakeGarbageCollected<DOMException>(
-                          DOMExceptionCode::kNotFoundError, kNoDevicesMessage));
-  }
-
   // Only one immersive session can be active at a time.
   if (is_immersive && frameProvider()->immersive_session()) {
     return ScriptPromise::RejectWithDOMException(
@@ -310,6 +301,19 @@ ScriptPromise XR::requestSession(ScriptState* script_state,
                                            kRequestRequiresUserActivation));
   }
 
+  // TODO(https://crbug.com/962991): The error handling here is not spec
+  // compliant. The spec says to reject based on no device before rejecting
+  // based on user gesture.
+  // If we no longer have a valid service connection reject the request, unless
+  // it was for an inline mode.  In which case, we'll end up creating the
+  // session in OnRequestSessionReturned.
+  if (!service_ && session_mode != XRSession::kModeInline) {
+    return ScriptPromise::RejectWithDOMException(
+        script_state,
+        MakeGarbageCollected<DOMException>(DOMExceptionCode::kNotSupportedError,
+                                           kNoDevicesMessage));
+  }
+
   auto* resolver = MakeGarbageCollected<ScriptPromiseResolver>(script_state);
   ScriptPromise promise = resolver->Promise();
 
@@ -340,6 +344,9 @@ void XR::DispatchRequestSession(PendingSessionQuery* query) {
       return;
     }
 
+    // TODO(https://crbug.com/962991): The spec says to reject with null.
+    // Clarify/fix the spec. In other places where we have no device or no
+    // service we return kNotSupportedError.
     query->resolver->Reject(MakeGarbageCollected<DOMException>(
         DOMExceptionCode::kNotSupportedError, kSessionNotSupported));
     return;
@@ -613,6 +620,9 @@ void XR::OnDeviceDisconnect() {
   HeapHashSet<Member<PendingSessionQuery>> request_queries =
       outstanding_request_queries_;
   for (const auto& query : request_queries) {
+    // We had a device, so rejecting the session request as though the mode
+    // isn't supported. TODO(https://crbug.com/962991): The spec should specify
+    // what is returned here.
     OnRequestSessionReturned(query, nullptr);
   }
   DCHECK(outstanding_support_queries_.IsEmpty());

third_party/blink/web_tests/http/tests/origin_trials/webexposed/xr/webxr-origin-trial-interfaces.html

@@ -92,24 +92,22 @@ test(t => {
 // Ensure that AR sessions are not exposed as part of the WebXR origin trial.
 // The webXREnabled check is effectively checking whether experimental features
 // are enabled. This works as long as at least one AR feature is experimental.
-// TODO(https://crbug.com/962685): Make the supportsSession checks first after
-// this issue is fixed.
 promise_test(t => {
-  let promise = navigator.xr.requestSession('immersive-ar');
+  let promise = navigator.xr.supportsSession('immersive-ar');
   if (OriginTrialsHelper.is_runtime_flag_enabled('webXREnabled')) {
-    return promise_rejects(t, "SecurityError", promise);
+    return promise_rejects(t, "NotSupportedError", promise);
   } else {
     return promise_rejects(t, new TypeError(), promise);
   }
-}, "immersive-ar is not recognized by requestSession() with a WebXR token.");
+}, "immersive-ar is not recognized by supportsSession() with a WebXR token.");
 promise_test(t => {
-  let promise = navigator.xr.supportsSession('immersive-ar');
+  let promise = navigator.xr.requestSession('immersive-ar');
   if (OriginTrialsHelper.is_runtime_flag_enabled('webXREnabled')) {
-    return promise_rejects(t, "NotSupportedError", promise);
+    return promise_rejects(t, "SecurityError", promise);
   } else {
     return promise_rejects(t, new TypeError(), promise);
   }
-}, "immersive-ar is not recognized by supportsSession() with a WebXR token.");
+}, "immersive-ar is not recognized by requestSession() with a WebXR token.");
 // Verify the rejection reason matches that for other invalid enum values.
 // It only makes sense to run these when the failure occurs.
 if (!OriginTrialsHelper.is_runtime_flag_enabled('webXREnabled')) {