Fix #592: allow multiple readers but no multiple scans on any reader

[zolkis]

Signed-off-by: Zoltan Kis [email protected]

---

Preview | Diff

[beaufortfrancois]

I'm confused with the algorithm.
See my notes.

[beaufortfrancois]

I think we should also abort steps below when promise gets rejected right?

[zolkis]

I don't understand.

[beaufortfrancois]

Same here, we should return |p| with rejected promise if failing.

[zolkis]

No, we just reject the pending scan, not this one. It's a cleanup operation.
Or do you mean if the abort fails, we should reject the scan?
i.e. should we handle the "ongoing operation" step as a fail to abort and propagate it up?

[beaufortfrancois]

To be clear, here are web platform tests for this issue:

nfc_test(async (t, mockNFC) => {
  const ndef = new NDEFReader();
  const promise1 = ndef.scan();
  const promise2 = promise_rejects_dom(t, 'InvalidStateError', ndef.scan());
  await promise1;
  await promise2;
}, "Test that NDEFReader.scan rejects if there is already an ongoing scan.");

nfc_test(async (t, mockNFC) => {
  const ndef = new NDEFReader();
  const controller = new AbortController();
  await ndef.scan({signal : controller.signal});
  controller.abort();
  await ndef.scan();
}, "Test that NDEFReader.scan can be started after the previous scan is aborted.");

[beaufortfrancois]

@zolkis rightly pointed out that write behaviour is different:

nfc_test(async (t, mockNFC) => {
  const ndef1 = new NDEFReader();
  const ndef2 = new NDEFReader();

  const p1 = ndef1.write(test_text_data, {overwrite: false});
  const p2 = ndef2.write(test_url_data, {overwrite: true});

  await new Promise((resolve, reject) => {
    // Make first push pending
    mockNFC.setPendingPushCompleted(false);
    let err = "";
    p1.then(() => {
      reject("pending push should not be fulfilled");
    }).catch(e => {
      err = e.name;
    });
    p2.then(() => {
      assertNDEFMessagesEqual(test_url_data, mockNFC.pushedMessage());
      assertNDEFWriteOptionsEqual( {overwrite: true}, mockNFC.writeOptions());
      assert_equals(err, "AbortError", "the pending push should be aborted");
      resolve();
    });
  });
}, "NDEFReader.write should replace all previously configured write operations.");

[zolkis]

I think it makes more sense to replace the previous ops with the current ones, rather than failing the current ones if old ones are ongoing. Saves an explicit abort from the app script. But I am open to be convinced.

[zolkis]

What's next? Merge this and update tests?

[beaufortfrancois]

Consistency with write() behaviour seems better to me. @riju @kenchris WDYT?

[kenchris]

+1 consistency

[zolkis]

Consistency which way?

If we align to write behavior, this PR is correct. As the title also tells, scan rejects prior scans, i.e. replaces them.

[beaufortfrancois]

In example below, how does user know that ndef1 is not listening to reading events anymore? Or is this what we want?

const ndef1 = new NDEFReader();
await ndef1.scan(); // Promise resolves and reading occurs...
ndef1.onreading => ({message}) => { /* Handle NDEF message */}

// few seconds later
const ndef2 = new NDEFReader();
await ndef2.scan(); // Promise resolves and reading occurs
ndef2.onreading => ({message}) => { /* Handle NDEF message */}

// ndef2 will get reading events.
// How about ndef1?

[zolkis]

Valid observations.

In this version of the API, and in this PR, if ndef1.scan() has already resolved and ndef2.scan() is called, the events are NOT coming through ndef1's event since the "abort a pending scan" algorithm steps.

But we don't have a way to notify ndef1 about it having been stopped. Using the onreadingerror event would be a misuse of it. Another pain point in this design. Could be avoided if we had a direct API like resolving the scan() promise with the data read. However, that would not support reading multiple tags (like a design with events does), unless that promise resolved with an array of NDEFMessage's.

(Sorry, this was edited twice).

[zolkis]

On a second thought, we could use an AbortError reported via onreadingerror to signal scan cancellation by a newer scan. At least scripts can react to that (even though it was the same script that made that condition by invoking a newer scan, so the point is kind of moot). Should I include that step in the "abort a pending scan" algo or should we continue with the current way of silently canceling onreading event delivery?

[zolkis]

According to the discussion in #592, at this point of time people prefer to fail scans if there is an existing scan, but new writes will continue replace pending writes. The reason is that scans report data through an event, which is different from writes.

[beaufortfrancois]

I believe the pending read tuple should be per reader.
This is not what I'm understanding with latest patch.

See JS code below for my expectations.

const ndef1 = new NDEFReader();
await ndef1.scan(); // Promise resolves and reading occurs...
ndef1.onreading => ({message}) => { /* Handle NDEF message */}

// few seconds later
const ndef2 = new NDEFReader();
await ndef2.scan(); // Promise resolves and reading occurs
ndef2.onreading => ({message}) => { /* Handle NDEF message */}

// ndef2 will get reading events.
// ndef1 will also continue to get reading events.

const ndef = new NDEFReader();
const p1 = ndef.scan(); // Promise resolves.
const p2 = ndef.scan(); // Promise rejects right away as there's already a pending scan request

