Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notification when a browsing context is closed? #57

Closed
mrj opened this issue Sep 28, 2015 · 18 comments
Closed

Notification when a browsing context is closed? #57

mrj opened this issue Sep 28, 2015 · 18 comments
Labels
Origin Trial Issues that would be good to solve before OT

Comments

@mrj
Copy link

mrj commented Sep 28, 2015

The API doesn't currently state what happens to active pushMessage() and watch() promises when their browser window is closed. Are the promises explicitly rejected with an AbortError (with the window.closed property indicating the reason), or is there only the separate unload event on the window object? If the former, do the aborts come before or after the window unload event?

If another document already specifies the behaviour of promises when their window is closed, this document should be referenced in the pushMessage() and watch() algorithm sections.

@kenchris
Copy link
Contributor

That is a good point. What would you think would be the right behaviour? What do other similar apis do?

@zolkis
Copy link
Contributor

zolkis commented Sep 28, 2015

Since both pushMessage and watch algorithms depend on adapter suspended state, which depends on the Window objects, when the Window is closed, I expect implementations reject the pending push Promises, cancel the pending push buffers/timers and also the list of active watches. We should add these steps as a separate algorithm. One way would be to add the pending push Promises as internal slots to the NFCAdapter object, and define unload behavior, part of which is rejecting pending Promises.

@mrj
Copy link
Author

mrj commented Oct 2, 2015

Currently being discussed at w3ctag/promises-guide#44. What we have now probably needs to change.

@zolkis
Copy link
Contributor

zolkis commented Oct 2, 2015

Reopening.

@zolkis zolkis reopened this Oct 2, 2015
@zolkis
Copy link
Contributor

zolkis commented Oct 2, 2015

Based on implementation feedback, we do need to specify the "release NFC adapter" steps anyway. Window unload behavior indeed needs to change. What @domenic suggested there makes sense, I make the changes now.

@zolkis
Copy link
Contributor

zolkis commented Oct 2, 2015

BTW thanks a lot @mrj, this was helpful.

@zolkis
Copy link
Contributor

zolkis commented Oct 2, 2015

@mrj, could you please review #59 ? It should fix this.

@mrj
Copy link
Author

mrj commented Oct 5, 2015

Further comments by a Mozilla engineer at w3ctag/promises-guide#44 have cast doubt on the earlier microtask statements. The event loop could still be running on unload.

I'd prefer the Web NFC API to confine itself to what API users will experience, rather than describing the NFCAdapter object release process. For example, if we don't want to support synchronous AJAX calls during unload, which are the only useful things than can be done during unload, but which disgust the promise-guide guys, the section could read...


10.4 Response to Document Unload

Once a Document is to be unloaded (its browsing context has either been closed or has been navigated away from the Document, and the unload has not been canceled by a user prompt requested by a beforeunload handler), the adapter is suspended, and any pending requestAdapter(), pushMessage(), or watch() promises, as well as any pending Web NFC promises created in event handlers during the unload process, remain pending during the remainder of script execution.

If a Web NFC message is being transmitted at the time an unload is initiated, transmission of that message is stopped. [can native do this?]


By the way, in Section 10.3, where it says that on suspension "no NFC content is pushed", it should be made clear whether this means no new NFC content is pushed, or whether in-progress transmissions are also terminated or suspended. Ditto for cancelPush().

@zolkis
Copy link
Contributor

zolkis commented Oct 6, 2015

I think the only thing we should do is remove the Note about the microtasks. Specifying what should happen as additional unload steps actually is quite ok.

About Section 10.3., I think we could say "no NFC content is pushed, and no received NFC content is presented via watches while suspended, as specified by the pushMessage(link) and watch(link) steps".

The rest is defined by the NFCAdapter suspended state and the algorithms for push (and watch).

If something is already set, it is not pushed on the next tap. The pushMessage() steps define the exact behaviour in step 13 (.1): if the adapter is suspended before the tap happens, it is not pushed, but if the page is unloaded during a tap, the interrupting the ongoing transfer is on best effort, most likely it is not interrupted. In fact, the spec cannot mandate that the UA guarantees canceling ongoing transfers. Same for cancelPush(). The Promise will tell whether there was success or not (if it manages to complete) so clients can act accordingly. When the state is suspended, timers continue running (on the native side) and the implementation should be able to sort out what to do when they expire vs when the Document is closed.

