[webnfc] Generate watch ids in Blink rather than DeviceService

Previously watch ids were generated in the DeviceService. This CL
switches to do that in Blink side, more specifically NFCProxy is
responsible to generate/manage them, and still keeps their uniqueness
over a given Mojo connection of device.mojom.NFC interface.

This makes it easier for NFCProxy to track the set of active readers,
specifically the 2 problems below can get solved.

1.
  a. NFCReader#start() --> device.mojom.NFC.Watch(options), we'll get
     the watch id via the response callback.
  b. in DeviceService the NFCImpl Watch() generates WATCH_ID_XXX and
     passes it back via the WatchResponse callback.
  c. in DeviceService NFCImpl gets a matched NDEFMessage from a NFC tag
     and call device.mojom.NFCClient.OnWatch(WATCH_ID_XXX, ndef_message).
As NFC and NFCClient run on separated Mojo message pipes, the Mojo
messages sent by the above a and b do not have any ordering guarantee,
i.e. c may gets to NFCProxy earlier than b, then, because NFCProxy did
not get WATCH_ID_XXX yet, it can not find the corresponding reader
that should actually be the receiver of this NDEFMessage.

2.
  a. NFCReader#start() --> device.mojom.NFC.Watch(options)
  b. NFCReader#stop() --> can not call device.mojom.NFC.CancelWatch(id)
     because we did not get the watch id yet.
  c. the WATCH_ID_XXX for step a above comes back.
This just leaves the watch session of WATCH_ID_XXX long live in NFCImpl
without any chance to cancel it.

BUG=520391

Change-Id: I136ee6aab54eb464e5dd684ed34a2d738d2f5198
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1730515
Commit-Queue: Leon Han <leon.han@intel.com>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#683914}

services/device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java

