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

Are unresolved promises rejected prior to a window.unload event? #44

Open
mrj opened this issue Sep 29, 2015 · 12 comments
Open

Are unresolved promises rejected prior to a window.unload event? #44

mrj opened this issue Sep 29, 2015 · 12 comments
Assignees

Comments

@mrj
Copy link

mrj commented Sep 29, 2015

What is best practice regarding the handling of unresolved promises when a browsing context is closed (nav away, window close)? Should the promises be explicitly rejected, and if so, what Error object should be returned so the reason for the rejection can be determined? Or should the promises remain unresolved, with only a window.unload event generated? And if rejected, what is the timing of the rejection in relation to the unload event?

This relates to development of the Web NFC API: w3c/web-nfc#57

@domenic
Copy link
Member

domenic commented Oct 1, 2015

Interesting question!

As the spec is structured right now, unloading does not run any code except the unload event. And, the unload event happens before any other specifications get to run their "unloading document cleanup steps". So right now, I don't think it is even possible for to choose the answer of a promise being rejected. (Or rather, it is possible, but if you rejected the promise, none of the handlers would ever fire, since after step 11 of the loading process, we never perform a microtask checkpoint again.)

I suspect that this design is somewhat intentional; unloading is a complicated process, and we want to run as little script as possible during it. But I do not know the full history.

In theory we could restructure the spec to make rejection work. E.g., we could add "perform a microtask checkpoint" after step 11. (Or, I guess you could even have your spec's document cleanup steps perform a microtask checkpoint.) But, re-entering the event loop during the very last phases of unloading seems bad...

@bzbarsky may have more insights on this.

@bzbarsky
Copy link

bzbarsky commented Oct 1, 2015

I'm generally opposed to pages running any script in unload; it's rarely used for anything good. I'd remove the unload event if I could. Does that help? ;)

@domenic
Copy link
Member

domenic commented Oct 1, 2015

Yeah, I think that settles it. So @mrj, just leave the promises pending; no need to add cleanup steps, since script wouldn't be able to react to them anyway.

Unsure how or whether to incorporate this into the promises guide... it's kind of a niche thing. Maybe it'd be worthwhile noting in some way that promise callbacks cannot run during unload.

@jan-ivar
Copy link

jan-ivar commented Oct 1, 2015

On the off chance there's a general design question here, we had a similar (but not the same) question in WebRTC where promise-returning methods on the RTCPeerConnection object are even allowed to queue up in certain circumstances. After pc.close() though, outstanding promises are defined to go silently nowhere (we have no language on unload).

@mrj
Copy link
Author

mrj commented Oct 1, 2015

Thanks for your thoughts.

I'm just thinking of practical uses for explicit rejection: making it easy to process the fact and the reason (fired unload flag of document is set or window.closed property set?) that NFC data hasn't been sent or received -- to warn the user (possibly so the unload can be stopped), to save unsent data, to substitute unreceived data, or just for analytic purposes. Without rejection, one would have to do this in an unload handler via variables that enumerate the active promises.

This is what has currently been chosen as the unload behaviour of the Web NFC API.

@domenic
Copy link
Member

domenic commented Oct 1, 2015

That behavior seems quite bad. It is a may, so it varies across browsers. It cancels things even though an unload might not happen, so if there's a beforeunload handler that prevents unloading, you still shut down all your NFC stuff. And it uses the undefined terminology "when an object receives the x event," which doesn't mean anything.

You should instead use the unloading document cleanup steps to run that, and probably a note saying "since no microtasks run during document unload, the promise rejections that happen are not observable."

@mrj
Copy link
Author

mrj commented Oct 2, 2015

Although no user interaction is possible in a beforeunload or unload handler, except for beforeunload being able to trigger a stay/leave page prompt (which can be preceded by DOM manipulation), synchronous AJAX requests are possible (but the event to which it should be attached seems to be browser-dependent). So there's some use in rejecting promises prior to resource clean-up.

I agree that it's wrong to specify that the NFC shutdown (including promise rejection) MAY happen during beforeunload, both for the developer uncertainty and the unload cancelability reasons you mention. The choices are to do it at the Step 11 clean-up step (why does this make the rejections unobservable?), or to allocate a promise rejection step after unload confirmation, during which an AJAX call, preceded by DOM changes such as a "saving" overlay, can be made, with clean-up done in its usual later step.

And, as you mention, you can't really attach this NFC shutdown algorithm to a particular event. It has to be attached to a browser micro-task sequence. So either NFC has to go it alone in specifying a rejection step, or a standard way needs to be specified.

@domenic
Copy link
Member

domenic commented Oct 2, 2015

why does this make the rejections unobservable?

Because no microtasks run after step 11. The event loop has already shut down, so any handlers queued with .catch(handler) will never be run, since they are put on the event loop's microtask queue.

allocate a promise rejection step after unload confirmation, during which an AJAX call, preceded by DOM changes such as a "saving" overlay, can be made, with clean-up done in its usual later step.

Please don't do this. You've heard explicitly from an implementer that authors should not run code while the browsing context is being unloaded. You should not be adding patterns to encourage that kind of problematic author code. Especially not if you are explicitly doing it to encourage synchronous Ajax calls.

@mrj
Copy link
Author

mrj commented Oct 2, 2015

If it's widely believed that synchronous Ajax calls during unload are intrinsically bad (because they cause a delay or lock-up at an unexpected time?), then there's no good reason to reject promises.

Determined developers will still hack around this by attaching to the unload or beforeunload events.

@bzbarsky
Copy link

bzbarsky commented Oct 2, 2015

Because no microtasks run after step 11.

Why does this matter? The event loop is not per-document, it's per-group-of-related-documents, no?

The event loop has already shut down

Where do you see this happening?

Note that ES6 doesn't seem to tie the Job queue to any particular Realm either, so it's assuming that all Realms that are relevant share a single Job queue.

@bzbarsky
Copy link

bzbarsky commented Oct 2, 2015

That all said, the problem of promises from unloading pages (or indeed script invocation in unloaded pages in general) not being specified very well and not being interoperably implemented is very real. See also https://bugzilla.mozilla.org/show_bug.cgi?id=1058695 where we ended up putting in some mitigations in Gecko that technically don't follow the spec, because technically following the spec requires leaking the world in common cases.... The problem is that there is no spec for this event loop stuff right now, and the spec for Promise is part of ES6, which doesn't really doesn't admit the possibility of Realms needing to go away in some sense, so there's nothing to even raise such issues against. :(

@atanassov
Copy link

@plinss and myself discussed this issue as part of our May 2020 virtual f2f.

Since the issue was raised and since agreed that there's no great way or reason to reject promises after unload we propose adding a new section to the Promises Guide. We should advise authors and users that promises may not resolve in circumstance that are considered 'not-normal' from the POV of user code execution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants