[webnfc] Do not support multiple scans on the same reader.

We added support for multiple scan invocations on the same Reader in
this CL https://chromium-review.googlesource.com/c/chromium/src/+/2145263

Since Origin Trials feedback was to remove Filtering,
spec : https://github.com/w3c/web-nfc/pull/565
WIP CL : https://chromium-review.googlesource.com/c/chromium/src/+/2225770/
we tried to reason if in a world without filtering, multiple scans on
the same Reader is needed at all. The present thinking is to simply
reject scan promise when there's already an ongoing scan.

Spec discussions: https://github.com/w3c/web-nfc/issues/592

Bug: 520391
Change-Id: Ic50873ddc30351cec6656ac7b030394dc9fa18ea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2297559
Commit-Queue: Rijubrata Bhaumik <rijubrata.bhaumik@intel.com>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#790438}

third_party/blink/renderer/modules/nfc/ndef_reader.cc

@@ -37,6 +37,7 @@ namespace {
 constexpr char kNotSupportedOrPermissionDenied[] =
     "WebNFC feature is unavailable or permission denied.";
 
+constexpr char kInvalidStateErrorMessage[] = "A scan() operation is ongoing.";
 }  // namespace
 
 // static
@@ -93,21 +94,27 @@ ScriptPromise NDEFReader::scan(ScriptState* script_state,
     return ScriptPromise();
   }
 
-  // With the note in
-  // https://w3c.github.io/web-nfc/#the-ndefreader-and-ndefwriter-objects,
-  // successive invocations of NDEFReader.scan() with new options should replace
-  // existing filters. So stop current reading for this case.
+  if (has_pending_scan_request_) {
+    exception_state.ThrowDOMException(DOMExceptionCode::kInvalidStateError,
+                                      kInvalidStateErrorMessage);
+    return ScriptPromise();
+  }
+  has_pending_scan_request_ = true;
+
+  // https://github.com/w3c/web-nfc/issues/592
+  // reject scan promise when there's already an ongoing scan.
   if (GetNfcProxy()->IsReading(this)) {
-    Abort(signal_.Get());
+    exception_state.ThrowDOMException(DOMExceptionCode::kInvalidStateError,
+                                      kInvalidStateErrorMessage);
+    return ScriptPromise();
   }
 
   resolver_ = MakeGarbageCollected<ScriptPromiseResolver>(script_state);
