FontAccess: Fail concurrent font chooser requests

This changes the behavior from concurrent font chooser requests all
returning the same data from a single chooser invocation to failing a
subsequent request.

As implemented, the User Activation check will consume the activation,
failing any other in-flight requests.

Fixed: 1149621
Change-Id: If14e7e12ff52967e8dc49c383a66da94995c9134
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2568931
Commit-Queue: Olivier Yiptong <oyiptong@chromium.org>
Auto-Submit: Olivier Yiptong <oyiptong@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#833129}

third_party/blink/renderer/modules/font_access/font_manager.cc

@@ -66,50 +66,45 @@ ScriptValue FontManager::query(ScriptState* script_state,
 
 ScriptPromise FontManager::showFontChooser(ScriptState* script_state,
                                            const QueryOptions* options) {
-  // TODO(crbug.com/1149621): Queue up font chooser requests.
-  if (!pending_resolver_) {
-    remote_manager_->ChooseLocalFonts(
-        WTF::Bind(&FontManager::DidShowFontChooser, WrapWeakPersistent(this)));
-    pending_resolver_ =
-        MakeGarbageCollected<ScriptPromiseResolver>(script_state);
-  }
+  auto* resolver = MakeGarbageCollected<ScriptPromiseResolver>(script_state);
+  ScriptPromise promise = resolver->Promise();
+
+  remote_manager_->ChooseLocalFonts(WTF::Bind(&FontManager::DidShowFontChooser,
+                                              WrapWeakPersistent(this),
+                                              WrapPersistent(resolver)));
 
-  return pending_resolver_->Promise();
+  return promise;
 }
 
 void FontManager::Trace(blink::Visitor* visitor) const {
   ScriptWrappable::Trace(visitor);
   ContextLifecycleObserver::Trace(visitor);
-  visitor->Trace(pending_resolver_);
 }
 
 void FontManager::DidShowFontChooser(
+    ScriptPromiseResolver* resolver,
     mojom::blink::FontEnumerationStatus status,
     Vector<mojom::blink::FontMetadataPtr> fonts) {
   switch (status) {
     case mojom::blink::FontEnumerationStatus::kOk:
       break;
     case mojom::blink::FontEnumerationStatus::kUnimplemented:
-      pending_resolver_->Reject(MakeGarbageCollected<DOMException>(
+      resolver->Reject(MakeGarbageCollected<DOMException>(
           DOMExceptionCode::kNotSupportedError,
           "Not yet supported on this platform."));
-      pending_resolver_.Clear();
       return;
     case mojom::blink::FontEnumerationStatus::kCanceled:
-      pending_resolver_->Reject(MakeGarbageCollected<DOMException>(
+      resolver->Reject(MakeGarbageCollected<DOMException>(
           DOMExceptionCode::kAbortError, "The user canceled the operation."));
-      pending_resolver_.Clear();
       return;
     case mojom::blink::FontEnumerationStatus::kNeedsUserActivation:
-      pending_resolver_->Reject(MakeGarbageCollected<DOMException>(
+      resolver->Reject(MakeGarbageCollected<DOMException>(
           DOMExceptionCode::kSecurityError, "User activation is required."));
-      pending_resolver_.Clear();
       return;
     case mojom::blink::FontEnumerationStatus::kUnexpectedError:
     default:
-      pending_resolver_->Reject(MakeGarbageCollected<DOMException>(
+      resolver->Reject(MakeGarbageCollected<DOMException>(
           DOMExceptionCode::kUnknownError, "An unexpected error occured."));
-      pending_resolver_.Clear();
       return;
   }
 
@@ -119,8 +114,7 @@ void FontManager::DidShowFontChooser(
                                       font->family};
     entries.push_back(FontMetadata::Create(std::move(entry)));
   }
-  pending_resolver_->Resolve(std::move(entries));
-  pending_resolver_.Clear();
+  resolver->Resolve(std::move(entries));
 }
 
 void FontManager::ContextDestroyed() {

third_party/blink/renderer/modules/font_access/font_manager.h

@@ -39,13 +39,13 @@ class FontManager final : public ScriptWrappable,
   void Trace(blink::Visitor*) const override;
 
  private:
-  void DidShowFontChooser(mojom::blink::FontEnumerationStatus status,
+  void DidShowFontChooser(ScriptPromiseResolver* resolver,
+                          mojom::blink::FontEnumerationStatus status,
                           Vector<mojom::blink::FontMetadataPtr> fonts);
   void ContextDestroyed() override;
   void OnDisconnect();
 
   mojo::Remote<mojom::blink::FontAccessManager> remote_manager_;
-  Member<ScriptPromiseResolver> pending_resolver_;
 };
 
 }  // namespace blink

third_party/blink/web_tests/NeverFixTests

@@ -2214,8 +2214,10 @@ crbug.com/626703 [ Win ] external/wpt/html/editing/dnd/events/historical-manual.
 external/wpt/font-access/font_access-blob.tentative.https.window.html [ Skip ]
 external/wpt/font-access/font_access-enumeration.tentative.https.window.html [ Skip ]
 external/wpt/font-access/font_access-chooser.tentative.manual.https.html [ Skip ]
+external/wpt/font-access/font_access-chooser-multiple.tentative.manual.https.html [ Skip ]
 virtual/font-access/external/wpt/font-access/font_access-blob.tentative.https.window.html [ Pass ]
 virtual/font-access/external/wpt/font-access/font_access-chooser.tentative.manual.https.html [ Skip ]
+virtual/font-access/external/wpt/font-access/font_access-chooser-multiple.tentative.manual.https.html [ Skip ]
 virtual/font-access/external/wpt/font-access/font_access-enumeration.tentative.https.window.html [ Pass ]
 
 # text-orientation:upright

third_party/blink/web_tests/external/wpt/font-access/font_access-chooser-multiple.tentative.manual.https.html

+<!doctype html>
+<title>Local Font Access: Multiple choosers</title>
+<meta charset=utf-8>
+
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="/resources/testdriver.js"></script>
+<script src="/resources/testdriver-vendor.js"></script>
+
+<script>
+  promise_test(async t => {
+    await new Promise(resolve => {
+      window.addEventListener('DOMContentLoaded', resolve);
+    });
+    // Small delay to give chrome's test automation a chance to actually install
+    // itself.
+    await new Promise(resolve => step_timeout(resolve, 100));
+
+    await window.test_driver.bless('show a font chooser.<br>Please select at least one font.');
+    const promise = navigator.fonts.showFontChooser()
+    promise_rejects_dom(t, 'SecurityError', navigator.fonts.showFontChooser());
+    const fonts = await promise;
+  }, 'showFontChooser multiple choosers');
+</script>