@@ -164,7 +164,7 @@ public class NfcImpl implements Nfc {
     /**
      * Sets NfcClient. NfcClient interface is used to notify mojo NFC service client when NFC
      * device is in proximity and has NdefMessage that matches NfcReaderOptions criteria.
-     * @see Nfc#watch(NfcReaderOptions options, WatchResponse callback)
+     * @see Nfc#watch(NfcReaderOptions options, int id, WatchResponse callback)
      *
      * @param client @see NfcClient
      */
@@ -237,20 +237,25 @@ public class NfcImpl implements Nfc {
 
     /**
      * Watch method allows to set filtering criteria for NdefMessages that are found when NFC device
-     * is within proximity. On success, watch ID is returned to caller through WatchResponse
-     * callback. When NdefMessage that matches NfcReaderOptions is found, it is passed to NfcClient
-     * interface together with corresponding watch ID.
+     * is within proximity. When NdefMessage that matches NfcReaderOptions is found, it is passed to
+     * NfcClient interface together with corresponding watch ID.
      * @see NfcClient#onWatch(int[] id, String serial_number, NdefMessage message)
      *
      * @param options used to filter NdefMessages, @see NfcReaderOptions.
-     * @param callback that is used to notify caller when watch() is completed and return watch ID.
+     * @param callback that is used to notify caller when watch() is completed.
      */
     @Override
-    public void watch(NfcReaderOptions options, WatchResponse callback) {
+    public void watch(NfcReaderOptions options, int id, WatchResponse callback) {
         if (!checkIfReady(callback)) return;
-        int watcherId = ++mWatcherId;
-        mWatchers.put(watcherId, options);
-        callback.call(watcherId, null);
+        // We received a duplicate |id| here that should never happen, in such a case we should
+        // report a bad message to Mojo but unfortunately Mojo bindings for Java does not support
+        // this feature yet. So, we just passes back a generic error instead.
+        if (mWatchers.indexOfKey(id) >= 0) {
+            callback.call(createError(NfcErrorType.NOT_READABLE));
+            return;
+        }
+        mWatchers.put(id, options);
+        callback.call(null);
         enableReaderModeIfNeeded();
         processPendingWatchOperations();
     }
@@ -393,7 +398,7 @@ public class NfcImpl implements Nfc {
         NfcError error = checkIfReady();
         if (error == null) return true;
 
-        callback.call(0, error);
+        callback.call(error);
         return false;
     }
 

services/device/public/mojom/nfc.mojom

@@ -124,8 +124,9 @@ interface NFC {
   CancelPush(NFCPushTarget target) => (NFCError? error);
 
   // Starts watching for nearby NFC devices with data that matches
-  // NFCReaderOptions filtering criteria. On success, watch id is returned.
-  Watch(NFCReaderOptions options) => (uint32 id, NFCError? error);
+  // NFCReaderOptions filtering criteria. |id| identifies each watch request on
+  // the current Mojo connection.
+  Watch(NFCReaderOptions options, uint32 id) => (NFCError? error);
 
   // Cancels watch operation with provided id.
   CancelWatch (uint32 id) => (NFCError? error);

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

@@ -165,11 +165,12 @@ ScriptPromise NFC::watch(ScriptState* script_state,
 
   auto* resolver = MakeGarbageCollected<ScriptPromiseResolver>(script_state);
   requests_.insert(resolver);
-  auto watch_callback =
-      WTF::Bind(&NFC::OnWatchRegistered, WrapPersistent(this),
-                WrapPersistent(callback), WrapPersistent(resolver));
+  auto watch_callback = WTF::Bind(&NFC::OnWatchRegistered, WrapPersistent(this),
+                                  WrapPersistent(callback),
+                                  WrapPersistent(resolver), next_watch_id_);
   nfc_->Watch(device::mojom::blink::NFCReaderOptions::From(options),
-              std::move(watch_callback));
+              next_watch_id_, std::move(watch_callback));
+  next_watch_id_++;
   return resolver->Promise();
 }
 
@@ -179,15 +180,14 @@ ScriptPromise NFC::cancelWatch(ScriptState* script_state, int32_t id) {
   if (!promise.IsEmpty())
     return promise;
 
-  if (id) {
-    callbacks_.erase(id);
-  } else {
+  if (!callbacks_.Contains(id)) {
     return ScriptPromise::RejectWithDOMException(
         script_state,
         MakeGarbageCollected<DOMException>(DOMExceptionCode::kNotFoundError,
                                            kNfcWatchIdNotFound));
   }
 
+  callbacks_.erase(id);
   auto* resolver = MakeGarbageCollected<ScriptPromiseResolver>(script_state);
   requests_.insert(resolver);
   nfc_->CancelWatch(id,
@@ -310,16 +310,6 @@ void NFC::OnWatchRegistered(V8MessageCallback* callback,
                             device::mojom::blink::NFCErrorPtr error) {
   requests_.erase(resolver);
 
-  // Invalid id was returned.
-  // https://w3c.github.io/web-nfc/#dom-nfc-watch
-  // 8. If the request fails, reject promise with "NotSupportedError"
-  // and abort these steps.
-  if (!id) {
-    resolver->Reject(NFCErrorTypeToDOMException(
-        device::mojom::blink::NFCErrorType::NOT_SUPPORTED));
-    return;
-  }
-
   if (error.is_null()) {
     callbacks_.insert(id, callback);
     resolver->Resolve(id);

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

@@ -93,6 +93,10 @@ class NFC final : public ScriptWrappable,
   HeapHashSet<Member<ScriptPromiseResolver>> requests_;
   using WatchCallbacksMap = HeapHashMap<uint32_t, Member<V8MessageCallback>>;
   WatchCallbacksMap callbacks_;
+
+  // Identifies watch requests tied to a given Mojo connection of NFC interface.
+  // Incremented each time a watch request is made.
+  uint32_t next_watch_id_ = 1;
 };
 
 }  // namespace blink

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

@@ -16,10 +16,6 @@
 
 namespace blink {
 
-namespace {
-constexpr uint32_t kInvalidNfcWatchId = 0;
-}
-
 // static
 const char NFCProxy::kSupplementName[] = "NFCProxy";
 
@@ -65,16 +61,18 @@ void NFCProxy::StartReading(NFCReader* reader) {
 
   EnsureMojoConnection();
   nfc_->Watch(device::mojom::blink::NFCReaderOptions::From(reader->options()),
+              next_watch_id_,
               WTF::Bind(&NFCProxy::OnReaderRegistered, WrapPersistent(this),
-                        WrapPersistent(reader)));
-  readers_.insert(reader, kInvalidNfcWatchId);
+                        WrapPersistent(reader), next_watch_id_));
+  readers_.insert(reader, next_watch_id_);
+  next_watch_id_++;
 }
 
 void NFCProxy::StopReading(NFCReader* reader) {
   DCHECK(reader);
   auto iter = readers_.find(reader);
   if (iter != readers_.end()) {
-    if (nfc_ && iter->value != kInvalidNfcWatchId) {
+    if (nfc_) {
       // We do not need to notify |reader| of anything.
       nfc_->CancelWatch(iter->value,
                         device::mojom::blink::NFC::CancelWatchCallback());
@@ -108,36 +106,41 @@ void NFCProxy::CancelPush(
 }
 
 // device::mojom::blink::NFCClient implementation.
-void NFCProxy::OnWatch(const Vector<uint32_t>& ids,
+void NFCProxy::OnWatch(const Vector<uint32_t>& watch_ids,
                        const String& serial_number,
                        device::mojom::blink::NDEFMessagePtr message) {
-  // Dispatch the event to all matched readers. Of course readers with
-  // kInvalidNfcWatchId do not match.
+  // Dispatch the event to all matched readers.
   // This loop is O(n^2), however, we assume the number of readers to be small
   // so it'd be just OK.
   for (auto pair : readers_) {
-    if (ids.Contains(pair.value)) {
+    if (watch_ids.Contains(pair.value)) {
       pair.key->OnReading(serial_number, *message);
     }
   }
 }
 
 void NFCProxy::OnReaderRegistered(NFCReader* reader,
-                                  uint32_t id,
+                                  uint32_t watch_id,
                                   device::mojom::blink::NFCErrorPtr error) {
   DCHECK(reader);
   // |reader| may have already stopped reading.
   if (!readers_.Contains(reader))
     return;
 
+  // |reader| already stopped reading for the previous |watch_id| request and
+  // started a new one, let's just ignore this response callback as we do not
+  // need to notify |reader| of anything for an obsoleted session.
+  if (readers_.at(reader) != watch_id)
+    return;
+
   if (error) {
     reader->OnError(error->error_type);
     readers_.erase(reader);
     return;
   }
 
-  DCHECK_NE(id, kInvalidNfcWatchId);
-  readers_.Set(reader, id);
+  // It's good the watch request has been accepted, we do nothing here but just
+  // wait for message notifications in OnWatch().
 }
 
 void NFCProxy::PageVisibilityChanged() {
@@ -204,12 +207,16 @@ void NFCProxy::OnMojoConnectionError() {
 
   // Notify all active readers about the connection error and clear the list.
   ReaderMap readers = std::move(readers_);
-  for (auto pair : readers) {
+  for (auto& pair : readers) {
     // The reader may call StopReading() to remove itself from |readers_| when
     // handling the error.
     pair.key->OnError(device::mojom::blink::NFCErrorType::NOT_READABLE);
   }
 
+  // Each connection maintains its own watch ID numbering, so reset to 1 on
+  // connection error.
+  next_watch_id_ = 1;
+
   // Notify all writers about the connection error.
   for (auto& writer : writers_) {
     writer->OnMojoConnectionError();

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

@@ -20,7 +20,7 @@ class NFCReader;
 class NFCWriter;
 
 // This is a proxy class used by NFCWriter(s) and NFCReader(s) to connect
-// to the DeviceService for device::mojom::blink::NFC service.
+// to implementation of device::mojom::blink::NFC interface.
 class MODULES_EXPORT NFCProxy final
     : public GarbageCollectedFinalized<NFCProxy>,
       public PageVisibilityObserver,
@@ -49,11 +49,10 @@ class MODULES_EXPORT NFCProxy final
   void StartReading(NFCReader*);
   void StopReading(NFCReader*);
   bool IsReading(const NFCReader*);
-  void Push(device::mojom::blink::NDEFMessagePtr message,
-            device::mojom::blink::NFCPushOptionsPtr options,
-            device::mojom::blink::NFC::PushCallback cb);
-  void CancelPush(const String& target,
-                  device::mojom::blink::NFC::CancelPushCallback callback);
+  void Push(device::mojom::blink::NDEFMessagePtr,
+            device::mojom::blink::NFCPushOptionsPtr,
+            device::mojom::blink::NFC::PushCallback);
+  void CancelPush(const String&, device::mojom::blink::NFC::CancelPushCallback);
 
  private:
   // Implementation of device::mojom::blink::NFCClient.
@@ -61,9 +60,9 @@ class MODULES_EXPORT NFCProxy final
                const String&,
                device::mojom::blink::NDEFMessagePtr) override;
 
-  void OnReaderRegistered(NFCReader* reader,
-                          uint32_t id,
-                          device::mojom::blink::NFCErrorPtr error);
+  void OnReaderRegistered(NFCReader*,
+                          uint32_t watch_id,
+                          device::mojom::blink::NFCErrorPtr);
 
   // Implementation of PageVisibilityObserver.
   void PageVisibilityChanged() override;
@@ -74,13 +73,17 @@ class MODULES_EXPORT NFCProxy final
   void UpdateSuspendedStatus();
   bool ShouldSuspendNFC() const;
 
-  // For the Mojo connection with the DeviceService.
   void EnsureMojoConnection();
+
+  // This could only happen when the embedder does not implement NFC interface.
   void OnMojoConnectionError();
 
-  // The <NFCReader, WatchId> map. An reader is inserted with the WatchId
-  // initially being 0, then we send a watch request to |nfc_|, the watch ID is
-  // then updated to the value (>= 1) passed to OnReaderRegistered().
+  // Identifies watch requests tied to a given Mojo connection of NFC interface,
+  // i.e. |nfc_|. Incremented each time a watch request is made.
+  uint32_t next_watch_id_ = 1;
+
+  // The <NFCReader, WatchId> map keeps all readers that have started reading.
+  // The watch id comes from |next_watch_id_|.
   using ReaderMap = HeapHashMap<WeakMember<NFCReader>, uint32_t>;
   ReaderMap readers_;
 

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

@@ -120,10 +120,10 @@ class FakeNfcService : public device::mojom::blink::NFC {
     std::move(callback).Run(nullptr);
   }
   void Watch(device::mojom::blink::NFCReaderOptionsPtr options,
+             uint32_t id,
              WatchCallback callback) override {
-    uint32_t id = ++last_watch_id_;
     watches_.insert(std::make_pair(id, std::move(options)));
-    std::move(callback).Run(id, nullptr);
+    std::move(callback).Run(nullptr);
   }
   void CancelWatch(uint32_t id, CancelWatchCallback callback) override {
     if (watches_.erase(id) < 1) {
@@ -142,7 +142,6 @@ class FakeNfcService : public device::mojom::blink::NFC {
 
   device::mojom::blink::NDEFMessagePtr tag_message_;
   device::mojom::blink::NFCClientPtr client_;
-  uint32_t last_watch_id_ = 0;
   std::map<uint32_t, device::mojom::blink::NFCReaderOptionsPtr> watches_;
   mojo::Binding<device::mojom::blink::NFC> binding_;
 };

third_party/blink/web_tests/nfc/resources/nfc-helpers.js

@@ -296,7 +296,6 @@ class MockNFC {
     this.push_timeout_id_ = null;
     this.push_completed_ = true;
     this.client_ = null;
-    this.watch_id_ = 0;
     this.watchers_ = [];
   }
 
@@ -347,17 +346,15 @@ class MockNFC {
     this.client_ = client;
   }
 
-  async watch(options) {
+  async watch(options, id) {
+    assert_true(id > 0);
     let error = this.isReady();
     if (error) {
-      error.id = 0;
       return error;
     }
 
-    let retVal = createNFCError(null);
-    retVal.id = ++this.watch_id_;
-    this.watchers_.push({id: this.watch_id_, options: options});
-    return retVal;
+    this.watchers_.push({id: id, options: options});
+    return createNFCError(null);
   }
 
   async cancelWatch(id) {
@@ -411,7 +408,6 @@ class MockNFC {
   reset() {
     this.hw_status_ = NFCHWStatus.ENABLED;
     this.push_completed_ = true;
-    this.watch_id_ = 0;
     this.watchers_ = [];
     this.cancelPendingPushOperation();
   }

third_party/blink/web_tests/nfc/watch.html

@@ -14,12 +14,12 @@ nfc_test(() => {
 
 nfc_test(() => {
   mockNFC.setHWStatus(NFCHWStatus.DISABLED);
-  return assertRejectsWithError(navigator.nfc.watch(noop), 'NotSupportedError');
+  return assertRejectsWithError(navigator.nfc.watch(noop), 'NotReadableError');
 }, 'Test that nfc.watch fails if NFC hardware is disabled.')
 
 nfc_test(() => {
   mockNFC.setHWStatus(NFCHWStatus.NOT_SUPPORTED);
-  return assertRejectsWithError(navigator.nfc.watch(noop), 'NotSupportedError');
+  return assertRejectsWithError(navigator.nfc.watch(noop), 'NotReadableError');
 }, 'Test that nfc.watch fails if NFC hardware is not supported.')
 
 nfc_test(async () => {