-  // 8. If reader.[[Signal]] is not null, then add the following abort steps to
-  // reader.[[Signal]]:
+  // 8. If reader.[[Signal]] is not null, then add the following abort steps
+  // to reader.[[Signal]]:
   if (options->hasSignal()) {
-    signal_ = options->signal();
-    signal_->AddAlgorithm(WTF::Bind(&NDEFReader::Abort, WrapPersistent(this),
-                                    WrapPersistent(options->signal())));
+    options->signal()->AddAlgorithm(
+        WTF::Bind(&NDEFReader::Abort, WrapPersistent(this)));
   }
 
   GetPermissionService()->RequestPermission(
@@ -130,16 +137,20 @@ PermissionService* NDEFReader::GetPermissionService() {
 
 void NDEFReader::OnRequestPermission(const NDEFScanOptions* options,
                                      PermissionStatus status) {
-  if (!resolver_)
+  if (!resolver_) {
+    has_pending_scan_request_ = false;
     return;
+  }
 
   if (status != PermissionStatus::GRANTED) {
+    has_pending_scan_request_ = false;
     resolver_->Reject(MakeGarbageCollected<DOMException>(
         DOMExceptionCode::kNotAllowedError, "NFC permission request denied."));
     resolver_.Clear();
     return;
   }
   if (options->hasSignal() && options->signal()->aborted()) {
+    has_pending_scan_request_ = false;
     resolver_->Reject(MakeGarbageCollected<DOMException>(
         DOMExceptionCode::kAbortError, "The NFC operation was cancelled."));
     resolver_.Clear();
@@ -157,6 +168,7 @@ void NDEFReader::OnRequestPermission(const NDEFScanOptions* options,
 
 void NDEFReader::OnScanRequestCompleted(
     device::mojom::blink::NDEFErrorPtr error) {
+  has_pending_scan_request_ = false;
   if (!resolver_)
     return;
 
@@ -173,7 +185,6 @@ void NDEFReader::OnScanRequestCompleted(
 void NDEFReader::Trace(Visitor* visitor) const {
   visitor->Trace(permission_service_);
   visitor->Trace(resolver_);
-  visitor->Trace(signal_);
   EventTargetWithInlineData::Trace(visitor);
   ActiveScriptWrappable::Trace(visitor);
   ExecutionContextLifecycleObserver::Trace(visitor);
@@ -217,14 +228,7 @@ void NDEFReader::ContextDestroyed() {
   GetNfcProxy()->StopReading(this);
 }
 
-void NDEFReader::Abort(AbortSignal* signal) {
-  // In the case of successive invocations of NDEFReader.scan() with
-  // different signals, we should make sure aborting on previous signal
-  // won't abort current reading.
-  // If this is not triggered by the current signal, just ignore it.
-  if (signal && signal != signal_)
-    return;
-
+void NDEFReader::Abort() {
   if (resolver_) {
     resolver_->Reject(MakeGarbageCollected<DOMException>(
         DOMExceptionCode::kAbortError, "The NFC operation was cancelled."));

third_party/blink/renderer/modules/nfc/ndef_reader.h

@@ -18,7 +18,6 @@
 
 namespace blink {
 
-class AbortSignal;
 class ExecutionContext;
 class NFCProxy;
 class NDEFScanOptions;
@@ -61,7 +60,7 @@ class MODULES_EXPORT NDEFReader : public EventTargetWithInlineData,
   // ExecutionContextLifecycleObserver overrides.
   void ContextDestroyed() override;
 
-  void Abort(AbortSignal* signal);
+  void Abort();
 
   NFCProxy* GetNfcProxy() const;
 
@@ -79,11 +78,8 @@ class MODULES_EXPORT NDEFReader : public EventTargetWithInlineData,
   // case the callback passed to Watch() won't be called and
   // mojo::WrapCallbackWithDefaultInvokeIfNotRun() is forbidden in Blink.
   Member<ScriptPromiseResolver> resolver_;
-
-  // Currently AbortSignal has no method to remove an algorithm so this
-  // field tracks the most recently configured AbortSignal so that others
-  // can be ignored.
-  Member<AbortSignal> signal_;
+  // To reject if there is already an ongoing scan.
+  bool has_pending_scan_request_ = false;
 };
 
 }  // namespace blink

third_party/blink/web_tests/external/wpt/web-nfc/NDEFReader_scan.https.html

@@ -284,4 +284,19 @@ nfc_test(async (t, mockNFC) => {
   await promise;
 }, "Test that reading message with multiple records should succeed.");
 
+nfc_test(async (t, mockNFC) => {
+  const reader = new NDEFReader();
+  const promise1 = reader.scan();
+  const promise2 = promise_rejects_dom(t, 'InvalidStateError', reader.scan());
+  await promise1;
+  await promise2;
+}, "Test that NDEFReader.scan rejects if there is already an ongoing scan.");
+
+nfc_test(async (t, mockNFC) => {
+  const reader = new NDEFReader();
+  const controller = new AbortController();
+  await reader.scan({signal : controller.signal});
+  controller.abort();
+  await reader.scan();
+}, "Test that NDEFReader.scan can be started after the previous scan is aborted.");
 </script>

third_party/blink/web_tests/external/wpt/web-nfc/NDEFReader_scan_filter.https.html

@@ -162,79 +162,4 @@ for (let multiMessagesTest of multiMessagesTests) {
   );
 }
 
-nfc_test(async (t, mockNFC) => {
-  const reader = new NDEFReader();
-  const controller = new AbortController();
-  const signal = controller.signal;
-  const textMsg = createMessage([createTextRecord(test_text_data)]);
-  const urlMsg = createMessage([createUrlRecord(test_url_data)]);
-  const mimeMsg = createMessage([createMimeRecord(test_buffer_data)]);
-
-  const readerWatcher = new EventWatcher(t, reader, ["reading", "error"]);
-  const promise = readerWatcher.wait_for("reading").then(event => {
-    controller.abort();
-    assertWebNDEFMessagesEqual(event.message, new NDEFMessage(mimeMsg));
-  });
-
-  const scanOptions1 = {recordType: "text", signal: signal};
-  const scanOptions2 = {recordType: "url", signal: signal};
-  const scanOptions3 = {recordType: "mime", signal: signal};
-
-  await reader.scan(scanOptions1);
-  await reader.scan(scanOptions2);
-  // There is maximum one filter for an NDEFReader object,
-  // last filter will replace all previous ones.
-  await reader.scan(scanOptions3);
-
-  mockNFC.setReadingMessage(textMsg);
-  mockNFC.setReadingMessage(urlMsg);
-  mockNFC.setReadingMessage(mimeMsg);
-  await promise;
-}, "Multiple scan() from the same NDEFReader object with new options should \
-replace existing filters.");
-
-nfc_test(async (t, mockNFC) => {
-  const reader = new NDEFReader();
-  const controller1 = new AbortController();
-  const controller2 = new AbortController();
-  const urlMsg = createMessage([createUrlRecord(test_url_data)]);
-  const mimeMsg = createMessage([createMimeRecord(test_buffer_data)]);
-
-  const readerWatcher = new EventWatcher(t, reader, ["reading", "error"]);
-  const promise = readerWatcher.wait_for("reading").then(event => {
-    assertWebNDEFMessagesEqual(event.message, new NDEFMessage(mimeMsg));
-    controller2.abort();
-  });
-
-  const scanOptions1 = {recordType: "url", signal: controller1.signal};
-  const scanOptions2 = {recordType: "mime", signal: controller2.signal};
-
-  // There is maximum one filter for an NDEFReader object,
-  // last filter will replace all previous ones.
-  await reader.scan(scanOptions1);
-  await reader.scan(scanOptions2);
-
-  mockNFC.setReadingMessage(urlMsg);
-
-  controller1.abort();
-
-  mockNFC.setReadingMessage(mimeMsg);
-  await promise;
-}, "Aborting on previous signal should not stop current reading for multiple \
-scan() with different signals.");
-
-nfc_test(async (t, mockNFC) => {
-  const reader = new NDEFReader();
-  const controller = new AbortController();
-  const scanOptions1 = {recordType: "url", signal: controller.signal };
-  const scanOptions2 = {recordType: "mime"};
-
-  await reader.scan(scanOptions1);
-  const promise = reader.scan(scanOptions2);
-  controller.abort();
-
-  await promise_rejects_dom(t, 'AbortError', promise);
-}, "Aborting on previous signal can stop current reading if no new signals \
-passed to successive scan().");
-
 </script>