I don't think we should not specify exact Promise behavior in this spec, especially if is not yet completely specified elsewhere. For implementations the important bit is how to handle adapter release (in fact even that is an implementation detail).

We will take into account implementation feedback.

@zolkis
Copy link
Contributor

zolkis commented Oct 6, 2015

Since we are not supporting workers, when a document is closed, I think the only thing that really matters is the user experience on the next document opened which uses Web NFC, and it is implementation detail how to make a clean start for the new page.

@mrj
Copy link
Author

mrj commented Oct 6, 2015

OK.

My concerns are, (1), that developers won't now know whether they'll have to handle rejection of pending promises on unload (explicitly state implementation-dependence?), and (2), that an NFC message can take up to 22 hours to transmit (per-record, message length is unlimited?), so there can be 22 hours of uncertainty whether data is being transmitted if it's unstated what happens to in-progress transmissions on a cancelPush, a valid concern for the clean slate of the next document that you've mentioned.

@zolkis
Copy link
Contributor

zolkis commented Oct 7, 2015

For 1), in the case of document unload, which is the script execution environment you want to see Promises handled in? IMO developers will only need to handle Promises, and not the corner cases when they are assumed not working. IMO it is not this specification which should deal with this. We return a Promise: under it the implementation is responsible, above it the developers.

  1. is invalid, no NFC transaction takes 22 hours. In most tag types it is sub-second. With Type 3 it could be theoretically as long as a minute, but in practice is also sub-second or second magnitude. If transmission is interrupted, you will get an error.
    The concern you mention is owned by implementations, and not the spec.

@mrj
Copy link
Author

mrj commented Oct 7, 2015

For (1), the API should give developers as much detail as possible on what promise abort situations they have to handle, otherwise they might be making wrong assumptions about the cause of an abort. If it's browser-dependent whether a cancelPush done by the API implementation on unload gets seen by a catch block, then developers should be told that so they can write their code to handle that possibility. My preference is to instead leave the promises pending (no cancelPush), and to state that, so developers don't have to consider the unload case.

As for (2), I'm thinking about peer-to-peer. A 4GB max record payload takes 22h to transmit at the NFC data rate, and a message can have multiple records. If cancellation of ongoing transfers by a cancelPush is implementation-dependent, again that should be stated in the API, whose aim should be to minimise developer uncertainty.

@zolkis
Copy link
Contributor

zolkis commented Oct 7, 2015

I agree the spec should list all errors that can occur when rejecting Promises. If we are missing some, please point them out.

For security reasons, only foreground NFC operation is allowed.

If the page is closed, i.e. the user is not interested any more in Web NFC, IMO the only thing to be done right is cleanup. The lifecycle of NFCAdapter is bound to that of Window. When/if service workers will be supported, this will change.

When transfer fails because the user physically broke range, or because a communication error, that case in handled by the spec by rejecting Promise.

If the transfer takes too long, and the user does not break range, implementations are not supposed to stop transmission for any reason, and as a matter of fact they cannot: we cannot implement interrupting ongoing transfers. As a developer, you don't need to implement a button calling cancelPush() function, since it is much easier for the user to break range than push a button. IMO this is a non-use-case.

Besides, NFC was never meant for 4GB data transfers, but doing handover in that case and do the transfer over WiFi or Bluetooth. 22 hrs long transfers are a non-use-case for the current version of the spec. Future versions may address handover.

In #62 I have added a note to cancelPush() steps, in order to avoid misunderstandings. In #63 I made explicit steps for handling timers and termination.

@kenchris
Copy link
Contributor

@zolkis can you close this and file follow up issues if needed?

@zolkis
Copy link
Contributor

zolkis commented Feb 26, 2019

I think we should ask @mrj if the current version of the spec has clarified the concerns.

@kenchris
Copy link
Contributor

kenchris commented Aug 5, 2019

@Honry can you check whether this is handled?

@kenchris kenchris added the Origin Trial Issues that would be good to solve before OT label Aug 5, 2019
@Honry
Copy link
Contributor

Honry commented Aug 5, 2019

I think we've handled all these, but may still need @mrj's feedback if he has further concerns.

Anyway, I suggest to close this issue by now as the spec has been refreshed too much, if there're further concerns, we can just create a new issue.

@kenchris kenchris closed this as completed Aug 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Origin Trial Issues that would be good to solve before OT
Projects
None yet
Development

No branches or pull requests

4 participants