[beaufortfrancois]

@riju can you check my sayings?

[zolkis]

I believe the pending read tuple should be per reader.

No, the spec has stated for ages it was on the settings object, i.e. tied to NfcProxy / NfcImpl, not the readers.
https://w3c.github.io/web-nfc/#nfc-state-associated-with-the-settings-object

The scan sequence diagram also reinforces that.
https://sites.google.com/a/chromium.org/dev/developers/design-documents/web-nfc

[zolkis]

I believe the pending read tuple should be per reader.

There cannot be a pending read tuple per reader, since then we would store the promise + reader in the reader, which makes no sense.

What we could do in the spec is return to the "activated reader list" internal slot, since the implementation still has that one (meaning the "pending read tuple" internal slot was never implemented).

In the Chromium implementation, NfcProxy already has a DCHECK() guard to not include the same reader twice. So all policies have to be made at NfcReader level. This means the NFC spec has not been implemented according how it was specified (internal slot for pending reader at the settings object). NfcReader and NfcProxy still allow multiple readers, so it goes to the Android side that way. In NfcImpl, the watch() function it will return an error if the id already exists, so a repeated scan() will fail. Therefore the has_pending_scan_request_ is not needed, but it's an optimization. In the end, the implementation still has the "activated reader list" and tag content will be dispatched to all readers.

For now, I will try to align the spec to the existing implementation, but this will need to be cleaned up in the future.
That would satisfy the examples of @beaufortfrancois above.

[zolkis]

Yet another change in this PR scope (and name). Now we allow multiple readers, but at each reader, successive scan() invocations will fail.

[zolkis]

@riju @kenchris please check.

[beaufortfrancois]

@zolkis Please have a look at https://chromium-review.googlesource.com/c/chromium/src/+/2297559/5/third_party/blink/renderer/modules/nfc/ndef_reader.cc and ask help from @riju, the has_pending_scan_request_ check is only for when the ndef reader didn't settle yet. I believe* this PR doesn't reflect @riju's CL.

[zolkis]

@beaufortfrancois

has_pending_scan_request_ check is only for when the ndef reader didn't settle yet

Do you refer to this? https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/modules/nfc/ndef_reader.cc;drc=aca54053714d442febdd5f87a26c4055e20bd632;l=187
(it's another question what is the relation with the similarly named NDEFReader::HasPendingActivity()...)

But then see that the condition is bound to the existence of the reader after it checked was there a pending scan request. Also the design doc states there is only one scan in a reader active at any time.
https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/modules/nfc/ndef_reader.cc;drc=aca54053714d442febdd5f87a26c4055e20bd632;l=135

Could you point out which algorithmic steps are in conflict with the implementation?

[zolkis]

We had a call with Arno and Riju, and that clarified things. I discovered a non-related bug in the scan algo, fixed in the commit above.

The has_pending_scan_request_ flag indeed covers the time between the scan() invocation and its promise resolving, i.e. until the reader is added to the activated reader objects.
Arno tried removing the flag and resulted in the renderer crashing at a second invocation of scan() because it ran into the DCHECK() in the beginning of NfcProxy::StartReading(). Which IMHO is an implementation bug, since it was supposed to fail the promise on that condition, not to crash the renderer. The has_pending_scan_request_ seems to be a workaround for that bug, not an algorithmic necessity.

In the scan algorithm in the spec is IMHO correct: the condition on reader being in the activated reader list is sufficient since the reading algorithm is triggered on the same condition. Single point of exclusion is desired, so the algorithm is correct. We don't need to factor out a separate internal slot to reflect the current implementation behavior that uses has_pending_scan_request_.

The existing WPT should be working as defined with the current specification of the scan algorithm.

[beaufortfrancois]

Arno tried removing the flag and resulted in the renderer crashing at a second invocation of scan() because it ran into the DCHECK() in the beginning of NfcProxy::StartReading(). Which IMHO is an implementation bug, since it was supposed to fail the promise on that condition, not to crash the renderer. The has_pending_scan_request_ seems to be a workaround for that bug, not an algorithmic necessity.

Crashing will only happen on Debug builds (hence DCHECK) and is only for chromium developers to debug non expected cases.

The existing WPT should be working as defined with the current specification of the scan algorithm.

I believe we should update one web platform test to make sure it's fully working:

 nfc_test(async (t, mockNFC) => {
   const ndef = new NDEFReader();
   const controller = new AbortController();
   await ndef.scan({signal : controller.signal});
+  await promise_rejects_dom(t, 'InvalidStateError', ndef.scan());
   controller.abort();
   await ndef.scan();
 }, "Test that NDEFReader.scan can be started after the previous scan is aborted.");

[zolkis]

Thanks for the updates.

"Test that NDEFReader.scan can be started after the previous scan is aborted."

Description needs update as well? Isn't that testing 3 scans? Tests if the second scan() fails since the first is started, and the third can be started once the first is aborted? Maybe split in 2 test cases?

[beaufortfrancois]

Description needs update as well? Isn't that testing 3 scans? Tests if the second scan() fails since the first is started, and the third can be started once the first is aborted? Maybe split in 2 test cases?

I've ended up updating another test. See web-platform-tests/wpt#27014

[beaufortfrancois]

Thank you @zolkis!